Received-SPF: pass (google.com: domain of linux-kernel+bounces-143274-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Thomas Gleixner <tglx@linutronix.de>
To: "Eric W. Biederman" <ebiederm@xmission.com>
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>
Subject: Re: [patch V2 10/50] posix-cpu-timers: Handle SIGEV_NONE timers
 correctly in timer_get()
In-Reply-To: <87cyquxw59.fsf@email.froward.int.ebiederm.org>
References: <20240410164558.316665885@linutronix.de>
 <20240410165551.571240036@linutronix.de>
 <87cyquxw59.fsf@email.froward.int.ebiederm.org>
Date: Fri, 12 Apr 2024 21:49:35 +0200
Message-ID: <87h6g6z7hs.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Fri, Apr 12 2024 at 13:40, Eric W. Biederman wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
>> -	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?

Duh, yes.
  		/*
>> -		 * 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.

Fair enough.

Thanks,

        tglx