ipc/sem.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
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.4Prakash,
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
> 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
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
> 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 >
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 > > >
© 2016 - 2026 Red Hat, Inc.