Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 2/4] sched: set new prio after checking schedule policy
To:     Hillf Danton <hdanton@sina.com>,
        Valentin Schneider <valentin.schneider@arm.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Mike Galbraith <efault@gmx.de>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mel Gorman <mgorman@techsingularity.net>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Phil Auld <pauld@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>
References: <20200424041832.11364-1-hdanton@sina.com>
 <20200424043650.14940-1-hdanton@sina.com>
 <20200430121301.3460-1-hdanton@sina.com>
From:   Dietmar Eggemann <dietmar.eggemann@arm.com>
Message-ID: <da5bf72d-1d50-5c5c-3bdb-113ed555dd10@arm.com>
Date:   Thu, 30 Apr 2020 16:06:15 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200430121301.3460-1-hdanton@sina.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
...