Re: [PATCH 3/9] nohz: New tick dependency mask

[Frederic Weisbecker <fweisbec@gmail.com>]

On Tue, Feb 16, 2016 at 09:03:45AM +0100, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > The tick dependency is evaluated on every IRQ and context switch. This
> > consists is a batch of checks which determine whether it is safe to
> > stop the tick or not. These checks are often split in many details:
> > posix cpu timers, scheduler, sched clock, perf events.... each of which
> > are made of smaller details: posix cpu timer involves checking process
> > wide timers then thread wide timers. Perf involves checking freq events
> > then more per cpu details.
> > 
> > Checking these informations asynchronously every time we update the full
> > dynticks state bring avoidable overhead and a messy layout.
> > 
> > Let's introduce instead tick dependency masks: one for system wide
> > dependency (unstable sched clock, freq based perf events), one for CPU
> > wide dependency (sched, throttling perf events), and task/signal level
> > dependencies (posix cpu timers). The subsystems are responsible
> > for setting and clearing their dependency through a set of APIs that will
> > take care of concurrent dependency mask modifications and kick targets
> > to restart the relevant CPU tick whenever needed.
> > 
> > This new dependency engine stays beside the old one until all subsystems
> > having a tick dependency are converted to it.
> > 
> > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > Suggested-by: Peter Zijlstra <peterz@infradead.org>
> > Reviewed-by: Chris Metcalf <cmetcalf@ezchip.com>
> > Cc: Christoph Lameter <cl@linux.com>
> > Cc: Chris Metcalf <cmetcalf@ezchip.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Luiz Capitulino <lcapitulino@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Rik van Riel <riel@redhat.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > ---
> >  include/linux/sched.h    |   8 +++
> >  include/linux/tick.h     |  92 +++++++++++++++++++++++++++++
> >  kernel/time/tick-sched.c | 150 ++++++++++++++++++++++++++++++++++++++++++++---
> >  kernel/time/tick-sched.h |   1 +
> >  4 files changed, 244 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index a10494a..d482cc8 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -719,6 +719,10 @@ struct signal_struct {
> >  	/* Earliest-expiration cache. */
> >  	struct task_cputime cputime_expires;
> >  
> > +#ifdef CONFIG_NO_HZ_FULL
> > +	unsigned long tick_dependency;
> > +#endif
> > +
> >  	struct list_head cpu_timers[3];
> >  
> >  	struct pid *tty_old_pgrp;
> > @@ -1542,6 +1546,10 @@ struct task_struct {
> >  		VTIME_SYS,
> >  	} vtime_snap_whence;
> >  #endif
> > +
> > +#ifdef CONFIG_NO_HZ_FULL
> > +	unsigned long tick_dependency;
> 
> 
> So I think it would be useful to name this in a way the expresses that this is a 
> mask.
> 
> 'tick_dep_mask' or so?
> 
> > +#endif
> >  	unsigned long nvcsw, nivcsw; /* context switch counts */
> >  	u64 start_time;		/* monotonic time in nsec */
> >  	u64 real_start_time;	/* boot based time in nsec */
> > diff --git a/include/linux/tick.h b/include/linux/tick.h
> > index 97fd4e5..a33adab 100644
> > --- a/include/linux/tick.h
> > +++ b/include/linux/tick.h
> > @@ -97,6 +97,18 @@ static inline void tick_broadcast_exit(void)
> >  	tick_broadcast_oneshot_control(TICK_BROADCAST_EXIT);
> >  }
> >  
> > +enum tick_dependency_bit {
> 
> s/tick_dep_bits
> 
> > +	TICK_POSIX_TIMER_BIT	= 0,
> > +	TICK_PERF_EVENTS_BIT	= 1,
> > +	TICK_SCHED_BIT		= 2,
> > +	TICK_CLOCK_UNSTABLE_BIT	= 3
> 
> s/TICK_DEP_BIT_...
> 
> > +};
> > +
> > +#define TICK_POSIX_TIMER_MASK		(1 << TICK_POSIX_TIMER_BIT)
> > +#define TICK_PERF_EVENTS_MASK		(1 << TICK_PERF_EVENTS_BIT)
> > +#define TICK_SCHED_MASK			(1 << TICK_SCHED_BIT)
> > +#define TICK_CLOCK_UNSTABLE_MASK	(1 << TICK_CLOCK_UNSTABLE_BIT)
> 
> So I'd rename this to:
> 
> #define TICK_DEP_MASK_POSIX_TIMER	(1 << TICK_POSIX_TIMER_BIT)
> #define TICK_DEP_MASK_PERF_EVENTS	(1 << TICK_PERF_EVENTS_BIT)
> #define TICK_DEP_MASK_SCHED		(1 << TICK_SCHED_BIT)
> #define TICK_DEP_MASK_CLOCK_UNSTABLE	(1 << TICK_CLOCK_UNSTABLE_BIT)
> 
> i.e. the 'tick_dep' and 'TICK_DEP' nomenclature would be used throughout the code 
> and the pattern would be easy to grep for.

I agree with all the above, I'll change that.

> 
> > +extern void tick_nohz_set_dep(enum tick_dependency_bit bit);
> > +extern void tick_nohz_clear_dep(enum tick_dependency_bit bit);
> > +extern void tick_nohz_set_dep_cpu(int cpu, enum tick_dependency_bit bit);
> > +extern void tick_nohz_clear_dep_cpu(int cpu, enum tick_dependency_bit bit);
> > +extern void tick_nohz_set_dep_task(struct task_struct *tsk,
> > +				   enum tick_dependency_bit bit);
> > +extern void tick_nohz_clear_dep_task(struct task_struct *tsk,
> > +				     enum tick_dependency_bit bit);
> > +extern void tick_nohz_set_dep_signal(struct signal_struct *signal,
> > +				     enum tick_dependency_bit bit);
> > +extern void tick_nohz_clear_dep_signal(struct signal_struct *signal,
> > +				       enum tick_dependency_bit bit);
> 
> Ditto, please rename it all to:
> 
> 	tick_dep_set()
> 	tick_dep_clear()
> 	tick_dep_set_cpu()
> 	tick_dep_clear_cpu()
> 	tick_dep_set_task()
> 	...

tick_dep_* are already used in the same patch as static key wrappers for the
tick_nohz_dep functions.

> 
> also, please don't line-break function prototypes, it only makes the result harder 
> to read.

Even if it exceeds 80 columns?

Thanks.

> 
> Thanks,
> 
> 	Ingo