Re: [PATCH 07/18] nohz: Selectively enable context tracking on fulldynticks CPUs

[Steven Rostedt]

On Fri, 2013-07-19 at 00:13 +0200, Frederic Weisbecker wrote:
> On Wed, Jul 17, 2013 at 02:27:17PM -0400, Steven Rostedt wrote:
> > On Wed, 2013-07-17 at 18:44 +0200, Frederic Weisbecker wrote:
> > > The code is ready to do so in the context tracking subsystem, now
> > 
> > "do so"? Do what?
> 
> It's referring to the patch title. The code is ready to selectively
> enable context tracking on the CPUs.
> 
> I see many changelogs that use that kind of style where the title
> of the patch is considered as the 1st line of the changelog. That's
> convenient because it avoids the need to rephrase the title in the
> changelog.
> 
> But may be the reference to the title is not obvious. if you prefer
> I can expand the "do so" here.

Yeah, I've seen that too. But this was a bit too subtle to get it. The
subject is a bit vague as well, which doesn't help the matter. What does
"selectively enable context tracking" mean? How is it selective?

> 
> > 
> > > we just need to pass our cpu range selection to it from the
> > 
> > Pass cpu range selection to what?
> > 
> > Pronouns are evil in technical documentation.
> 
> How about:
> 
> """
> The code in the context tracking subsystem is ready to selectively
> enable its tracking on specificied CPU ranges instead of inconditionally

"specified" "unconditionally"

> force it on all CPUs.
> 
> What we need to do now is to pass the desired CPU ranges to track from
> the full dynticks subsystem, according to the ranges specified in the
> "nohz_full=" boot option.
> """ 
> 
> > > diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
> > > index 12045ce..2c2b73aa 100644
> > > --- a/include/linux/context_tracking.h
> > > +++ b/include/linux/context_tracking.h
> > > @@ -34,6 +34,8 @@ static inline bool context_tracking_active(void)
> > >  	return __this_cpu_read(context_tracking.active);
> > >  }
> > >  
> > > +extern void context_tracking_cpu_set(int cpu);
> > > +
> > >  extern void user_enter(void);
> > >  extern void user_exit(void);
> > >  
> > > diff --git a/init/Kconfig b/init/Kconfig
> > > index 247084b..914da3f 100644
> > > --- a/init/Kconfig
> > > +++ b/init/Kconfig
> > > @@ -527,7 +527,7 @@ config RCU_USER_QS
> > >  config CONTEXT_TRACKING_FORCE
> > >  	bool "Force context tracking"
> > >  	depends on CONTEXT_TRACKING
> > > -	default CONTEXT_TRACKING
> > > +	default y if !NO_HZ_FULL
> > 
> > Why the if !NO_HZ_FULL?
> > 
> > That selects this anyway. Oh wait, you changed this.
> 
> Yeah that's probably confusing. Ok lets consider a system with:
> 
>      CONTEXT_TRACKING=y
> 
> By default it doesn't track any CPU, it's inactive unless you set:
> 
>      CONTEXT_TRACKING_FORCE=y
> 
> In this case, all CPUs are tracked.
> 
> The full dynticks subsystem was supposed to pass its CPU range to context
> tracking such that it activates the tracking only on the relevant CPUs.
> 
> But the context tracking code was merged before full dynticks. So nothing
> was there to enabled CPUs on context tracking initially. So we needed
> CONTEXT_TRACKING_FORCE for testing.
> 
> Then later we merged full dynticks. But we got lazy and rushed and instead of
> selecting the CPUs to track on runtime from the full dynticks subsystem to
> the context tracking subsystem, we forced CONTEXT_TRACKING_FORCE=y when
> NO_HZ_FULL=y. Then using runtime selection became a TODO.
> 
> Now these patches handle that TODO and full dynticks passes its range to
> contex tracking.
> 
> Now one could argue why we keep CONTEXT_TRACKING_FORCE around, since we
> have full dynticks and NO_HZ_FULL_ALL for wide automated testing.
> 
> This is because CONTEXT_TRACKING is not sufficient for NO_HZ_FULL alone.
> Especially because of the 64bits requirement that I need to drop after
> careful review of any use of cputime_t. But anyway, CONTEXT_TRACKING_FORCE
> is still handy to keep around for archs that want support for nohz full
> but don't yet meet all dependencies.

OK, that needs a comment in the Kconfig. Perhaps something like:

"CONTEXT_TRACKING is only needed by NO_HZ_FULL, but the user may want to
test CONTEXT_TRACKING on systems that do not yet support NO_HZ_FULL, in
which case we must keep the FORCE to enable it."

Or something to that nature. That way people have a clue to why that's
like that.

-- Steve


--
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/