Re: [PATCH tip/core/rcu 4/9] nohz_full: Add rcu_dyntick data forscalable detection of all-idle state

[Josh Triplett]

On Sun, Aug 18, 2013 at 06:22:29PM -0700, Paul E. McKenney wrote:
> On Sat, Aug 17, 2013 at 08:02:34PM -0700, Josh Triplett wrote:
> > On Sat, Aug 17, 2013 at 06:49:39PM -0700, Paul E. McKenney wrote:
> > > +#ifdef CONFIG_NO_HZ_FULL_SYSIDLE
> > > +
> > > +/*
> > > + * Initialize dynticks sysidle state for CPUs coming online.
> > > + */
> > > +static void rcu_sysidle_init_percpu_data(struct rcu_dynticks *rdtp)
> > > +{
> > > +	rdtp->dynticks_idle_nesting = DYNTICK_TASK_NEST_VALUE;
> > > +}
> > > +
> > > +#else /* #ifdef CONFIG_NO_HZ_FULL_SYSIDLE */
> > > +
> > > +static void rcu_sysidle_init_percpu_data(struct rcu_dynticks *rdtp)
> > > +{
> > > +}
> > > +
> > > +#endif /* #else #ifdef CONFIG_NO_HZ_FULL_SYSIDLE */
> > 
> > Just move the ifdef around the function body:
> > 
> > static void rcu_sysidle_init_percpu_data(struct rcu_dynticks *rdtp)
> > {
> > #ifdef CONFIG_NO_HZ_FULL_SYSIDLE
> > 	rdtp->dynticks_idle_nesting = DYNTICK_TASK_NEST_VALUE;
> > #endif /* CONFIG_NO_HZ_FULL_SYSIDLE */
> > }
> 
> This makes sense for this isolated function, and it would also
> make sense if the end result had only functions that were exported.
> But if I try to apply this to the result, I will end up with something
> like the following.  Is that really what you want?
> 
> I suppose I could individually enclose whole functions whose definitions
> are unneeded for CONFIG_NO_HZ_FULL_SYSIDLE=n, but that doesn't seem
> helpful either.
> 
> Thoughts?

I see what you mean.  Short of sorting the functions to put all the
unexported ones together, which seems suboptimal, I don't see an obvious
fix.  The result you posted isn't *terrible*, but it's not great either.

I had mostly hoped to avoid having two duplicate function headers that
would then both need changing whenever changing the function signature,
and which could then potentially get out of sync without causing a
compilation error.

I'd say that if you have a single-function block like the one above, you
should use the ifdef-body approach, but if you've got a group of
functions that don't all use one or the other approach, go ahead and
wrap the whole thing in one big ifdef rather than one per function.

Use whichever approach seems most sensible on a case-by-case basis; your
call.  Feel free to add a
Reviewed-by: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
to whichever approach you go with.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/