Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   bsegall@google.com
To:     =?utf-8?B?546L6LSH?= <yun.wang@linux.alibaba.com>
Cc:     bsegall@google.com, Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mel Gorman <mgorman@suse.de>,
        "open list\:SCHEDULER" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH] sched: fix the nonsense shares when load of cfs_rq is too, small
References: <44fa1cee-08db-e4ab-e5ab-08d6fbd421d7@linux.alibaba.com>
        <20200303195245.GF2596@hirez.programming.kicks-ass.net>
        <xm26o8tc3qkv.fsf@bsegall-linux.svl.corp.google.com>
        <1180c6cd-ff61-2c9f-d689-ffe58f8c5a68@linux.alibaba.com>
Date:   Fri, 06 Mar 2020 11:17:10 -0800
In-Reply-To: <1180c6cd-ff61-2c9f-d689-ffe58f8c5a68@linux.alibaba.com>
 (=?utf-8?B?IueOi+i0hw==?=
        "'s message of "Thu, 5 Mar 2020 09:14:47 +0800")
Message-ID: <xm267dzx47k9.fsf@bsegall-linux.svl.corp.google.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

=E7=8E=8B=E8=B4=87 <yun.wang@linux.alibaba.com> writes:

> On 2020/3/5 =E4=B8=8A=E5=8D=882:47, bsegall@google.com wrote:
> [snip]
>>> Argh, because A->cfs_rq.load.weight is B->se.load.weight which is
>>> B->shares/nr_cpus.
>>>
>>>> While the se of D on root cfs_rq is far more bigger than 2, so it
>>>> wins the battle.
>>>>
>>>> This patch add a check on the zero load and make it as MIN_SHARES
>>>> to fix the nonsense shares, after applied the group C wins as
>>>> expected.
>>>>
>>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
>>>> ---
>>>>  kernel/sched/fair.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>>> index 84594f8aeaf8..53d705f75fa4 100644
>>>> --- a/kernel/sched/fair.c
>>>> +++ b/kernel/sched/fair.c
>>>> @@ -3182,6 +3182,8 @@ static long calc_group_shares(struct cfs_rq *cfs=
_rq)
>>>>  	tg_shares =3D READ_ONCE(tg->shares);
>>>>
>>>>  	load =3D max(scale_load_down(cfs_rq->load.weight), cfs_rq->avg.load_=
avg);
>>>> +	if (!load && cfs_rq->load.weight)
>>>> +		load =3D MIN_SHARES;
>>>>
>>>>  	tg_weight =3D atomic_long_read(&tg->load_avg);
>>>
>>> Yeah, I suppose that'll do. Hurmph, wants a comment though.
>>>
>>> But that has me looking at other users of scale_load_down(), and doesn't
>>> at least update_tg_cfs_load() suffer the same problem?
>>=20
>> I think instead we should probably scale_load_down(tg_shares) and
>> scale_load(load_avg). tg_shares is always a scaled integer, so just
>> moving the source of the scaling in the multiply should do the job.
>>=20
>> ie
>>=20
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index fcc968669aea..6d7a9d72d742 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -3179,9 +3179,9 @@ static long calc_group_shares(struct cfs_rq *cfs_r=
q)
>>         long tg_weight, tg_shares, load, shares;
>>         struct task_group *tg =3D cfs_rq->tg;
>>=20=20
>> -       tg_shares =3D READ_ONCE(tg->shares);
>> +       tg_shares =3D scale_load_down(READ_ONCE(tg->shares));
>>=20=20
>> -       load =3D max(scale_load_down(cfs_rq->load.weight), cfs_rq->avg.l=
oad_avg);
>> +       load =3D max(cfs_rq->load.weight, scale_load(cfs_rq->avg.load_av=
g));
>>=20=20
>>         tg_weight =3D atomic_long_read(&tg->load_avg);
>
> Get the point, but IMHO fix scale_load_down() sounds better, to
> cover all the similar cases, let's first try that way see if it's
> working :-)

Yeah, that might not be a bad idea as well; it's just that doing this
fix would keep you from losing all your precision (and I'd have to think
if that would result in fairness issues like having all the group ses
having the full tg shares, or something like that).