* [PATCH] ipc: Update semtimedop() to use hrtimer @ 2022-04-19 1:51 Prakash Sangappa 2022-04-25 19:38 ` Prakash Sangappa 2022-04-27 22:06 ` Thomas Gleixner 0 siblings, 2 replies; 6+ messages in thread From: Prakash Sangappa @ 2022-04-19 1:51 UTC (permalink / raw) To: linux-kernel; +Cc: akpm, tglx, peterz, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ipc: Update semtimedop() to use hrtimer 2022-04-19 1:51 [PATCH] ipc: Update semtimedop() to use hrtimer Prakash Sangappa @ 2022-04-25 19:38 ` Prakash Sangappa 2022-04-25 20:50 ` Andrew Morton 2022-04-27 22:06 ` Thomas Gleixner 1 sibling, 1 reply; 6+ messages in thread From: Prakash Sangappa @ 2022-04-25 19:38 UTC (permalink / raw) To: LKML; +Cc: akpm@linux-foundation.org, Thomas Gleixner, Peter Zijlstra > 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipc: Update semtimedop() to use hrtimer 2022-04-25 19:38 ` Prakash Sangappa @ 2022-04-25 20:50 ` Andrew Morton 0 siblings, 0 replies; 6+ messages in thread From: Andrew Morton @ 2022-04-25 20:50 UTC (permalink / raw) To: Prakash Sangappa Cc: LKML, Thomas Gleixner, Peter Zijlstra, Davidlohr Bueso, Manfred Spraul 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 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipc: Update semtimedop() to use hrtimer 2022-04-19 1:51 [PATCH] ipc: Update semtimedop() to use hrtimer Prakash Sangappa 2022-04-25 19:38 ` Prakash Sangappa @ 2022-04-27 22:06 ` Thomas Gleixner 2022-04-27 23:42 ` Prakash Sangappa 1 sibling, 1 reply; 6+ messages in thread From: Thomas Gleixner @ 2022-04-27 22:06 UTC (permalink / raw) To: Prakash Sangappa, linux-kernel; +Cc: akpm, peterz, prakash.sangappa 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipc: Update semtimedop() to use hrtimer 2022-04-27 22:06 ` Thomas Gleixner @ 2022-04-27 23:42 ` Prakash Sangappa 2022-04-28 7:47 ` Thomas Gleixner 0 siblings, 1 reply; 6+ messages in thread From: Prakash Sangappa @ 2022-04-27 23:42 UTC (permalink / raw) To: Thomas Gleixner Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org, peterz@infradead.org > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipc: Update semtimedop() to use hrtimer 2022-04-27 23:42 ` Prakash Sangappa @ 2022-04-28 7:47 ` Thomas Gleixner 0 siblings, 0 replies; 6+ messages in thread From: Thomas Gleixner @ 2022-04-28 7:47 UTC (permalink / raw) To: Prakash Sangappa Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org, peterz@infradead.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-28 7:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-04-19 1:51 [PATCH] ipc: Update semtimedop() to use hrtimer Prakash Sangappa 2022-04-25 19:38 ` Prakash Sangappa 2022-04-25 20:50 ` Andrew Morton 2022-04-27 22:06 ` Thomas Gleixner 2022-04-27 23:42 ` Prakash Sangappa 2022-04-28 7:47 ` Thomas Gleixner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox