Re: [PATCH] ipc: Update semtimedop() to use hrtimer

[Prakash Sangappa]

> On Apr 27, 2022, at 3:06 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> 
> Prakash,
> 
> On Mon, Apr 18 2022 at 18:51, Prakash Sangappa wrote:
>> @@ -1995,7 +1995,10 @@ long __do_semtimedop(int semid, struct sembuf *sops,
>> 	int max, locknum;
>> 	bool undos = false, alter = false, dupsop = false;
>> 	struct sem_queue queue;
>> -	unsigned long dup = 0, jiffies_left = 0;
>> +	unsigned long dup = 0;
>> +	ktime_t expires;
>> +	int timed_out = 0;
> 
> bool perhaps?

Sure, will change that.

> 
>> +	struct timespec64 end_time;
>> 
>> 	if (nsops < 1 || semid < 0)
>> 		return -EINVAL;
>> @@ -2008,7 +2011,9 @@ long __do_semtimedop(int semid, struct sembuf *sops,
> 
> While at it, can you please replace the open coded validation of timeout
> with timespec64_valid()?

Will do

> 
>> 			error = -EINVAL;
>> 			goto out;
>> 		}
>> -		jiffies_left = timespec64_to_jiffies(timeout);
>> +		ktime_get_ts64(&end_time);
>> +		end_time = timespec64_add_safe(end_time, *timeout);
>> +		expires = timespec64_to_ktime(end_time);
> 
> Converting to ktime first makes this cheaper:
> 
>                expires = ktime_get() + timespec64_to_ns(timeout);

Since user provided timespec is added to current time, shouldn’t it check for overflow?

So, perhaps 

		expires = ktime_add_safe(ktime_get(), timespec64_to_ns(timeout));

> 
> Less code lines and shorter execution time because adding scalars is
> obviously cheaper than adding timespecs.
> 
> Now if you add:
> 
>       ktime_t expires, *exp = NULL;
> 
> then you can do here:
> 
>                exp = &expires;
>> 	}
>> 
>> 
>> @@ -2167,7 +2172,9 @@ long __do_semtimedop(int semid, struct sembuf *sops,
>> 		rcu_read_unlock();
>> 
>> 		if (timeout)
>> -			jiffies_left = schedule_timeout(jiffies_left);
>> +			timed_out = !schedule_hrtimeout_range(&expires,
>> +						current->timer_slack_ns,
>> +						HRTIMER_MODE_ABS);
>> 		else
>> 			schedule();
> 
> and this can be simplified to:
> 
>                timed_out = !schedule_hrtimeout_range(exp, current->timer_slack_ns,
> 						      HRTIMER_MODE_ABS)
> 
> schedule_hrtimeout_range() directly invokes schedule() when @exp == NULL
> and returns != 0 when woken up in that case.

Sure that makes it cleaner

> 
>> @@ -2210,7 +2217,7 @@ long __do_semtimedop(int semid, struct sembuf *sops,
>> 		/*
>> 		 * If an interrupt occurred we have to clean up the queue.
>> 		 */
>> -		if (timeout && jiffies_left == 0)
>> +		if (timeout && timed_out)
> 
> and this becomes
> 
>                if (timed_out)
> 
>> 			error = -EAGAIN;
>> 	} while (error == -EINTR && !signal_pending(current)); /* spurious */
> 
> Hmm?
> 
> Done right, you should end up with a negative diffstat :)

Thanks for the review.
Will send out update patch.

-Prakash


> 
> Thanks,
> 
>        tglx