Messages in this thread | | | Date | Tue, 16 Feb 2016 09:03:45 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH 3/9] nohz: New tick dependency mask |
| |
* 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.
> +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() ...
also, please don't line-break function prototypes, it only makes the result harder to read.
Thanks,
Ingo
| |