| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 16 Feb 2016 14:38:49 +0100
From: Frederic Weisbecker <fweisbec@...il.com>
To: Ingo Molnar <mingo@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
Peter Zijlstra <peterz@...radead.org>,
Chris Metcalf <cmetcalf@...hip.com>,
Thomas Gleixner <tglx@...utronix.de>,
Luiz Capitulino <lcapitulino@...hat.com>,
Christoph Lameter <cl@...ux.com>,
Viresh Kumar <viresh.kumar@...aro.org>,
Rik van Riel <riel@...hat.com>
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@...il.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@...utronix.de>
> > Suggested-by: Peter Zijlstra <peterz@...radead.org>
> > Reviewed-by: Chris Metcalf <cmetcalf@...hip.com>
> > Cc: Christoph Lameter <cl@...ux.com>
> > Cc: Chris Metcalf <cmetcalf@...hip.com>
> > Cc: Ingo Molnar <mingo@...nel.org>
> > Cc: Luiz Capitulino <lcapitulino@...hat.com>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Rik van Riel <riel@...hat.com>
> > Cc: Thomas Gleixner <tglx@...utronix.de>
> > Cc: Viresh Kumar <viresh.kumar@...aro.org>
> > Signed-off-by: Frederic Weisbecker <fweisbec@...il.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
Powered by blists - more mailing lists