linux-kernel - Re: [PATCH 2/4] sched: set new prio after checking schedule policy

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <jhjsgglc8h5.mognet@arm.com>
Date:   Thu, 30 Apr 2020 16:13:26 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Dietmar Eggemann <dietmar.eggemann@....com>
Cc:     Hillf Danton <hdanton@...a.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Mike Galbraith <efault@....de>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Phil Auld <pauld@...hat.com>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH 2/4] sched: set new prio after checking schedule policy


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.