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

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

[Prakash Sangappa]

semtimedop() should be converted to use hrtimer like it has been
done for most of the system calls with timeouts. This system call
already takes a struct timespec as an argument and can therefore
provide finer granularity timed wait.

Signed-off-by: Prakash Sangappa <prakash.sangappa@oracle.com>
---
 ipc/sem.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 0dbdb98..6cd1a1b8 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -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;
+	struct timespec64 end_time;
 
 	if (nsops < 1 || semid < 0)
 		return -EINVAL;
@@ -2008,7 +2011,9 @@ long __do_semtimedop(int semid, struct sembuf *sops,
 			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);
 	}
 
 
@@ -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();
 
@@ -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)
 			error = -EAGAIN;
 	} while (error == -EINTR && !signal_pending(current)); /* spurious */
 
-- 
2.7.4

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

[Prakash Sangappa]

> On Apr 18, 2022, at 6:51 PM, Prakash Sangappa <prakash.sangappa@oracle.com> wrote:
> 
> semtimedop() should be converted to use hrtimer like it has been
> done for most of the system calls with timeouts. This system call
> already takes a struct timespec as an argument and can therefore
> provide finer granularity timed wait.
> 
> Signed-off-by: Prakash Sangappa <prakash.sangappa@oracle.com>

Can I get a review of this patch?
Note this patch has been added to Andrew's mm tree.


> ---
> ipc/sem.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 0dbdb98..6cd1a1b8 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -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;
> +	struct timespec64 end_time;
> 
> 	if (nsops < 1 || semid < 0)
> 		return -EINVAL;
> @@ -2008,7 +2011,9 @@ long __do_semtimedop(int semid, struct sembuf *sops,
> 			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);
> 	}
> 
> 
> @@ -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();
> 
> @@ -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)
> 			error = -EAGAIN;
> 	} while (error == -EINTR && !signal_pending(current)); /* spurious */
> 
> -- 
> 2.7.4
> 

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

[Andrew Morton]

On Mon, 25 Apr 2022 19:38:44 +0000 Prakash Sangappa <prakash.sangappa@oracle.com> wrote:

> 
> 
> > On Apr 18, 2022, at 6:51 PM, Prakash Sangappa <prakash.sangappa@oracle.com> wrote:
> > 
> > semtimedop() should be converted to use hrtimer like it has been
> > done for most of the system calls with timeouts. This system call
> > already takes a struct timespec as an argument and can therefore
> > provide finer granularity timed wait.
> > 
> > Signed-off-by: Prakash Sangappa <prakash.sangappa@oracle.com>
> 
> Can I get a review of this patch?

That would be nice.

I looked at it, seemed OK.  Perhaps the usual IPC developers (Davidlohr
and Manfred) can comment?

> Note this patch has been added to Andrew's mm tree.
> 
> 
> > ---
> > ipc/sem.c | 15 +++++++++++----
> > 1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/ipc/sem.c b/ipc/sem.c
> > index 0dbdb98..6cd1a1b8 100644
> > --- a/ipc/sem.c
> > +++ b/ipc/sem.c
> > @@ -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;
> > +	struct timespec64 end_time;
> > 
> > 	if (nsops < 1 || semid < 0)
> > 		return -EINVAL;
> > @@ -2008,7 +2011,9 @@ long __do_semtimedop(int semid, struct sembuf *sops,
> > 			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);
> > 	}
> > 
> > 
> > @@ -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();
> > 
> > @@ -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)
> > 			error = -EAGAIN;
> > 	} while (error == -EINTR && !signal_pending(current)); /* spurious */
> > 
> > -- 
> > 2.7.4
> > 
> 

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

[Thomas Gleixner]

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?

> +	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()?

>  			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);

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.

> @@ -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,

        tglx

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

[Prakash Sangappa]

> On Apr 27, 2022, at 3:06 PM, Thomas Gleixner <tglx@linutronix.de> 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

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

[Thomas Gleixner]

On Wed, Apr 27 2022 at 23:42, Prakash Sangappa wrote:
>> On Apr 27, 2022, at 3:06 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> 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));

Of course. This was just for illustration and I assumed you figure it
out, which you did. :)

Thanks,

        tglx