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

[Frederic Weisbecker <fweisbec@gmail.com>]

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.

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

Thanks.