Re: [patch V2 08/17] posix-timers: Rework timer removal

[Pavel Tikhomirov]

On 3/4/25 18:10, Pavel Tikhomirov wrote:
>    > -/* Delete a POSIX.1b interval timer. */
> > -SYSCALL_DEFINE1(timer_delete, timer_t, timer_id)
> > +static void posix_timer_delete(struct k_itimer *timer)
> >   {
> > -    struct k_itimer *timer = lock_timer(timer_id);
> > -
> > -retry_delete:
> > -    if (!timer)
> > -        return -EINVAL;
> > -
> > -    /* Prevent signal delivery and rearming. */
> > +    /*
> > +     * Invalidate the timer, remove it from the linked list and remove
> > +     * it from the ignored list if pending.
> > +     *
> > +     * The invalidation must be written with siglock held so that the
> > +     * signal code observes timer->it_valid == false in do_sigaction(),
> > +     * which prevents it from moving a pending signal of a deleted
> > +     * timer to the ignore list.
> > +     *
> > +     * The invalidation also prevents signal queueing, signal delivery
> > +     * and therefore rearming from the signal delivery path.
> > +     *
> > +     * A concurrent lookup can still find the timer in the hash, but it
> > +     * will check timer::it_signal with timer::it_lock held and observe
> > +     * bit 0 set, which invalidates it. That also prevents the timer ID
> > +     * from being handed out before this timer is completely gone.
> > +     */
> >       timer->it_signal_seq++;
> > -    if (unlikely(timer->kclock->timer_del(timer) == TIMER_RETRY)) {
> > -        /* Unlocks and relocks the timer if it still exists */
> > -        timer = timer_wait_running(timer);
> > -        goto retry_delete;
> > -    }
> > -
> >       scoped_guard (spinlock, &current->sighand->siglock) {
> > +        unsigned long sig = (unsigned long)timer->it_signal | 1UL;
> > +
> > +        WRITE_ONCE(timer->it_signal, (struct signal_struct *)sig);
> >           hlist_del(&timer->list);
> >           posix_timer_cleanup_ignored(timer);
> > -        /*
> > -         * A concurrent lookup could check timer::it_signal lockless. It
> > -         * will reevaluate with timer::it_lock held and observe the 
> > NULL.
> > -         *
> > -         * It must be written with siglock held so that the signal code
> > -         * observes timer->it_signal == NULL in do_sigaction(SIG_IGN),
> > -         * which prevents it from moving a pending signal of a deleted
> > -         * timer to the ignore list.
> > -         */
> > -        WRITE_ONCE(timer->it_signal, NULL);
> >       }
> > -    unlock_timer(timer);
> > -    posix_timer_unhash_and_free(timer);
> > -    return 0;
> > +    while (timer->kclock->timer_del(timer) == TIMER_RETRY) {
> > +        guard(rcu)();
> > +        spin_unlock_irq(&timer->it_lock);
>
> Maybe "guard(spinlock_irq)(&timer->it_lock);", since we already use 
> guard constructions everywhere else?

Sorry I'm just stupid. Please ignore.

> > +        timer_wait_running(timer);
> > +        spin_lock_irq(&timer->it_lock);
> > +    }
> >   }

--
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.