Messages in this thread Patch in this message |  | | From | Alessio Balsini <> | Subject | Re: [PATCH] sched/core: sched_getattr returning consistent sched_priority | Date | Wed, 20 Dec 2017 12:32:47 +0000 |
| |
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.
|  |