Re: [PATCH] sched/core: sched_getattr returning consistent sched_priority

[Alessio Balsini]

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.