Message-ID: <20130719141920.GA21752@somewhere>
Date:	Fri, 19 Jul 2013 16:19:22 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Ingo Molnar <mingo@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Borislav Petkov <bp@...en8.de>,
	Li Zhong <zhong@...ux.vnet.ibm.com>,
	Mike Galbraith <efault@....de>,
	Kevin Hilman <khilman@...aro.org>
Subject: Re: [PATCH 07/18] nohz: Selectively enable context tracking on full
 dynticks CPUs

On Thu, Jul 18, 2013 at 06:51:57PM -0400, Steven Rostedt wrote:
> 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?

Yeah selectively means here that it's enabled only on some CPUs, not
all of them.

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

Oops :)

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

Ok, I'll add that.

Thanks!

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

Powered by blists - more mailing lists