[PATCH v2 0/1] hrtimer: More fixes for handling of timer slack of rt tasks

[PATCH v2 0/1] hrtimer: More fixes for handling of timer slack of rt tasks

[Felix Moessbauer]

This series fixes the (hopefully) last location of an incorrectly
handled timer slack on rt tasks in hrtimer_start_range_ns(), which was
uncovered by a userland change in glibc 2.33.

Changes since v1:

- drop patch "hrtimer: Document, that PI boosted tasks have no timer slack", as
  this behavior is incorrect and is already adressed in 20240610192018.1567075-1-qyousef@layalina.io
- use task_is_realtime() instead of rt_task()
- fix style of commit message

v1 discussion: https://lore.kernel.org/lkml/20240805124116.21394-1-felix.moessbauer@siemens.com

Best regards,
Felix Moessbauer
Siemens AG

Felix Moessbauer (1):
  hrtimer: Ignore slack time for RT tasks in hrtimer_start_range_ns()

 kernel/time/hrtimer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH v2 1/1] hrtimer: Ignore slack time for RT tasks in hrtimer_start_range_ns()

[Felix Moessbauer]

RT tasks do not have any timerslack, as this induces jitter. By
that, the timer slack is already ignored in the nanosleep family and
schedule_hrtimeout_range() (fixed in 0c52310f2600).

The hrtimer_start_range_ns function is indirectly used by glibc-2.33+
for timed waits on condition variables. These are sometimes used in
RT applications for realtime queue processing. At least on the
combination of kernel 5.10 and glibc-2.31, the timed wait on condition
variables in rt tasks was precise (no slack), however glibc-2.33
changed the internal wait implementation, exposing this oversight.

Make the timer slack consistent across all hrtimer programming code,
by ignoring the timerslack for tasks with rt policies also in the last
remaining location in hrtimer_start_range_ns().

Cc: stable@vger.kernel.org
Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
---
 kernel/time/hrtimer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index b8ee320208d4..e8b44e7c281f 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1274,7 +1274,7 @@ static int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
  * hrtimer_start_range_ns - (re)start an hrtimer
  * @timer:	the timer to be added
  * @tim:	expiry time
- * @delta_ns:	"slack" range for the timer
+ * @delta_ns:	"slack" range for the timer for SCHED_OTHER tasks
  * @mode:	timer mode: absolute (HRTIMER_MODE_ABS) or
  *		relative (HRTIMER_MODE_REL), and pinned (HRTIMER_MODE_PINNED);
  *		softirq based mode is considered for debug purpose only!
@@ -1299,6 +1299,10 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 
 	base = lock_hrtimer_base(timer, &flags);
 
+	/* rt-tasks do not have a timer slack for obvious reasons */
+	if (task_is_realtime(current))
+		delta_ns = 0;
+
 	if (__hrtimer_start_range_ns(timer, tim, delta_ns, mode, base))
 		hrtimer_reprogram(timer, true);
 
-- 
2.39.2

Re: [PATCH v2 0/1] hrtimer: More fixes for handling of timer slack of rt tasks

[Qais Yousef]

On 08/05/24 16:09, Felix Moessbauer wrote:
> This series fixes the (hopefully) last location of an incorrectly
> handled timer slack on rt tasks in hrtimer_start_range_ns(), which was
> uncovered by a userland change in glibc 2.33.
> 
> Changes since v1:
> 
> - drop patch "hrtimer: Document, that PI boosted tasks have no timer slack", as
>   this behavior is incorrect and is already adressed in 20240610192018.1567075-1-qyousef@layalina.io

There was discussion about this hrtimer usage in earlier version if it helps to
come up with a potentially better patch

	https://lore.kernel.org/lkml/20240521110035.KRIwllGe@linutronix.de/

My patches got picked up by the way, you'd probably want to rebase and resend
as now the function is named rt_or_dl_task_policy()


Cheers

--
Qais Yousef

> - use task_is_realtime() instead of rt_task()
> - fix style of commit message
> 
> v1 discussion: https://lore.kernel.org/lkml/20240805124116.21394-1-felix.moessbauer@siemens.com
> 
> Best regards,
> Felix Moessbauer
> Siemens AG
> 
> Felix Moessbauer (1):
>   hrtimer: Ignore slack time for RT tasks in hrtimer_start_range_ns()
> 
>  kernel/time/hrtimer.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 0/1] hrtimer: More fixes for handling of timer slack of rt tasks

[MOESSBAUER, Felix]

