Re: [patch 04/11] posix-timers: Remove pointless unlock_timer() wrapper

[Thomas Gleixner]

On Mon, Feb 24 2025 at 17:21, Peter Zijlstra wrote:
> On Mon, Feb 24, 2025 at 11:15:28AM +0100, Thomas Gleixner wrote:
>> It's just a wrapper around spin_unlock_irqrestore() with zero value.
>
> Well, I disagree... the value is that is matches lock_timer(). Both in
> naming and in argument types.

Sure, but it's not used consistently as we have places where
lock_timer() is not involved.

> @@ -327,14 +350,13 @@ bool posixtimer_deliver_signal(struct ke
>  	 * Release siglock to ensure proper locking order versus
>  	 * timr::it_lock. Keep interrupts disabled.
>  	 */
> -	spin_unlock(&current->sighand->siglock);
> +	guard(spinlock)(&current->sighand->siglock);

How is that equivalent?

This is a unlock/lock pair because __posix_timer_deliver_signal() takes
timr->it_lock and now you introduced the ABBA which the sighand::siglock
unlock carefully avoids :)

>  
>  	ret = __posixtimer_deliver_signal(info, timr);
> 
>  	/* Drop the reference which was acquired when the signal was queued */
>  	posixtimer_putref(timr);
>  
> -	spin_lock(&current->sighand->siglock);
>  	return ret;
>  }
>  
> @@ -717,24 +739,20 @@ void common_timer_get(struct k_itimer *t
>  
>  static int do_timer_gettime(timer_t timer_id,  struct itimerspec64 *setting)
>  {
> -	const struct k_clock *kc;
> -	struct k_itimer *timr;
> -	unsigned long flags;
> -	int ret = 0;
> -
> -	timr = lock_timer(timer_id, &flags);
> -	if (!timr)
> -		return -EINVAL;
> +	scoped_guard (lock_timer, timer_id) {
> +		struct k_itimer *timr = __guard_ptr(lock_timer)(&scope);
> +		const struct k_clock *kc;
> +
> +		memset(setting, 0, sizeof(*setting));
> +		kc = timr->kclock;
> +		if (WARN_ON_ONCE(!kc || !kc->timer_get))
> +			return -EINVAL;
>  
> -	memset(setting, 0, sizeof(*setting));
> -	kc = timr->kclock;
> -	if (WARN_ON_ONCE(!kc || !kc->timer_get))
> -		ret = -EINVAL;
> -	else
>  		kc->timer_get(timr, setting);
> +		return 0;
> +	}
>  
> -	spin_unlock_irqrestore(&timr->it_lock, flags);
> -	return ret;
> +	return -EINVAL;

So the resulting code is:

	scoped_guard (lock_timer, timer_id) {
		struct k_itimer *timr = __guard_ptr(lock_timer)(&scope);
		const struct k_clock *kc;

		memset(setting, 0, sizeof(*setting));
		kc = timr->kclock;
		if (WARN_ON_ONCE(!kc || !kc->timer_get))
			return -EINVAL;
 
		return 0;
	}
	return -EINVAL;

I had to go and stare at the guard/class muck 10 times to convince
myself, that this actually works. This really wants to be express the
condition of the scoped_guard() somehow, e.g. scoped_cond_guard() or
such.

>  /* Delete a POSIX.1b interval timer. */
>  SYSCALL_DEFINE1(timer_delete, timer_t, timer_id)
>  {
> -	return posix_timer_delete(NULL, timer_id);
> +	scoped_guard (lock_timer, timer_id) {
> +		posix_timer_invalidate(scope.lock, scope.flags);
> +		scoped_guard_end(lock_timer);
> +		posix_timer_unhash_and_free(scope.lock);

Not sure whether it's a good idea to free the scope.lock and not
scope.timer :)

Thanks,

        tglx