Received-SPF: pass (google.com: domain of linux-kernel+bounces-143219-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: "Eric W. Biederman" <ebiederm@xmission.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>,  Anna-Maria Behnsen
 <anna-maria@linutronix.de>,  Frederic Weisbecker <frederic@kernel.org>,
  John Stultz <jstultz@google.com>,  Peter Zijlstra <peterz@infradead.org>,
  Ingo Molnar <mingo@kernel.org>,  Stephen Boyd <sboyd@kernel.org>,  Oleg
 Nesterov <oleg@redhat.com>
References: <20240410164558.316665885@linutronix.de>
	<20240410165551.571240036@linutronix.de>
Date: Fri, 12 Apr 2024 13:40:02 -0500
In-Reply-To: <20240410165551.571240036@linutronix.de> (Thomas Gleixner's
	message of "Thu, 11 Apr 2024 00:46:29 +0200 (CEST)")
Message-ID: <87cyquxw59.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [patch V2 10/50] posix-cpu-timers: Handle SIGEV_NONE timers
 correctly in timer_get()

Thomas Gleixner <tglx@linutronix.de> writes:

> Expired SIGEV_NONE oneshot timers must return 0 nsec for the expiry time in
> timer_get(), but the posix CPU timer implementation returns 1 nsec.
>
> Add the missing conditional.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> V2: Split out into new patch to make review simpler - Frederic
> ---
>  kernel/time/posix-cpu-timers.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -787,15 +787,17 @@ static int posix_cpu_timer_set(struct k_
>  
>  static void __posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec64 *itp, u64 now)
>  {
> +	bool sigev_none = timer->it_sigev_notify == SIGEV_NONE;
>  	u64 expires, iv = timer->it_interval;
>  
>  	/*
>  	 * Make sure that interval timers are moved forward for the
>  	 * following cases:
> +	 *  - SIGEV_NONE timers which are never armed
>  	 *  - Timers which expired, but the signal has not yet been
>  	 *    delivered
>  	 */
> -	if (iv && (timer->it_requeue_pending & REQUEUE_PENDING))
> +	if (iv && ((timer->it_requeue_pending & REQUEUE_PENDING) || sigev_none))
>  		expires = bump_cpu_timer(timer, now);
>  	else
>  		expires = cpu_timer_getexpires(&timer->it.cpu);
> @@ -809,11 +811,13 @@ static void __posix_cpu_timer_get(struct
>  		itp->it_value = ns_to_timespec64(expires - now);
>  	} else {
Why not make this else condition?
	} else if (!sigev_none) {
And not need to change the rest of the code?
>  		/*
> -		 * The timer should have expired already, but the firing
> -		 * hasn't taken place yet.  Say it's just about to expire.
> +		 * A single shot SIGEV_NONE timer must return 0, when it is
> +		 * expired! Timers which have a real signal delivery mode
> +		 * must return a remaining time greater than 0 because the
> +		 * signal has not yet been delivered.
>  		 */
> -		itp->it_value.tv_nsec = 1;
> -		itp->it_value.tv_sec = 0;
> +		if (!sigev_none)
> +			itp->it_value.tv_nsec = 1;

Do you perhaps need a comment somewhere that itp is zeroed in
do_timer_gettime?  The code now depends upon that for setting
itp->it_value when it did not used to, making it look at first
glance like you have created an uninitialized variable.

Probably just something in the description of the change would be
sufficient.

>  	}
>  }
>