Groups > linux.kernel > #1335124 > unrolled thread

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

Started by	Ingo Molnar <mingo@kernel.org>
First post	2016-02-16 09:10 +0100
Last post	2016-02-16 14:40 +0100
Articles	2 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

  Re: [PATCH 3/9] nohz: New tick dependency mask Ingo Molnar <mingo@kernel.org> - 2016-02-16 09:10 +0100
    Re: [PATCH 3/9] nohz: New tick dependency mask Frederic Weisbecker <fweisbec@gmail.com> - 2016-02-16 14:40 +0100

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

From	Ingo Molnar <mingo@kernel.org>
Date	2016-02-16 09:10 +0100
Subject	Re: [PATCH 3/9] nohz: New tick dependency mask
Message-ID	<r2IRQ-5aD-7@gated-at.bofh.it>

* 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

[toc] | [next] | [standalone]

#1335384

From	Frederic Weisbecker <fweisbec@gmail.com>
Date	2016-02-16 14:40 +0100
Message-ID	<r2O1c-8t4-15@gated-at.bofh.it>
In reply to	#1335124

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

[toc] | [prev] | [standalone]

csiph-web

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

Contents

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

#1335384