* Re: [PATCH 1/4] sched: set p->prio reguardless of p->mm
[not found] ` <20200424043041.15084-1-hdanton@sina.com>
@ 2020-04-24 13:55 ` Steven Rostedt
0 siblings, 0 replies; 5+ messages in thread
From: Steven Rostedt @ 2020-04-24 13:55 UTC (permalink / raw)
To: Hillf Danton
Cc: Peter Zijlstra, Ingo Molnar, lkml, Mike Galbraith, Mel Gorman,
Vincent Guittot, Phil Auld, Valentin Schneider, Dietmar Eggemann,
Mark Rutland
On Fri, 24 Apr 2020 12:30:41 +0800
Hillf Danton <hdanton@sina.com> wrote:
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4795,14 +4795,6 @@ recheck:
> if (attr->sched_flags & ~(SCHED_FLAG_ALL | SCHED_FLAG_SUGOV))
> return -EINVAL;
>
> - /*
> - * Valid priorities for SCHED_FIFO and SCHED_RR are
> - * 1..MAX_USER_RT_PRIO-1, valid priority for SCHED_NORMAL,
> - * SCHED_BATCH and SCHED_IDLE is 0.
> - */
> - if ((p->mm && attr->sched_priority > MAX_USER_RT_PRIO-1) ||
> - (!p->mm && attr->sched_priority > MAX_RT_PRIO-1))
> - return -EINVAL;
So if someone passes in sched_priority > MAX_RT_PRIO-1, where does it get
checked?
-- Steve
> if ((dl_policy(policy) && !__checkparam_dl(attr)) ||
> (rt_policy(policy) != (attr->sched_priority != 0)))
> return -EINVAL;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] sched: set new prio after checking schedule policy
[not found] ` <20200424043650.14940-1-hdanton@sina.com>
@ 2020-04-28 16:32 ` Valentin Schneider
[not found] ` <20200430121301.3460-1-hdanton@sina.com>
1 sibling, 0 replies; 5+ messages in thread
From: Valentin Schneider @ 2020-04-28 16:32 UTC (permalink / raw)
To: Hillf Danton
Cc: Peter Zijlstra, Ingo Molnar, lkml, Mike Galbraith,
Steven Rostedt, Mel Gorman, Vincent Guittot, Phil Auld,
Dietmar Eggemann, Mark Rutland
On 24/04/20 05:36, Hillf Danton wrote:
> Set newprio the same way as normal_prio() does after checking schedule
> policy and to MAX_PRIO -1 by default.
>
> What is also added is boundary checks for RT and fair priorities.
>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Phil Auld <pauld@redhat.com>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Hillf Danton <hdanton@sina.com>
> ---
>
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4768,8 +4768,7 @@ static int __sched_setscheduler(struct t
> const struct sched_attr *attr,
> bool user, bool pi)
> {
> - int newprio = dl_policy(attr->sched_policy) ? MAX_DL_PRIO - 1 :
> - MAX_RT_PRIO - 1 - attr->sched_priority;
> + int newprio;
> int retval, oldprio, oldpolicy = -1, queued, running;
> int new_effective_prio, policy = attr->sched_policy;
> const struct sched_class *prev_class;
> @@ -4800,6 +4799,26 @@ recheck:
> return -EINVAL;
>
> /*
> + * compute newprio after checking policy, see normal_prio();
> + * it's used in pi boost below
> + */
> + if (dl_policy(policy)) {
> + newprio = MAX_DL_PRIO - 1;
> + }
> + else if (rt_policy(policy)) {
> + if (attr->sched_priority > MAX_RT_PRIO - 1)
> + return -EINVAL;
> + newprio = MAX_RT_PRIO - 1 - attr->sched_priority;
> + }
> + else if (fair_policy(policy)) {
> + if (attr->sched_nice < MIN_NICE ||
> + attr->sched_nice > MAX_NICE)
> + return -EINVAL;
We can't hit this with the syscall route, since we (silently) clamp those
values in sched_copy_attr(). setpriority() does the same. There's this
comment in sched_copy_attr() that asks whether we should clamp or return an
error; seems like the current consensus is on clamping, but then we might
want to get rid of that comment :)
> + newprio = NICE_TO_PRIO(attr->sched_nice);
This is new, however AFAICT it doesn't change anything for CFS (or about to
be) tasks since what matters is calling check_class_changed() further
down.
> + } else
> + newprio = MAX_PRIO - 1;
> +
> + /*
> * Allow unprivileged RT tasks to decrease priority:
> */
> if (user && !capable(CAP_SYS_NICE)) {
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] sched: set new prio after checking schedule policy
[not found] ` <20200430121301.3460-1-hdanton@sina.com>
@ 2020-04-30 14:06 ` Dietmar Eggemann
2020-04-30 14:18 ` Valentin Schneider
0 siblings, 1 reply; 5+ messages in thread
From: Dietmar Eggemann @ 2020-04-30 14:06 UTC (permalink / raw)
To: Hillf Danton, Valentin Schneider
Cc: Peter Zijlstra, Ingo Molnar, lkml, Mike Galbraith,
Steven Rostedt, Mel Gorman, Vincent Guittot, Phil Auld,
Mark Rutland
On 30/04/2020 14:13, Hillf Danton wrote:
>
> On Tue, 28 Apr 2020 17:32:45 Valentin Schneider wrote:
>>
>>> + else if (fair_policy(policy)) {
>>> + if (attr->sched_nice < MIN_NICE ||
>>> + attr->sched_nice > MAX_NICE)
>>> + return -EINVAL;
>>
>> We can't hit this with the syscall route, since we (silently) clamp those
>> values in sched_copy_attr(). setpriority() does the same. There's this
>> comment in sched_copy_attr() that asks whether we should clamp or return an
>> error; seems like the current consensus is on clamping, but then we might
>> want to get rid of that comment :)
>>
> Yes it's quite likely for me to miss the cases covered by that clamp;
> otherwise what is added does not break that consensus.
>
>>> + newprio = NICE_TO_PRIO(attr->sched_nice);
>>
>> This is new, however AFAICT it doesn't change anything for CFS (or about to
>> be) tasks since what matters is calling check_class_changed() further down.
>
> Yes it's only used by rt_effective_prio().
>
Looks like changing a SCHED_NORMAL to a SCHED_BATCH task will create a different
queue_flags value.
# chrt -p $$
pid 2803's current scheduling policy: SCHED_OTHER
pid 2803's current scheduling priority: 0
# chrt -b -p 0 $$
...
[bash 2803] policy=3 oldprio=120 newprio=[99->120] new_effective_prio=[99->120] queue_flags=[0xe->0xa]
[bash 2803] queued=0 running=0
...
But since in this example 'queued=0' it has no further effect here.
Why is SCHED_NORMAL/SCHED_BATCH (fair_policy()) now treated differently than SCHED_IDLE?
# chrt -i -p 0 $$
...
[bash 2803] policy=5 newprio=99 oldprio=120 new_effective_prio=99 queue_flags=0xe
[bash 2803] queued=0 running=0
...
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] sched: set new prio after checking schedule policy
2020-04-30 14:06 ` Dietmar Eggemann
@ 2020-04-30 14:18 ` Valentin Schneider
2020-04-30 15:13 ` Valentin Schneider
0 siblings, 1 reply; 5+ messages in thread
From: Valentin Schneider @ 2020-04-30 14:18 UTC (permalink / raw)
To: Dietmar Eggemann
Cc: Hillf Danton, Peter Zijlstra, Ingo Molnar, lkml, Mike Galbraith,
Steven Rostedt, Mel Gorman, Vincent Guittot, Phil Auld,
Mark Rutland
On 30/04/20 15:06, Dietmar Eggemann wrote:
>>>> + newprio = NICE_TO_PRIO(attr->sched_nice);
>>>
>>> This is new, however AFAICT it doesn't change anything for CFS (or about to
>>> be) tasks since what matters is calling check_class_changed() further down.
>>
>> Yes it's only used by rt_effective_prio().
>>
>
> Looks like changing a SCHED_NORMAL to a SCHED_BATCH task will create a different
> queue_flags value.
>
> # chrt -p $$
> pid 2803's current scheduling policy: SCHED_OTHER
> pid 2803's current scheduling priority: 0
>
> # chrt -b -p 0 $$
>
> ...
> [bash 2803] policy=3 oldprio=120 newprio=[99->120] new_effective_prio=[99->120] queue_flags=[0xe->0xa]
> [bash 2803] queued=0 running=0
> ...
>
> But since in this example 'queued=0' it has no further effect here.
>
> Why is SCHED_NORMAL/SCHED_BATCH (fair_policy()) now treated differently than SCHED_IDLE?
>
> # chrt -i -p 0 $$
>
> ...
> [bash 2803] policy=5 newprio=99 oldprio=120 new_effective_prio=99 queue_flags=0xe
> [bash 2803] queued=0 running=0
> ...
Good catch; I suppose we'll want to special case SCHED_IDLE (IIRC should
map to nice 20).
As you pointed out, right now the newprio computation for CFS tasks is
kinda bonkers, so it seems we'll almost always clear DEQUEUE_MOVE from
queue_flags for them.
For CFS, not having DEQUEUE_MOVE here would lead to not calling
update_min_vruntime() on the dequeue. I'm not sure how much it matters in
this one case - I don't expect sched_setscheduler() calls to be *too*
frequent, and that oughta be fixed by the next entity_tick()) - but that is
an actual change.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] sched: set new prio after checking schedule policy
2020-04-30 14:18 ` Valentin Schneider
@ 2020-04-30 15:13 ` Valentin Schneider
0 siblings, 0 replies; 5+ messages in thread
From: Valentin Schneider @ 2020-04-30 15:13 UTC (permalink / raw)
To: Dietmar Eggemann
Cc: Hillf Danton, Peter Zijlstra, Ingo Molnar, lkml, Mike Galbraith,
Steven Rostedt, Mel Gorman, Vincent Guittot, Phil Auld,
Mark Rutland
On 30/04/20 15:18, Valentin Schneider wrote:
> On 30/04/20 15:06, Dietmar Eggemann wrote:
>>>>> + newprio = NICE_TO_PRIO(attr->sched_nice);
>>>>
>>>> This is new, however AFAICT it doesn't change anything for CFS (or about to
>>>> be) tasks since what matters is calling check_class_changed() further down.
>>>
>>> Yes it's only used by rt_effective_prio().
>>>
>>
>> Looks like changing a SCHED_NORMAL to a SCHED_BATCH task will create a different
>> queue_flags value.
>>
>> # chrt -p $$
>> pid 2803's current scheduling policy: SCHED_OTHER
>> pid 2803's current scheduling priority: 0
>>
>> # chrt -b -p 0 $$
>>
>> ...
>> [bash 2803] policy=3 oldprio=120 newprio=[99->120] new_effective_prio=[99->120] queue_flags=[0xe->0xa]
>> [bash 2803] queued=0 running=0
>> ...
>>
>> But since in this example 'queued=0' it has no further effect here.
>>
>> Why is SCHED_NORMAL/SCHED_BATCH (fair_policy()) now treated differently than SCHED_IDLE?
>>
>> # chrt -i -p 0 $$
>>
>> ...
>> [bash 2803] policy=5 newprio=99 oldprio=120 new_effective_prio=99 queue_flags=0xe
>> [bash 2803] queued=0 running=0
>> ...
>
>
> Good catch; I suppose we'll want to special case SCHED_IDLE (IIRC should
> map to nice 20).
>
> As you pointed out, right now the newprio computation for CFS tasks is
> kinda bonkers, so it seems we'll almost always clear DEQUEUE_MOVE from
> queue_flags for them.
>
Of course I misread that, it's the other way around: since newprio is
always 99 for SCHED_OTHER/BATCH/IDLE tasks, we'll never have
new_effective_prio == oldprio (unless pi involves a FIFO 99 task), thus
will never clear DEQUEUE_MOVE.
> For CFS, not having DEQUEUE_MOVE here would lead to not calling
> update_min_vruntime() on the dequeue. I'm not sure how much it matters in
> this one case - I don't expect sched_setscheduler() calls to be *too*
> frequent, and that oughta be fixed by the next entity_tick()) - but that is
> an actual change.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-04-30 15:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200424041832.11364-1-hdanton@sina.com>
[not found] ` <20200424043041.15084-1-hdanton@sina.com>
2020-04-24 13:55 ` [PATCH 1/4] sched: set p->prio reguardless of p->mm Steven Rostedt
[not found] ` <20200424043650.14940-1-hdanton@sina.com>
2020-04-28 16:32 ` [PATCH 2/4] sched: set new prio after checking schedule policy Valentin Schneider
[not found] ` <20200430121301.3460-1-hdanton@sina.com>
2020-04-30 14:06 ` Dietmar Eggemann
2020-04-30 14:18 ` Valentin Schneider
2020-04-30 15:13 ` Valentin Schneider
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).