linux-kernel - Re: [PATCH v3 1/1] hrtimer: use and report correct timerslack values for realtime tasks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <76cd1f3e07c6c12a7dceac31cdf7a3ca8d1e8265.camel@siemens.com>
Date: Tue, 13 Aug 2024 10:23:02 +0000
From: "MOESSBAUER, Felix" <felix.moessbauer@...mens.com>
To: "tglx@...utronix.de" <tglx@...utronix.de>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
CC: "qyousef@...alina.io" <qyousef@...alina.io>, "frederic@...nel.org"
	<frederic@...nel.org>, "Kiszka, Jan" <jan.kiszka@...mens.com>,
	"bigeasy@...utronix.de" <bigeasy@...utronix.de>, "anna-maria@...utronix.de"
	<anna-maria@...utronix.de>
Subject: Re: [PATCH v3 1/1] hrtimer: use and report correct timerslack values
 for realtime tasks

On Tue, 2024-08-13 at 11:45 +0200, Thomas Gleixner wrote:
> On Tue, Aug 13 2024 at 09:29, Felix Moessbauer wrote:
> > @@ -2569,10 +2569,11 @@ static ssize_t timerslack_ns_write(struct
> > file *file, const char __user *buf,
> >         }
> >  
> >         task_lock(p);
> > -       if (slack_ns == 0)
> > -               p->timer_slack_ns = p->default_timer_slack_ns;
> > -       else
> > -               p->timer_slack_ns = slack_ns;
> > +       if (task_is_realtime(p))
> > +               slack_ns = 0;
> 
> This should respect the user supplied value, i.e.

Ok, but then we need to update the man page as well (see below).

> 
>     if (!task_is_realtime(p) && !slack_ns)
>             slack_ns = p->default_timer_slack_ns;
> 
> > +       else if (slack_ns == 0)
> > +               slack_ns = p->default_timer_slack_ns;
> > +       p->timer_slack_ns = slack_ns;
> >         task_unlock(p);
> >  
> >  out:
> > diff --git a/fs/select.c b/fs/select.c
> > index 9515c3fa1a03..153124ed50fd 100644
> > --- a/fs/select.c
> > +++ b/fs/select.c
> > @@ -77,19 +77,13 @@ u64 select_estimate_accuracy(struct timespec64
> > *tv)
> >  {
> >         u64 ret;
> >         struct timespec64 now;
> > -
> > -       /*
> > -        * Realtime tasks get a slack of 0 for obvious reasons.
> > -        */
> > -
> > -       if (rt_task(current))
> > -               return 0;
> > +       u64 slack = current->timer_slack_ns;
> >  
> >         ktime_get_ts64(&now);
> >         now = timespec64_sub(*tv, now);
> >         ret = __estimate_accuracy(&now);
> > -       if (ret < current->timer_slack_ns)
> > -               return current->timer_slack_ns;
> > +       if (ret < slack || slack == 0)
> > +               return slack;
> 
> Seriously? Do all the calculations first and then discard them when
> slack is 0?

Ok, I'll short circuit.

> 
> > diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
> > index ae1b42775ef9..195d2f2834a9 100644
> > --- a/kernel/sched/syscalls.c
> > +++ b/kernel/sched/syscalls.c
> > @@ -406,6 +406,14 @@ static void __setscheduler_params(struct
> > task_struct *p,
> >         else if (fair_policy(policy))
> >                 p->static_prio = NICE_TO_PRIO(attr->sched_nice);
> >  
> > +       /* rt-policy tasks do not have a timerslack */
> > +       if (task_is_realtime(p)) {
> > +               p->timer_slack_ns = 0;
> > +       } else if (p->timer_slack_ns == 0) {
> > +               /* when switching back to non-rt policy, restore
> > timerslack */
> > +               p->timer_slack_ns = p->default_timer_slack_ns;
> > +       }
> > +
> >         /*
> >          * __sched_setscheduler() ensures attr->sched_priority == 0
> > when
> >          * !rt_policy. Always setting this ensures that things like
> > diff --git a/kernel/sys.c b/kernel/sys.c
> > index 3a2df1bd9f64..e3c4cffb520c 100644
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -2557,6 +2557,8 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned
> > long, arg2, unsigned long, arg3,
> >                         error = current->timer_slack_ns;
> >                 break;
> >         case PR_SET_TIMERSLACK:
> > +               if (task_is_realtime(current))
> > +                       break;
> 
> Why are you declaring that a RT task has to have 0 slack if we are
> lifting the hard coded slack zeroing in the hrtimer functions?

This is what the manpage states [1]:

+ Timer slack is not applied to threads that are scheduled under a
+ real-time scheduling policy (see sched_setscheduler(2)).

[1] https://man7.org/linux/man-pages/man2/PR_SET_TIMERSLACK.2const.html

Best regards,
Felix

> 
> Thanks,
> 
>         tglx

-- 
Siemens AG, Technology
Linux Expert Center