diff mbox

[tip/core/rcu,02/15] rcu: Size rcu_node tree from nr_cpu_ids rather than NR_CPUS

Message ID 1339794370-28119-2-git-send-email-paulmck@linux.vnet.ibm.com
State New
Headers show

Commit Message

Paul E. McKenney June 15, 2012, 9:05 p.m. UTC 
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

The rcu_node tree array is sized based on compile-time constants,
including NR_CPUS.  Although this approach has worked well in the past,
the recent trend by many distros to define NR_CPUS=4096 results in
excessive grace-period-initialization latencies.

This commit therefore substitutes the run-time computed nr_cpu_ids for
the compile-time NR_CPUS when building the tree.  This can result in
much of the compile-time-allocated rcu_node array being unused.  If
this is a major problem, you are in a specialized situation anyway,
so you can manually adjust the NR_CPUS, RCU_FANOUT, and RCU_FANOUT_LEAF
kernel config parameters.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutree.c        |    2 +-
 kernel/rcutree_plugin.h |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

Comments

Josh Triplett June 15, 2012, 9:47 p.m. UTC | #1 
On Fri, Jun 15, 2012 at 02:05:57PM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> 
> The rcu_node tree array is sized based on compile-time constants,
> including NR_CPUS.  Although this approach has worked well in the past,
> the recent trend by many distros to define NR_CPUS=4096 results in
> excessive grace-period-initialization latencies.
> 
> This commit therefore substitutes the run-time computed nr_cpu_ids for
> the compile-time NR_CPUS when building the tree.  This can result in
> much of the compile-time-allocated rcu_node array being unused.  If
> this is a major problem, you are in a specialized situation anyway,
> so you can manually adjust the NR_CPUS, RCU_FANOUT, and RCU_FANOUT_LEAF
> kernel config parameters.
> 
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  kernel/rcutree.c        |    2 +-
>  kernel/rcutree_plugin.h |    2 ++
>  2 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index a151184..9098910 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -2672,7 +2672,7 @@ static void __init rcu_init_geometry(void)
>  {
>  	int i;
>  	int j;
> -	int n = NR_CPUS;
> +	int n = nr_cpu_ids;

Same question as before: why have this as a variable when it never
changes?

- Josh Triplett
Paul E. McKenney June 16, 2012, 12:37 a.m. UTC | #2 
On Fri, Jun 15, 2012 at 02:47:26PM -0700, Josh Triplett wrote:
> On Fri, Jun 15, 2012 at 02:05:57PM -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > 
> > The rcu_node tree array is sized based on compile-time constants,
> > including NR_CPUS.  Although this approach has worked well in the past,
> > the recent trend by many distros to define NR_CPUS=4096 results in
> > excessive grace-period-initialization latencies.
> > 
> > This commit therefore substitutes the run-time computed nr_cpu_ids for
> > the compile-time NR_CPUS when building the tree.  This can result in
> > much of the compile-time-allocated rcu_node array being unused.  If
> > this is a major problem, you are in a specialized situation anyway,
> > so you can manually adjust the NR_CPUS, RCU_FANOUT, and RCU_FANOUT_LEAF
> > kernel config parameters.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  kernel/rcutree.c        |    2 +-
> >  kernel/rcutree_plugin.h |    2 ++
> >  2 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > index a151184..9098910 100644
> > --- a/kernel/rcutree.c
> > +++ b/kernel/rcutree.c
> > @@ -2672,7 +2672,7 @@ static void __init rcu_init_geometry(void)
> >  {
> >  	int i;
> >  	int j;
> > -	int n = NR_CPUS;
> > +	int n = nr_cpu_ids;
> 
> Same question as before: why have this as a variable when it never
> changes?

Ah, that explains why.  This prevented me from forgetting the random
NR_CPUS.

							Thanx, Paul
Josh Triplett June 16, 2012, 5:17 a.m. UTC | #3 
On Fri, Jun 15, 2012 at 05:37:14PM -0700, Paul E. McKenney wrote:
> On Fri, Jun 15, 2012 at 02:47:26PM -0700, Josh Triplett wrote:
> > On Fri, Jun 15, 2012 at 02:05:57PM -0700, Paul E. McKenney wrote:
> > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > 
> > > The rcu_node tree array is sized based on compile-time constants,
> > > including NR_CPUS.  Although this approach has worked well in the past,
> > > the recent trend by many distros to define NR_CPUS=4096 results in
> > > excessive grace-period-initialization latencies.
> > > 
> > > This commit therefore substitutes the run-time computed nr_cpu_ids for
> > > the compile-time NR_CPUS when building the tree.  This can result in
> > > much of the compile-time-allocated rcu_node array being unused.  If
> > > this is a major problem, you are in a specialized situation anyway,
> > > so you can manually adjust the NR_CPUS, RCU_FANOUT, and RCU_FANOUT_LEAF
> > > kernel config parameters.
> > > 
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > ---
> > >  kernel/rcutree.c        |    2 +-
> > >  kernel/rcutree_plugin.h |    2 ++
> > >  2 files changed, 3 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > > index a151184..9098910 100644
> > > --- a/kernel/rcutree.c
> > > +++ b/kernel/rcutree.c
> > > @@ -2672,7 +2672,7 @@ static void __init rcu_init_geometry(void)
> > >  {
> > >  	int i;
> > >  	int j;
> > > -	int n = NR_CPUS;
> > > +	int n = nr_cpu_ids;
> > 
> > Same question as before: why have this as a variable when it never
> > changes?
> 
> Ah, that explains why.  This prevented me from forgetting the random
> NR_CPUS.

Does that mean it can go away now that you've written the patches?

- Josh Triplett
Paul E. McKenney June 16, 2012, 6:38 a.m. UTC | #4 
On Fri, Jun 15, 2012 at 10:17:12PM -0700, Josh Triplett wrote:
> On Fri, Jun 15, 2012 at 05:37:14PM -0700, Paul E. McKenney wrote:
> > On Fri, Jun 15, 2012 at 02:47:26PM -0700, Josh Triplett wrote:
> > > On Fri, Jun 15, 2012 at 02:05:57PM -0700, Paul E. McKenney wrote:
> > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > > 
> > > > The rcu_node tree array is sized based on compile-time constants,
> > > > including NR_CPUS.  Although this approach has worked well in the past,
> > > > the recent trend by many distros to define NR_CPUS=4096 results in
> > > > excessive grace-period-initialization latencies.
> > > > 
> > > > This commit therefore substitutes the run-time computed nr_cpu_ids for
> > > > the compile-time NR_CPUS when building the tree.  This can result in
> > > > much of the compile-time-allocated rcu_node array being unused.  If
> > > > this is a major problem, you are in a specialized situation anyway,
> > > > so you can manually adjust the NR_CPUS, RCU_FANOUT, and RCU_FANOUT_LEAF
> > > > kernel config parameters.
> > > > 
> > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > ---
> > > >  kernel/rcutree.c        |    2 +-
> > > >  kernel/rcutree_plugin.h |    2 ++
> > > >  2 files changed, 3 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > > > index a151184..9098910 100644
> > > > --- a/kernel/rcutree.c
> > > > +++ b/kernel/rcutree.c
> > > > @@ -2672,7 +2672,7 @@ static void __init rcu_init_geometry(void)
> > > >  {
> > > >  	int i;
> > > >  	int j;
> > > > -	int n = NR_CPUS;
> > > > +	int n = nr_cpu_ids;
> > > 
> > > Same question as before: why have this as a variable when it never
> > > changes?
> > 
> > Ah, that explains why.  This prevented me from forgetting the random
> > NR_CPUS.
> 
> Does that mean it can go away now that you've written the patches?

If I don't have to change from nr_cpu_ids to yet another thing over
the next while, then it might be worth changing.

							Thanx, Paul
Josh Triplett June 16, 2012, 9:17 a.m. UTC | #5 
On Fri, Jun 15, 2012 at 11:38:48PM -0700, Paul E. McKenney wrote:
> On Fri, Jun 15, 2012 at 10:17:12PM -0700, Josh Triplett wrote:
> > On Fri, Jun 15, 2012 at 05:37:14PM -0700, Paul E. McKenney wrote:
> > > On Fri, Jun 15, 2012 at 02:47:26PM -0700, Josh Triplett wrote:
> > > > On Fri, Jun 15, 2012 at 02:05:57PM -0700, Paul E. McKenney wrote:
> > > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > > > 
> > > > > The rcu_node tree array is sized based on compile-time constants,
> > > > > including NR_CPUS.  Although this approach has worked well in the past,
> > > > > the recent trend by many distros to define NR_CPUS=4096 results in
> > > > > excessive grace-period-initialization latencies.
> > > > > 
> > > > > This commit therefore substitutes the run-time computed nr_cpu_ids for
> > > > > the compile-time NR_CPUS when building the tree.  This can result in
> > > > > much of the compile-time-allocated rcu_node array being unused.  If
> > > > > this is a major problem, you are in a specialized situation anyway,
> > > > > so you can manually adjust the NR_CPUS, RCU_FANOUT, and RCU_FANOUT_LEAF
> > > > > kernel config parameters.
> > > > > 
> > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > ---
> > > > >  kernel/rcutree.c        |    2 +-
> > > > >  kernel/rcutree_plugin.h |    2 ++
> > > > >  2 files changed, 3 insertions(+), 1 deletions(-)
> > > > > 
> > > > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > > > > index a151184..9098910 100644
> > > > > --- a/kernel/rcutree.c
> > > > > +++ b/kernel/rcutree.c
> > > > > @@ -2672,7 +2672,7 @@ static void __init rcu_init_geometry(void)
> > > > >  {
> > > > >  	int i;
> > > > >  	int j;
> > > > > -	int n = NR_CPUS;
> > > > > +	int n = nr_cpu_ids;
> > > > 
> > > > Same question as before: why have this as a variable when it never
> > > > changes?
> > > 
> > > Ah, that explains why.  This prevented me from forgetting the random
> > > NR_CPUS.
> > 
> > Does that mean it can go away now that you've written the patches?
> 
> If I don't have to change from nr_cpu_ids to yet another thing over
> the next while, then it might be worth changing.

That sounds like an argument for a #define or a static const, rather
than a local variable. :)

- Josh Triplett
Paul E. McKenney June 16, 2012, 2:44 p.m. UTC | #6 
On Sat, Jun 16, 2012 at 02:17:33AM -0700, Josh Triplett wrote:
> On Fri, Jun 15, 2012 at 11:38:48PM -0700, Paul E. McKenney wrote:
> > On Fri, Jun 15, 2012 at 10:17:12PM -0700, Josh Triplett wrote:
> > > On Fri, Jun 15, 2012 at 05:37:14PM -0700, Paul E. McKenney wrote:
> > > > On Fri, Jun 15, 2012 at 02:47:26PM -0700, Josh Triplett wrote:
> > > > > On Fri, Jun 15, 2012 at 02:05:57PM -0700, Paul E. McKenney wrote:
> > > > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > > > > 
> > > > > > The rcu_node tree array is sized based on compile-time constants,
> > > > > > including NR_CPUS.  Although this approach has worked well in the past,
> > > > > > the recent trend by many distros to define NR_CPUS=4096 results in
> > > > > > excessive grace-period-initialization latencies.
> > > > > > 
> > > > > > This commit therefore substitutes the run-time computed nr_cpu_ids for
> > > > > > the compile-time NR_CPUS when building the tree.  This can result in
> > > > > > much of the compile-time-allocated rcu_node array being unused.  If
> > > > > > this is a major problem, you are in a specialized situation anyway,
> > > > > > so you can manually adjust the NR_CPUS, RCU_FANOUT, and RCU_FANOUT_LEAF
> > > > > > kernel config parameters.
> > > > > > 
> > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > > ---
> > > > > >  kernel/rcutree.c        |    2 +-
> > > > > >  kernel/rcutree_plugin.h |    2 ++
> > > > > >  2 files changed, 3 insertions(+), 1 deletions(-)
> > > > > > 
> > > > > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > > > > > index a151184..9098910 100644
> > > > > > --- a/kernel/rcutree.c
> > > > > > +++ b/kernel/rcutree.c
> > > > > > @@ -2672,7 +2672,7 @@ static void __init rcu_init_geometry(void)
> > > > > >  {
> > > > > >  	int i;
> > > > > >  	int j;
> > > > > > -	int n = NR_CPUS;
> > > > > > +	int n = nr_cpu_ids;
> > > > > 
> > > > > Same question as before: why have this as a variable when it never
> > > > > changes?
> > > > 
> > > > Ah, that explains why.  This prevented me from forgetting the random
> > > > NR_CPUS.
> > > 
> > > Does that mean it can go away now that you've written the patches?
> > 
> > If I don't have to change from nr_cpu_ids to yet another thing over
> > the next while, then it might be worth changing.
> 
> That sounds like an argument for a #define or a static const, rather
> than a local variable. :)

OK, static const it is!

							Thanx, Paul
Paul E. McKenney June 16, 2012, 2:51 p.m. UTC | #7 
On Sat, Jun 16, 2012 at 07:44:53AM -0700, Paul E. McKenney wrote:
> On Sat, Jun 16, 2012 at 02:17:33AM -0700, Josh Triplett wrote:
> > On Fri, Jun 15, 2012 at 11:38:48PM -0700, Paul E. McKenney wrote:
> > > On Fri, Jun 15, 2012 at 10:17:12PM -0700, Josh Triplett wrote:
> > > > On Fri, Jun 15, 2012 at 05:37:14PM -0700, Paul E. McKenney wrote:
> > > > > On Fri, Jun 15, 2012 at 02:47:26PM -0700, Josh Triplett wrote:
> > > > > > On Fri, Jun 15, 2012 at 02:05:57PM -0700, Paul E. McKenney wrote:
> > > > > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > > > > > 
> > > > > > > The rcu_node tree array is sized based on compile-time constants,
> > > > > > > including NR_CPUS.  Although this approach has worked well in the past,
> > > > > > > the recent trend by many distros to define NR_CPUS=4096 results in
> > > > > > > excessive grace-period-initialization latencies.
> > > > > > > 
> > > > > > > This commit therefore substitutes the run-time computed nr_cpu_ids for
> > > > > > > the compile-time NR_CPUS when building the tree.  This can result in
> > > > > > > much of the compile-time-allocated rcu_node array being unused.  If
> > > > > > > this is a major problem, you are in a specialized situation anyway,
> > > > > > > so you can manually adjust the NR_CPUS, RCU_FANOUT, and RCU_FANOUT_LEAF
> > > > > > > kernel config parameters.
> > > > > > > 
> > > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > > > ---
> > > > > > >  kernel/rcutree.c        |    2 +-
> > > > > > >  kernel/rcutree_plugin.h |    2 ++
> > > > > > >  2 files changed, 3 insertions(+), 1 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > > > > > > index a151184..9098910 100644
> > > > > > > --- a/kernel/rcutree.c
> > > > > > > +++ b/kernel/rcutree.c
> > > > > > > @@ -2672,7 +2672,7 @@ static void __init rcu_init_geometry(void)
> > > > > > >  {
> > > > > > >  	int i;
> > > > > > >  	int j;
> > > > > > > -	int n = NR_CPUS;
> > > > > > > +	int n = nr_cpu_ids;
> > > > > > 
> > > > > > Same question as before: why have this as a variable when it never
> > > > > > changes?
> > > > > 
> > > > > Ah, that explains why.  This prevented me from forgetting the random
> > > > > NR_CPUS.
> > > > 
> > > > Does that mean it can go away now that you've written the patches?
> > > 
> > > If I don't have to change from nr_cpu_ids to yet another thing over
> > > the next while, then it might be worth changing.
> > 
> > That sounds like an argument for a #define or a static const, rather
> > than a local variable. :)
> 
> OK, static const it is!

Except that the compiler doesn't like the run-time initialization of
a static const variable.

Can't have everything, I guess.  ;-)

							Thanx, Paul
Josh Triplett June 16, 2012, 8:31 p.m. UTC | #8 
On Sat, Jun 16, 2012 at 07:51:54AM -0700, Paul E. McKenney wrote:
> On Sat, Jun 16, 2012 at 07:44:53AM -0700, Paul E. McKenney wrote:
> > On Sat, Jun 16, 2012 at 02:17:33AM -0700, Josh Triplett wrote:
> > > On Fri, Jun 15, 2012 at 11:38:48PM -0700, Paul E. McKenney wrote:
> > > > On Fri, Jun 15, 2012 at 10:17:12PM -0700, Josh Triplett wrote:
> > > > > On Fri, Jun 15, 2012 at 05:37:14PM -0700, Paul E. McKenney wrote:
> > > > > > On Fri, Jun 15, 2012 at 02:47:26PM -0700, Josh Triplett wrote:
> > > > > > > On Fri, Jun 15, 2012 at 02:05:57PM -0700, Paul E. McKenney wrote:
> > > > > > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > > > > > > 
> > > > > > > > The rcu_node tree array is sized based on compile-time constants,
> > > > > > > > including NR_CPUS.  Although this approach has worked well in the past,
> > > > > > > > the recent trend by many distros to define NR_CPUS=4096 results in
> > > > > > > > excessive grace-period-initialization latencies.
> > > > > > > > 
> > > > > > > > This commit therefore substitutes the run-time computed nr_cpu_ids for
> > > > > > > > the compile-time NR_CPUS when building the tree.  This can result in
> > > > > > > > much of the compile-time-allocated rcu_node array being unused.  If
> > > > > > > > this is a major problem, you are in a specialized situation anyway,
> > > > > > > > so you can manually adjust the NR_CPUS, RCU_FANOUT, and RCU_FANOUT_LEAF
> > > > > > > > kernel config parameters.
> > > > > > > > 
> > > > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > > > > ---
> > > > > > > >  kernel/rcutree.c        |    2 +-
> > > > > > > >  kernel/rcutree_plugin.h |    2 ++
> > > > > > > >  2 files changed, 3 insertions(+), 1 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > > > > > > > index a151184..9098910 100644
> > > > > > > > --- a/kernel/rcutree.c
> > > > > > > > +++ b/kernel/rcutree.c
> > > > > > > > @@ -2672,7 +2672,7 @@ static void __init rcu_init_geometry(void)
> > > > > > > >  {
> > > > > > > >  	int i;
> > > > > > > >  	int j;
> > > > > > > > -	int n = NR_CPUS;
> > > > > > > > +	int n = nr_cpu_ids;
> > > > > > > 
> > > > > > > Same question as before: why have this as a variable when it never
> > > > > > > changes?
> > > > > > 
> > > > > > Ah, that explains why.  This prevented me from forgetting the random
> > > > > > NR_CPUS.
> > > > > 
> > > > > Does that mean it can go away now that you've written the patches?
> > > > 
> > > > If I don't have to change from nr_cpu_ids to yet another thing over
> > > > the next while, then it might be worth changing.
> > > 
> > > That sounds like an argument for a #define or a static const, rather
> > > than a local variable. :)
> > 
> > OK, static const it is!
> 
> Except that the compiler doesn't like the run-time initialization of
> a static const variable.
> 
> Can't have everything, I guess.  ;-)

Ah, right, nr_cpu_ids is not a static const.

In that case, just waiting until you think this definition won't churn
anymore and substituting nr_cpu_ids for n everywhere seems like the
right solution.

- Josh Triplett
diff mbox

Patch

diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index a151184..9098910 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -2672,7 +2672,7 @@  static void __init rcu_init_geometry(void)
 {
 	int i;
 	int j;
-	int n = NR_CPUS;
+	int n = nr_cpu_ids;
 	int rcu_capacity[MAX_RCU_LVLS + 1];
 
 	/* If the compile-time values are accurate, just leave. */
diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
index e9b44c3..7cb86ae 100644
--- a/kernel/rcutree_plugin.h
+++ b/kernel/rcutree_plugin.h
@@ -72,6 +72,8 @@  static void __init rcu_bootup_announce_oddness(void)
 #endif
 	if (rcu_fanout_leaf != CONFIG_RCU_FANOUT_LEAF)
 		printk(KERN_INFO "\tExperimental boot-time adjustment of leaf fanout.\n");
+	if (nr_cpu_ids != NR_CPUS)
+		printk(KERN_INFO "\tRCU restricting CPUs from NR_CPUS=%d to nr_cpu_ids=%d.\n", NR_CPUS, nr_cpu_ids);
 }
 
 #ifdef CONFIG_TREE_PREEMPT_RCU