Messages in this thread | | | Date | Tue, 16 Feb 2016 14:38:49 +0100 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 3/9] nohz: New tick dependency mask |
| |
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
| |