Re: [patch V2 02/10] hrtimer: Consolidate reprogramming code

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hi,

On 13.07.2021 15:39, Thomas Gleixner wrote:
> From: Peter Zijlstra <peterz@infradead.org>
>
> This code is mostly duplicated. The redudant store in the force reprogram
> case does no harm and the in hrtimer interrupt condition cannot be true for
> the force reprogram invocations.
>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   kernel/time/hrtimer.c |   72 ++++++++++++++++++++------------------------------
>   1 file changed, 29 insertions(+), 43 deletions(-)
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

This patch landed in today's linux-next (next-20210812) as commit 
b14bca97c9f5 ("hrtimer: Consolidate reprogramming code"). It breaks 
booting of many of my test machines: ARM 32bit Exynos based boards, ARM 
64bit QEmu virt machine or ARM64 Qualcomm DragonBoard410c board.

I've managed to catch the following log on QEmu's virt ARM64 machine:

rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
rcu:     0-...!: (0 ticks this GP) idle=330/0/0x0 softirq=42/42 fqs=0  
(false positive?)
  (detected by 1, t=6502 jiffies, g=-1091, q=115)

============================================
WARNING: possible recursive locking detected
5.14.0-rc5+ #10668 Not tainted
--------------------------------------------
swapper/1/0 is trying to acquire lock:
ffffbb9c1e4ca1d8 (rcu_node_0){-.-.}-{2:2}, at: 
rcu_dump_cpu_stacks+0x68/0x1c4

but task is already holding lock:
ffffbb9c1e4ca1d8 (rcu_node_0){-.-.}-{2:2}, at: 
rcu_sched_clock_irq+0x83c/0x1778

other info that might help us debug this:
  Possible unsafe locking scenario:

        CPU0
        ----
   lock(rcu_node_0);
   lock(rcu_node_0);

  *** DEADLOCK ***

  May be due to missing lock nesting notation

1 lock held by swapper/1/0:
  #0: ffffbb9c1e4ca1d8 (rcu_node_0){-.-.}-{2:2}, at: 
rcu_sched_clock_irq+0x83c/0x1778

stack backtrace:
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.14.0-rc5+ #10668
Hardware name: linux,dummy-virt (DT)
Call trace:
  dump_backtrace+0x0/0x1d0
  show_stack+0x14/0x20
  dump_stack_lvl+0x88/0xb0
  dump_stack+0x14/0x2c
  __lock_acquire+0x17a4/0x1840
  lock_acquire+0x130/0x3e8
  _raw_spin_lock_irqsave+0x78/0x148
  rcu_dump_cpu_stacks+0x68/0x1c4
  rcu_sched_clock_irq+0x11e8/0x1778
  update_process_times+0x88/0xd0
  tick_sched_handle.isra.19+0x30/0x50
  tick_sched_timer+0x48/0x98
  __hrtimer_run_queues+0x380/0x5b0
  hrtimer_interrupt+0xe4/0x240
  arch_timer_handler_virt+0x30/0x40
  handle_percpu_devid_irq+0xc0/0x3d0
  handle_domain_irq+0x58/0x88
  gic_handle_irq+0xa8/0xc8
  call_on_irq_stack+0x28/0x38
  do_interrupt_handler+0x54/0x60
  el1_interrupt+0x2c/0x108
  el1h_64_irq_handler+0x14/0x20
  el1h_64_irq+0x74/0x78
  arch_cpu_idle+0x14/0x20
  default_idle_call+0x88/0x390
  do_idle+0x200/0x290
  cpu_startup_entry+0x20/0x80
  secondary_start_kernel+0x1c0/0x1f0
  __secondary_switched+0x7c/0x80

I hope it helps fixing the issue.

> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -652,21 +652,24 @@ static inline int hrtimer_hres_active(vo
>   	return __hrtimer_hres_active(this_cpu_ptr(&hrtimer_bases));
>   }
>   
> -/*
> - * Reprogram the event source with checking both queues for the
> - * next event
> - * Called with interrupts disabled and base->lock held
> - */
>   static void
> -hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal)
> +__hrtimer_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal,
> +		    struct hrtimer *next_timer, ktime_t expires_next)
>   {
> -	ktime_t expires_next;
> +	/*
> +	 * If the hrtimer interrupt is running, then it will reevaluate the
> +	 * clock bases and reprogram the clock event device.
> +	 */
> +	if (cpu_base->in_hrtirq)
> +		return;
>   
> -	expires_next = hrtimer_update_next_event(cpu_base);
> +	if (expires_next > cpu_base->expires_next)
> +		return;
>   
>   	if (skip_equal && expires_next == cpu_base->expires_next)
>   		return;
>   
> +	cpu_base->next_timer = next_timer;
>   	cpu_base->expires_next = expires_next;
>   
>   	/*
> @@ -689,7 +692,23 @@ hrtimer_force_reprogram(struct hrtimer_c
>   	if (!__hrtimer_hres_active(cpu_base) || cpu_base->hang_detected)
>   		return;
>   
> -	tick_program_event(cpu_base->expires_next, 1);
> +	tick_program_event(expires_next, 1);
> +}
> +
> +/*
> + * Reprogram the event source with checking both queues for the
> + * next event
> + * Called with interrupts disabled and base->lock held
> + */
> +static void
> +hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal)
> +{
> +	ktime_t expires_next;
> +
> +	expires_next = hrtimer_update_next_event(cpu_base);
> +
> +	__hrtimer_reprogram(cpu_base, skip_equal, cpu_base->next_timer,
> +			    expires_next);
>   }
>   
>   /* High resolution timer related functions */
> @@ -835,40 +854,7 @@ static void hrtimer_reprogram(struct hrt
>   	if (base->cpu_base != cpu_base)
>   		return;
>   
> -	/*
> -	 * If the hrtimer interrupt is running, then it will
> -	 * reevaluate the clock bases and reprogram the clock event
> -	 * device. The callbacks are always executed in hard interrupt
> -	 * context so we don't need an extra check for a running
> -	 * callback.
> -	 */
> -	if (cpu_base->in_hrtirq)
> -		return;
> -
> -	if (expires >= cpu_base->expires_next)
> -		return;
> -
> -	/* Update the pointer to the next expiring timer */
> -	cpu_base->next_timer = timer;
> -	cpu_base->expires_next = expires;
> -
> -	/*
> -	 * If hres is not active, hardware does not have to be
> -	 * programmed yet.
> -	 *
> -	 * If a hang was detected in the last timer interrupt then we
> -	 * do not schedule a timer which is earlier than the expiry
> -	 * which we enforced in the hang detection. We want the system
> -	 * to make progress.
> -	 */
> -	if (!__hrtimer_hres_active(cpu_base) || cpu_base->hang_detected)
> -		return;
> -
> -	/*
> -	 * Program the timer hardware. We enforce the expiry for
> -	 * events which are already in the past.
> -	 */
> -	tick_program_event(expires, 1);
> +	__hrtimer_reprogram(cpu_base, true, timer, expires);
>   }
>   
>   /*
>
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland