* 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
[parent not found: <20200424043650.14940-1-hdanton@sina.com>]
* 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
[parent not found: <20200430121301.3460-1-hdanton@sina.com>]
* 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).