lkml.org 
[lkml]   [2012]   [Jun]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH tip/core/rcu 01/15] rcu: Control RCU_FANOUT_LEAF from boot-time parameter
On Fri, Jun 15, 2012 at 02:43:09PM -0700, Josh Triplett wrote:
> On Fri, Jun 15, 2012 at 02:05:56PM -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> >
> > Although making RCU_FANOUT_LEAF a kernel configuration parameter rather
> > than a fixed constant makes it easier for people to decrease cache-miss
> > overhead for large systems, it is of little help for people who must
> > run a single pre-built kernel binary.
> >
> > This commit therefore allows the value of RCU_FANOUT_LEAF to be
> > increased (but not decreased!) via a boot-time parameter named
> > rcutree.rcu_fanout_leaf.
> >
> > Reported-by: Mike Galbraith <efault@gmx.de>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> > Documentation/kernel-parameters.txt | 4 ++
> > kernel/rcutree.c | 97 ++++++++++++++++++++++++++++++-----
> > kernel/rcutree.h | 23 +++++----
> > kernel/rcutree_plugin.h | 4 +-
> > kernel/rcutree_trace.c | 2 +-
> > 5 files changed, 104 insertions(+), 26 deletions(-)
> >
> > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > index c45513d..88bd3ef 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -2367,6 +2367,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> > Set maximum number of finished RCU callbacks to process
> > in one batch.
> >
> > + rcutree.fanout_leaf= [KNL,BOOT]
> > + Set maximum number of finished RCU callbacks to process
> > + in one batch.
>
> Copy-paste problem.

Indeed! Good catch!

> > rcutree.qhimark= [KNL,BOOT]
> > Set threshold of queued
> > RCU callbacks over which batch limiting is disabled.
> > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > index 0da7b88..a151184 100644
> > --- a/kernel/rcutree.c
> > +++ b/kernel/rcutree.c
> > @@ -60,17 +60,10 @@
> >
> > /* Data structures. */
> >
> > -static struct lock_class_key rcu_node_class[NUM_RCU_LVLS];
> > +static struct lock_class_key rcu_node_class[RCU_NUM_LVLS];
>
> I assume that the requirement to only increase the fanout and never
> decrease it comes from the desire to not increase the sizes of all of
> these arrays to MAX_RCU_LVLS?

Actually, it is the node[] array in the rcu_state structure that is
of concern.

> > +/*
> > + * Compute the rcu_node tree geometry from kernel parameters. This cannot
> > + * replace the definitions in rcutree.h because those are needed to size
> > + * the ->node array in the rcu_state structure.
> > + */
> > +static void __init rcu_init_geometry(void)
> > +{
> > + int i;
> > + int j;
> > + int n = NR_CPUS;
> > + int rcu_capacity[MAX_RCU_LVLS + 1];
> > +
> > + /* If the compile-time values are accurate, just leave. */
> > + if (rcu_fanout_leaf == CONFIG_RCU_FANOUT_LEAF)
> > + return;
> > +
> > + /*
> > + * Compute number of nodes that can be handled an rcu_node tree
> > + * with the given number of levels. Setting rcu_capacity[0] makes
> > + * some of the arithmetic easier.
> > + */
> > + rcu_capacity[0] = 1;
> > + rcu_capacity[1] = rcu_fanout_leaf;
> > + for (i = 2; i <= MAX_RCU_LVLS; i++)
> > + rcu_capacity[i] = rcu_capacity[i - 1] * CONFIG_RCU_FANOUT;
> > +
> > + /*
> > + * The boot-time rcu_fanout_leaf parameter is only permitted
> > + * to increase the leaf-level fanout, not decrease it. Of course,
> > + * the leaf-level fanout cannot exceed the number of bits in
> > + * the rcu_node masks. Finally, the tree must be able to accommodate
> > + * the configured number of CPUs. Complain and fall back to the
> > + * compile-timer values if these limits are exceeded.
>
> Typo: s/timer/time/

Good catch!

> > + */
> > + if (rcu_fanout_leaf < CONFIG_RCU_FANOUT_LEAF ||
> > + rcu_fanout_leaf > sizeof(unsigned long) * 8 ||
> > + n > rcu_capacity[4]) {
>
> 4 seems like a magic number here; did you mean MAX_RCU_LVLS or similar?

I believe so, good catch! That would have been painful if another
level of the hierarchy were needed...

> Also, why have n as a variable when it never changes?

Will propagate the value unless I can come up with a good reason. ;-)

> > --- a/kernel/rcutree.h
> > +++ b/kernel/rcutree.h
> > @@ -42,28 +42,28 @@
> > #define RCU_FANOUT_4 (RCU_FANOUT_3 * CONFIG_RCU_FANOUT)
> >
> > #if NR_CPUS <= RCU_FANOUT_1
> > -# define NUM_RCU_LVLS 1
> > +# define RCU_NUM_LVLS 1
>
> I assume you made this change to make it easier to track down all the
> uses of the macro to change them; however, having now done so, the
> change itself seems rather gratuitous, and inconsistent with the other
> macros. Would you consider changing it back?
>
> > +extern int rcu_num_lvls;
> > +extern int rcu_num_nodes;
>
> Given the above, you might also want to change these for consistency.

This might make sense. If I run into too many conflicts, I may defer
the change to the join of the topic trees.

> Also, have you checked the various loops using these variables to figure
> out if GCC emits less optimal code now that it can't rely on a
> compile-time constant? I don't expect it to make much of a difference,
> but it seems worth checking.

I am sure that it generates worse code, but the uses are on slowpaths,
so I really am not worried about it.

> You might also consider marking these as __read_mostly, at a minimum.

This sounds quite sensible, will do.

> > diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> > index 2411000..e9b44c3 100644
> > --- a/kernel/rcutree_plugin.h
> > +++ b/kernel/rcutree_plugin.h
> > @@ -68,8 +68,10 @@ static void __init rcu_bootup_announce_oddness(void)
> > printk(KERN_INFO "\tAdditional per-CPU info printed with stalls.\n");
> > #endif
> > #if NUM_RCU_LVL_4 != 0
> > - printk(KERN_INFO "\tExperimental four-level hierarchy is enabled.\n");
> > + printk(KERN_INFO "\tFour-level hierarchy is enabled.\n");
>
> This change seems entirely unrelated to this patch. Seems simple enough
> to split it into a separate one-line patch ("Mark four-level hierarchy
> as no longer experimental").

Can't see why not...

> > #endif
> > + if (rcu_fanout_leaf != CONFIG_RCU_FANOUT_LEAF)
> > + printk(KERN_INFO "\tExperimental boot-time adjustment of leaf fanout.\n");
>
> You might consider printing rcu_fanout_leaf in this message.

Ouch! Good point, will fix.

Thanx, Paul



\
 
 \ /
  Last update: 2012-06-16 00:41    [from the cache]
©2003-2011 Jasper Spaans