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 <[email protected]>
---
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 Apr 18, 2022, at 6:51 PM, Prakash Sangappa <[email protected]> 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 <[email protected]>
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 <[email protected]> wrote:
>
>
> > On Apr 18, 2022, at 6:51 PM, Prakash Sangappa <[email protected]> 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 <[email protected]>
>
> 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
> >
>
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
> On Apr 27, 2022, at 3:06 PM, Thomas Gleixner <[email protected]> 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 <[email protected]> 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