Re: [patch 13/45] posix-cpu-timers: Replace old expiry retrieval in posix_cpu_timer_set()

[Frederic Weisbecker]

On Tue, Jun 06, 2023 at 04:37:39PM +0200, Thomas Gleixner wrote:
> Reuse the split out __posix_cpu_timer_get() function which does already the
> right thing.
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
>  kernel/time/posix-cpu-timers.c |   28 +++++-----------------------
>  1 file changed, 5 insertions(+), 23 deletions(-)
> 
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -609,6 +609,8 @@ static void cpu_timer_fire(struct k_itim
>  	}
>  }
>  
> +static void __posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec64
> *itp, u64 now);

Perhaps you can move __posix_cpu_timer_get() above posix_cpu_timer_set()?

> +
>  /*
>   * Guts of sys_timer_settime for CPU timers.
>   * This is called with the timer locked and interrupts disabled.
> @@ -680,29 +682,9 @@ static int posix_cpu_timer_set(struct k_
>  	else
>  		now = cpu_clock_sample_group(clkid, p, !sigev_none);
>  
> -	if (old) {
> -		if (old_expires == 0) {
> -			old->it_value.tv_sec = 0;
> -			old->it_value.tv_nsec = 0;
> -		} else {
> -			/*
> -			 * Update the timer in case it has overrun already.
> -			 * If it has, we'll report it as having overrun and
> -			 * with the next reloaded timer already ticking,
> -			 * though we are swallowing that pending
> -			 * notification here to install the new setting.
> -			 */
> -			u64 exp = bump_cpu_timer(timer, now);
> -
> -			if (now < exp) {
> -				old_expires = exp - now;
> -				old->it_value = ns_to_timespec64(old_expires);
> -			} else {
> -				old->it_value.tv_nsec = 1;
> -				old->it_value.tv_sec = 0;
> -			}
> -		}
> -	}
> +	/* Retrieve the previous expiry value if requested. */
> +	if (old && old_expires)
> +		__posix_cpu_timer_get(timer, old, now);

The changelog should probably mention the desired side effect that sigev_none
will return a zero old value if expired.

Reviewed-by: Frederic Weisbecker <frederic@xxxxxxxxxx>

Thanks.

>  
>  	/* Retry if the timer expiry is running concurrently */
>  	if (unlikely(ret)) {
>