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

[Ingo Molnar <mingo@kernel.org>]

* 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