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

[Pavel Tikhomirov <ptikhomirov@virtuozzo.com>]

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.