2022-04-22 19:29:04

by Prakash Sangappa

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

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


2022-04-25 22:28:02

by Prakash Sangappa

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



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

2022-04-26 14:50:30

by Andrew Morton

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

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

2022-04-27 22:59:21

by Thomas Gleixner

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

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

2022-04-28 03:03:52

by Prakash Sangappa

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



> 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

2022-04-28 13:38:43

by Thomas Gleixner

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

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