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

[PATCH v3] 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>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---
v1->v2:
  - Use timespec64_valid() to validate timeout
     and other changes as suggested by Thomas Gleixner
v2-v3 Added Reviewed by tag
---
 ipc/sem.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 0dbdb98..44b65b6 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1995,7 +1995,9 @@ 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, *exp = NULL;
+	bool timed_out = false;
 
 	if (nsops < 1 || semid < 0)
 		return -EINVAL;
@@ -2003,12 +2005,11 @@ long __do_semtimedop(int semid, struct sembuf *sops,
 		return -E2BIG;
 
 	if (timeout) {
-		if (timeout->tv_sec < 0 || timeout->tv_nsec < 0 ||
-			timeout->tv_nsec >= 1000000000L) {
-			error = -EINVAL;
-			goto out;
-		}
-		jiffies_left = timespec64_to_jiffies(timeout);
+		if (!timespec64_valid(timeout))
+			return -EINVAL;
+		expires = ktime_add_safe(ktime_get(),
+				timespec64_to_ktime(*timeout));
+		exp = &expires;
 	}
 
 
@@ -2166,10 +2167,8 @@ long __do_semtimedop(int semid, struct sembuf *sops,
 		sem_unlock(sma, locknum);
 		rcu_read_unlock();
 
-		if (timeout)
-			jiffies_left = schedule_timeout(jiffies_left);
-		else
-			schedule();
+		timed_out = !schedule_hrtimeout_range(exp,
+				current->timer_slack_ns, HRTIMER_MODE_ABS);
 
 		/*
 		 * fastpath: the semop has completed, either successfully or
@@ -2210,7 +2209,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 (timed_out)
 			error = -EAGAIN;
 	} while (error == -EINTR && !signal_pending(current)); /* spurious */
 
-- 
2.7.4

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

[Davidlohr Bueso]

On Thu, 28 Apr 2022, Prakash Sangappa 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>
>Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
>---
>v1->v2:
>  - Use timespec64_valid() to validate timeout
>     and other changes as suggested by Thomas Gleixner
>v2-v3 Added Reviewed by tag
>---

The second '---' is not necesary, it's the first one that counts.

>@@ -2166,10 +2167,8 @@ long __do_semtimedop(int semid, struct sembuf *sops,
>		sem_unlock(sma, locknum);
>		rcu_read_unlock();
>
>-		if (timeout)
>-			jiffies_left = schedule_timeout(jiffies_left);
>-		else
>-			schedule();
>+		timed_out = !schedule_hrtimeout_range(exp,
>+				current->timer_slack_ns, HRTIMER_MODE_ABS);

I'm wondering if the slack parameter instead of passing the timer_slack_ns
value immediately, we should do a rt_task() check and pass zero if so.
And the opposite for the posix mqueue case: bit a little more lenient when
!rt_task() and pass current->timer_slack_ns instead of zero. Of course mq
is a lot more rt aware than sysv semaphores, but that doesn't mean both forms
of ipc are not seen used with or without RT constraints.

Thanks,
Davidlohr

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

[Thomas Gleixner]

On Thu, Apr 28 2022 at 13:50, Davidlohr Bueso wrote:
> On Thu, 28 Apr 2022, Prakash Sangappa wrote:
>>-		if (timeout)
>>-			jiffies_left = schedule_timeout(jiffies_left);
>>-		else
>>-			schedule();
>>+		timed_out = !schedule_hrtimeout_range(exp,
>>+				current->timer_slack_ns, HRTIMER_MODE_ABS);
>
> I'm wondering if the slack parameter instead of passing the timer_slack_ns
> value immediately, we should do a rt_task() check and pass zero if so.

We should have a wrapper function which takes care of that instead of
having checks all over the place.

Thanks,

        tglx

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

[Prakash Sangappa]

> On Apr 28, 2022, at 3:02 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Thu, Apr 28 2022 at 13:50, Davidlohr Bueso wrote:
>> On Thu, 28 Apr 2022, Prakash Sangappa wrote:
>>> -		if (timeout)
>>> -			jiffies_left = schedule_timeout(jiffies_left);
>>> -		else
>>> -			schedule();
>>> +		timed_out = !schedule_hrtimeout_range(exp,
>>> +				current->timer_slack_ns, HRTIMER_MODE_ABS);
>> 
>> I'm wondering if the slack parameter instead of passing the timer_slack_ns
>> value immediately, we should do a rt_task() check and pass zero if so.
> 
> We should have a wrapper function which takes care of that instead of
> having checks all over the place.

Ok  it can be an inline function in sched.h which returns appropriate 
slack time. Use that in  futex_wait() and sigtimedwait() also in addition to 
semtimedop() & mqueue codepath?

Should that be a separate patch?

Thanks,
-Prakash

> 
> Thanks,
> 
>        tglx

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

[Thomas Gleixner]

On Thu, Apr 28 2022 at 22:23, Prakash Sangappa wrote:
>> On Apr 28, 2022, at 3:02 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Thu, Apr 28 2022 at 13:50, Davidlohr Bueso wrote:
>>> On Thu, 28 Apr 2022, Prakash Sangappa wrote:
>>>> -		if (timeout)
>>>> -			jiffies_left = schedule_timeout(jiffies_left);
>>>> -		else
>>>> -			schedule();
>>>> +		timed_out = !schedule_hrtimeout_range(exp,
>>>> +				current->timer_slack_ns, HRTIMER_MODE_ABS);
>>> 
>>> I'm wondering if the slack parameter instead of passing the timer_slack_ns
>>> value immediately, we should do a rt_task() check and pass zero if so.
>> 
>> We should have a wrapper function which takes care of that instead of
>> having checks all over the place.
>
> Ok  it can be an inline function in sched.h which returns appropriate 
> slack time. Use that in  futex_wait() and sigtimedwait() also in addition to 
> semtimedop() & mqueue codepath?

No. What I meant is a function which handles this internally, not an inline
function which has to be invoked on various call sites.

> Should that be a separate patch?

Definitely. That's an orthogonal problem.

Thanks,

        tglx

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

[Davidlohr Bueso]

On Thu, 28 Apr 2022, Prakash Sangappa 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>
>Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

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

[Davidlohr Bueso]

On Fri, 29 Apr 2022, Thomas Gleixner wrote:

>No. What I meant is a function which handles this internally, not an inline
>function which has to be invoked on various call sites.

Originally I was also thinking about it on a per-user basis, but
after more thought I agree it's better if it is done by the hrtimer.

Something like so?

Thanks,
Davidlohr

---------------8<-----------------------------------------------------
[PATCH] hrtimer: Ignore slack time for RT tasks

While in theory the timer can be triggered before expires+delta,
for the cases of RT tasks they really have no business giving
any lenience for extra slack time, so override any passed value
by the user and always use zero.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
  kernel/time/hrtimer.c | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 0ea8702eb516..5ef0f1651040 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -2297,6 +2297,13 @@ schedule_hrtimeout_range_clock(ktime_t *expires, u64 delta,
		return -EINTR;
	}

+	/*
+	 * Override any slack passed by the user if under
+	 * rt contraints.
+	 */
+	if (rt_task(current))
+		delta = 0;
+
	hrtimer_init_sleeper_on_stack(&t, clock_id, mode);
	hrtimer_set_expires_range_ns(&t.timer, *expires, delta);
	hrtimer_sleeper_start_expires(&t, mode);
@@ -2326,6 +2333,7 @@ schedule_hrtimeout_range_clock(ktime_t *expires, u64 delta,
   * actual wakeup to a time that is both power and performance friendly.
   * The kernel give the normal best effort behavior for "@expires+@delta",
   * but may decide to fire the timer earlier, but no earlier than @expires.
+ * For scenarios under realtime constraints, @delta is always zero.
   *
   * You can set the task state as follows -
   *
--
2.36.0