Re: [patch v2a 01/20] posix-timers: Prevent RT livelock in itimer_delete()

Frederic Weisbecker
2023-06-05T11:00:10Z

Le Thu, Jun 01, 2023 at 10:16:34PM +0200, Thomas Gleixner a écrit :
> itimer_delete() has a retry loop when the timer is concurrently expired. On
> non-RT kernels this just spin-waits until the timer callback has completed.
> On RT kernels this is a potential livelock when the exiting task preempted
> the hrtimer soft interrupt.

It's not just RT but also archs supporting HAVE_POSIX_CPU_TIMERS_TASK_WORK

>
> Replace spin_unlock() with an invocation of timer_wait_running() to handle
> it the same way as the other retry loops in the posix timer code.
>
> Fixes: ec8f954a40da ("posix-timers: Use a callback for cancel synchronization on PREEMPT_RT")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> V2: Remove bogus comments vs. posix CPU timers - Frederic
> V2a: Send the real fixed up version
> ---
> kernel/time/posix-timers.c | 43 +++++++++++++++++++++++++++++++++++--------
> 1 file changed, 35 insertions(+), 8 deletions(-)
>
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -1037,27 +1037,52 @@ SYSCALL_DEFINE1(timer_delete, timer_t, t
> }
>
> /*
> - * return timer owned by the process, used by exit_itimers
> + * Delete a timer if it is armed, remove it from the hash and schedule it
> + * for RCU freeing.
> */
> static void itimer_delete(struct k_itimer *timer)
> {
> -retry_delete:
> - spin_lock_irq(&timer->it_lock);
> + unsigned long flags;
>
> +retry_delete:
> + /*
> + * irqsave is required to make timer_wait_running() work.
> + */
> + spin_lock_irqsave(&timer->it_lock, flags);
> +
> + /*
> + * Even if the timer is not longer accessible from other tasks
> + * it still might be armed and queued in the underlying timer
> + * mechanism. Worse, that timer mechanism might run the expiry
> + * function concurrently.
> + */
> if (timer_delete_hook(timer) == TIMER_RETRY) {
> - spin_unlock_irq(&timer->it_lock);
> + /*
> + * Timer is expired concurrently, prevent livelocks
> + * and pointless spinning on RT.

Ditto.

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Thanks.