Re: [PATCH v3 2/2] sched: Add a check for cpu unbound deferrable timers

From: Thomas Gleixner
Date: Wed May 06 2020 - 10:03:42 EST


Prasad Sodagudi <psodagud@xxxxxxxxxxxxxx> writes:

> Subject: sched: Add...

How is this related to the scheduler?

> Add a check to find expired unbound deferrable timers
> and trigger softirq for handling timers. This way a CPU
> can process all the expired deferrable timers whenever
> it is out off idle state due to an interrupt.

A little bit more context would be useful.

> Signed-off-by: Prasad Sodagudi <psodagud@xxxxxxxxxxxxxx>
> ---
> include/linux/timer.h | 3 +++
> kernel/time/tick-sched.c | 8 +++++++-
> kernel/time/timer.c | 29 ++++++++++++++++++++++++++++-
> 3 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/timer.h b/include/linux/timer.h
> index 0dc19a8..e85dd2d 100644
> --- a/include/linux/timer.h
> +++ b/include/linux/timer.h
> @@ -172,6 +172,9 @@ extern int del_timer(struct timer_list * timer);
> extern int mod_timer(struct timer_list *timer, unsigned long expires);
> extern int mod_timer_pending(struct timer_list *timer, unsigned long expires);
> extern int timer_reduce(struct timer_list *timer, unsigned long expires);
> +#ifdef CONFIG_SMP

This #ifdef is useless because ...

> +extern bool check_pending_deferrable_timers(int cpu);
> +#endif
>
> /*
> * The jiffies value which is added to now, when there is no timer
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 3e2dc9b..16aec80 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -23,6 +23,7 @@
> #include <linux/module.h>
> #include <linux/irq_work.h>
> #include <linux/posix-timers.h>
> +#include <linux/timer.h>
> #include <linux/context_tracking.h>
> #include <linux/mm.h>
>
> @@ -1274,8 +1275,13 @@ static inline void tick_nohz_irq_enter(void)
> now = ktime_get();
> if (ts->idle_active)
> tick_nohz_stop_idle(ts, now);
> - if (ts->tick_stopped)
> + if (ts->tick_stopped) {
> tick_nohz_update_jiffies(now);
> +#ifdef CONFIG_SMP
> + if (check_pending_deferrable_timers(smp_processor_id()))
> + raise_softirq_irqoff(TIMER_SOFTIRQ);
> +#endif

... you failed to provide a stub function which avoids this #ifdef

> + }
> }
>
> #else
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 1bf9b49..5947c63 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -221,6 +221,7 @@ static DECLARE_WORK(timer_update_work, timer_update_keys);
>
> #ifdef CONFIG_SMP
> struct timer_base timer_base_deferrable;
> +static atomic_t deferrable_pending;
> unsigned int sysctl_timer_migration = 1;
>
> DEFINE_STATIC_KEY_FALSE(timers_migration_enabled);
> @@ -1610,6 +1611,31 @@ static u64 cmp_next_hrtimer_event(u64 basem, u64 expires)
> return DIV_ROUND_UP_ULL(nextevt, TICK_NSEC) * TICK_NSEC;
> }
>
> +
> +#ifdef CONFIG_SMP
> +/*
> + * check_pending_deferrable_timers - Check for unbound deferrable timer expiry
> + * @cpu - Current CPU

Bogus doc format.

> + *
> + * The function checks whether any global deferrable pending timers
> + * are exipired or not. This function does not check cpu bounded
> + * diferrable pending timers expiry.

Editors have spell check.

> + *
> + * The function returns true when a cpu unbounded deferrable timer is expired.
> + */
> +bool check_pending_deferrable_timers(int cpu)
> +{
> + if (cpu == tick_do_timer_cpu ||
> + tick_do_timer_cpu == TICK_DO_TIMER_NONE) {

The second line conditional wants to be aligned with the first line
conditional.

> + if (time_after_eq(jiffies, timer_base_deferrable.clk)
> + && !atomic_cmpxchg(&deferrable_pending, 0, 1)) {

Ditto. Aside of that this deferrable pending magic lacks any form of
explanation.

> + return true;
> + }
> + }
> + return false;
> +}
> +#endif
> +
> /**
> * get_next_timer_interrupt - return the time (clock mono) of the next timer
> * @basej: base time jiffies
> @@ -1801,7 +1827,8 @@ static __latent_entropy void run_timer_softirq(struct softirq_action *h)
> if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) {
> __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF]));
> #ifdef CONFIG_SMP
> - if (tick_do_timer_cpu == TICK_DO_TIMER_NONE ||
> + if ((atomic_cmpxchg(&deferrable_pending, 1, 0) &&

How is that supposed to compile with NOHZ=n?

For every version of these patches the 0-day robot is complaining about
exactly the same problem:

Your testing is solely done with one config, i.e. the config you are
interested in.

Is it really so hard to compile test for a total of 4 combinations of
SMP and NOHZ?

Thanks,

tglx