Re: [patch V2 08/12] rseq: Implement time slice extension enforcement timer

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2025-10-27 17:26:29 [+0100], Thomas Gleixner wrote:
> On Mon, Oct 27 2025 at 12:38, Sebastian Andrzej Siewior wrote:
> > On 2025-10-22 14:57:38 [+0200], Thomas Gleixner wrote:
> >> +static enum hrtimer_restart rseq_slice_expired(struct hrtimer *tmr)
> >> +{
> >> +	struct slice_timer *st = container_of(tmr, struct slice_timer, timer);
> >> +
> >> +	if (st->cookie == current && current->rseq.slice.state.granted) {
> >> +		rseq_stat_inc(rseq_stats.s_expired);
> >> +		set_need_resched_current();
> >> +	}
> >
> > You arm the timer while leaving to userland. Once in userland the task
> > can be migrated to another CPU. Once migrated, this CPU can host another
> > task while the timer fires and does nothing.
> 
> That's inevitable. If the scheduler decides to do that then there is
> nothing which can be done about it and that's why the cookie pointer
> exists.

Without an interrupt on the target CPU, there is nothing stopping the
task from overstepping its fair share.

> >> +	return HRTIMER_NORESTART;
> >> +}
> >> +
> > …
> >> +static void rseq_cancel_slice_extension_timer(void)
> >> +{
> >> +	struct slice_timer *st = this_cpu_ptr(&slice_timer);
> >> +
> >> +	/*
> >> +	 * st->cookie can be safely read as preemption is disabled and the
> >> +	 * timer is CPU local. The active check can obviously race with the
> >> +	 * hrtimer interrupt, but that's better than disabling interrupts
> >> +	 * unconditionally right away.
> >> +	 *
> >> +	 * As this is most probably the first expiring timer, the cancel is
> >> +	 * expensive as it has to reprogram the hardware, but that's less
> >> +	 * expensive than going through a full hrtimer_interrupt() cycle
> >> +	 * for nothing.
> >> +	 *
> >> +	 * hrtimer_try_to_cancel() is sufficient here as with interrupts
> >> +	 * disabled the timer callback cannot be running and the timer base
> >> +	 * is well determined as the timer is pinned on the local CPU.
> >> +	 */
> >> +	if (st->cookie == current && hrtimer_active(&st->timer)) {
> >> +		scoped_guard(irq)
> >> +			hrtimer_try_to_cancel(&st->timer);
> >
> > I don't see why hrtimer_active() and IRQ-disable is a benefit here.
> > Unless you want to avoid a branch to hrtimer_try_to_cancel().
> >
> > The function has its own hrtimer_active() check and disables interrupts
> > while accessing the hrtimer_base lock. Since preemption is disabled,
> > st->cookie remains stable.
> > It can fire right after the hrtimer_active() here. You could just
> >
> > 	if (st->cookie == current)
> > 		hrtimer_try_to_cancel(&st->timer);
> >
> > at the expense of a branch to hrtimer_try_to_cancel() if the timer
> > already expired (no interrupts off/on).
> 
> That's not equivalent. As this is CPU local the interrupt disable
> ensures that the timer is not running on this CPU. Otherwise you need
> hrtimer_cancel(). Read the comment. :)

Since it is a CPU local timer which is HRTIMER_MODE_HARD, from this CPUs
perspective it is either about to run or it did run. Therefore the
hrtimer_try_to_cancel() can't return -1 due to
hrtimer_callback_running() == true.
If you drop hrtimer_active() check and scoped_guard(irq),
hrtimer_try_to_cancel() will do the same hrtimer_active() check as you
have above followed by disable interrupts via lock_hrtimer_base() and
here hrtimer_callback_running() can't return true because interrupts are
disabled and the timer can't run on a remote CPU because it is a
CPU-local timer.

So you avoid a branch to hrtimer_try_to_cancel() if the timer already
fired.

> Thanks,
> 
>         tglx

Sebastian