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] [day] [month] [year] [list]
Date:   Wed, 20 Dec 2017 12:32:47 +0000
From:   Alessio Balsini <alessio.balsini@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Tommaso Cucinotta <tommaso.cucinotta@...tannapisa.it>,
        Luca Abeni <luca.abeni@...tannapisa.it>
Subject: Re: [PATCH] sched/core: sched_getattr returning consistent
 sched_priority

Hi,

On 19/12/17 16:16, Peter Zijlstra wrote:
[...]
>> This inaccuracy was introduced in 06a76fe08d4d, that moved the function
>> from core.c to deadline.c. Before that, it was making more sense to
>> access sched_priority, either if the function name __getparam_dl() was
>> misleading.
>
> OK, I'm dense, what?

Too verbose? :)
What about:

---8<---
sched/core: sched_getattr cleanup for sched_priority

Always initialise the sched_priority field of the sched_attr struct
returned by sched_getattr() with the rt_priority of the process.
This makes the code even more readable and saves a compare+jump.
The consistency of the rt_priority value is guaranteed by
__sched_setscheduler() that requires:
  - 1 <= sched_priority <= 99 for RT tasks, and
  - sched_priority == 0 otherwise.

Remove the sched_priority returned by __getparam_dl() since is not in the
scope of the DL class and does not affect the behaviour of the system.
---8<---

?

>
>> [...]
>> -    attr->sched_priority = p->rt_priority;
>>      attr->sched_runtime = dl_se->dl_runtime;
>>      attr->sched_deadline = dl_se->dl_deadline;
>>      attr->sched_period = dl_se->dl_period;
>
> That seems sane.

Good.

>>      attr.sched_policy = p->policy;
>>      if (p->sched_reset_on_fork)
>>              attr.sched_flags |= SCHED_FLAG_RESET_ON_FORK;
>> +    attr.sched_priority = p->rt_priority;
>>      if (task_has_dl_policy(p))
>>              __getparam_dl(p, &attr);
>> -    else if (task_has_rt_policy(p))
>> -            attr.sched_priority = p->rt_priority;
>> -    else
>> +    else if (task_has_fair_policy(p))
>>              attr.sched_nice = task_nice(p);
>>
>>      rcu_read_unlock();
>
> This is confusing, why unconditionally assign? We initialize to 0, and
> if it must be 0 for !RT, then we should only assign when rt.
>

Right! But as I highlighted in the new changelog, this saves a
compare+jump and, since attr.sched_priority is in the stack, that data
assignment should not cause any relevant cache-related performance loss.

For the same reason, IMHO this cleanup could also be extended to the
sched_nice field:

---8<---
@@ -4606,12 +4606,10 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid,
struct sched_attr __user *, uattr,
         attr.sched_policy = p->policy;
         if (p->sched_reset_on_fork)
                 attr.sched_flags |= SCHED_FLAG_RESET_ON_FORK;
+       attr.sched_nice = task_nice(p);
+       attr.sched_priority = p->rt_priority;
         if (task_has_dl_policy(p))
                 __getparam_dl(p, &attr);
-       else if (task_has_rt_policy(p))
-               attr.sched_priority = p->rt_priority;
-       else
-               attr.sched_nice = task_nice(p);

         rcu_read_unlock();
---8<---

This should make the code even more readable and causes no problem since
the documentation does not specify sched_nice field values for !FAIR
classes.
I left the task_has_dl_policy(p): __getparam_dl() represents a real
waste of time when !DL.

Thanks,
Alessio
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Powered by blists - more mailing lists