On Fri, 2024-08-09 at 02:34 +0100, Qais Yousef wrote:
> On 08/05/24 16:09, Felix Moessbauer wrote:
> > This series fixes the (hopefully) last location of an incorrectly
> > handled timer slack on rt tasks in hrtimer_start_range_ns(), which
> > was
> > uncovered by a userland change in glibc 2.33.
> > 
> > Changes since v1:
> > 
> > - drop patch "hrtimer: Document, that PI boosted tasks have no
> > timer slack", as
> >   this behavior is incorrect and is already adressed in
> > 20240610192018.1567075-1-qyousef@layalina.io
> 
> There was discussion about this hrtimer usage in earlier version if
> it helps to
> come up with a potentially better patch

Hi, Sebastian already pointed me to this thread.

When debugging my issue, I did not know about it but was scratching my
head if the behavior / usage of rt_task is actually correct.
The whole naming was quite confusing. Many thanks for cleaning that up.

> 
>         
> https://lore.kernel.org/lkml/20240521110035.KRIwllGe@linutronix.de/
> 
> My patches got picked up by the way, you'd probably want to rebase
> and resend
> as now the function is named rt_or_dl_task_policy()

As we use rt_or_dl_task() in nanosleep, I'm wondering if we should use
the same in hrtimer_start_range_ns(). Is that because PI boosted tasks
need to acquire a lock which can only be a mutex_t or equivalent
sleeping lock on PREEMPT_RT?

Anyways, I'm thinking about getting rid of the policy based delta=0 and
just set the task->timer_slack_ns to 0 when changing the scheduling
policy (and changing it back to the default when reverting to
SCHED_OTHER). By that, we can get rid of the special handling and users
of the procfs would also see correct data in /timerslack_ns.

Felix

> 
> 
> Cheers
> 
> --
> Qais Yousef
> 
> > - use task_is_realtime() instead of rt_task()
> > - fix style of commit message
> > 
> > v1 discussion:
> > https://lore.kernel.org/lkml/20240805124116.21394-1-felix.moessbauer@siemens.com
> > 
> > Best regards,
> > Felix Moessbauer
> > Siemens AG
> > 
> > Felix Moessbauer (1):
> >   hrtimer: Ignore slack time for RT tasks in
> > hrtimer_start_range_ns()
> > 
> >  kernel/time/hrtimer.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > -- 
> > 2.39.2
> > 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v2 0/1] hrtimer: More fixes for handling of timer slack of rt tasks

[Thomas Gleixner]

On Fri, Aug 09 2024 at 08:47, Felix MOESSBAUER wrote:
> On Fri, 2024-08-09 at 02:34 +0100, Qais Yousef wrote:
>> On 08/05/24 16:09, Felix Moessbauer wrote:
>> > This series fixes the (hopefully) last location of an incorrectly
>> > handled timer slack on rt tasks in hrtimer_start_range_ns(), which
>> > was
>> > uncovered by a userland change in glibc 2.33.
>> > 
>> > Changes since v1:
>> > 
>> > - drop patch "hrtimer: Document, that PI boosted tasks have no
>> > timer slack", as
>> >   this behavior is incorrect and is already adressed in
>> > 20240610192018.1567075-1-qyousef@layalina.io
>> 
>> There was discussion about this hrtimer usage in earlier version if
>> it helps to
>> come up with a potentially better patch
>
> Hi, Sebastian already pointed me to this thread.
>
> When debugging my issue, I did not know about it but was scratching my
> head if the behavior / usage of rt_task is actually correct.
> The whole naming was quite confusing. Many thanks for cleaning that up.
>
>> 
>>         
>> https://lore.kernel.org/lkml/20240521110035.KRIwllGe@linutronix.de/
>> 
>> My patches got picked up by the way, you'd probably want to rebase
>> and resend
>> as now the function is named rt_or_dl_task_policy()
>
> As we use rt_or_dl_task() in nanosleep, I'm wondering if we should use
> the same in hrtimer_start_range_ns(). Is that because PI boosted tasks
> need to acquire a lock which can only be a mutex_t or equivalent
> sleeping lock on PREEMPT_RT?

No. Arming the timer has nothing to do with mutexes or such. It's an
optimization to grant RT/DL tasks zero slack automatically.

The correct thing is to use policy based delta adjustment.

The fact that a task got boosted temporatily does not make it eligble
for zero slack. It stays a SCHED_OTHER task no matter what.

rt_or_dl_task() in nanosleep() is fundamentally wrong and needs to be
replaced with rt_or_dl_task_policy() and not the other way round.

> Anyways, I'm thinking about getting rid of the policy based delta=0 and
> just set the task->timer_slack_ns to 0 when changing the scheduling
> policy (and changing it back to the default when reverting to
> SCHED_OTHER). By that, we can get rid of the special handling and users
> of the procfs would also see correct data in /timerslack_ns.

That makes sense.

Thanks

        tglx