Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <60cbd089-f514-44eb-a629-62556936be35@bytedance.com>
Date:   Thu, 14 Apr 2022 23:38:06 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.8.0
Subject: Re: [RFC v2 2/2] sched/fair: introduce sched-idle balance
Content-Language: en-US
To:     Josh Don <joshdon@google.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Mel Gorman <mgorman@suse.de>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Abel Wu <wuyun.abel@bytedance.com>
References: <20220409135104.3733193-1-wuyun.abel@bytedance.com>
 <20220409135104.3733193-3-wuyun.abel@bytedance.com>
 <CABk29NvE=Fmgo4xqQLfy-K8j0hNS-+ppGdYt37yDUnRJiqjZ5w@mail.gmail.com>
 <801da029-6bbe-2a0c-7de0-afffc3d5de02@bytedance.com>
 <CABk29Nu9QTUKHP3cyx3qpiGcNM-PbaSxCvfhvBvOs++67KeXCw@mail.gmail.com>
From:   Abel Wu <wuyun.abel@bytedance.com>
In-Reply-To: <CABk29Nu9QTUKHP3cyx3qpiGcNM-PbaSxCvfhvBvOs++67KeXCw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/14/22 8:08 AM, Josh Don Wrote:
>>>>    /*
>>>>     * Use locality-friendly rq->overloaded to cache the status of the rq
>>>>     * to minimize the heavy cost on LLC shared data.
>>>> @@ -7837,6 +7867,22 @@ int can_migrate_task(struct task_struct *p, struct lb_env *env)
>>>>           if (kthread_is_per_cpu(p))
>>>>                   return 0;
>>>>
>>>> +       if (unlikely(task_h_idle(p))) {
>>>> +               /*
>>>> +                * Disregard hierarchically idle tasks during sched-idle
>>>> +                * load balancing.
>>>> +                */
>>>> +               if (env->idle == CPU_SCHED_IDLE)
>>>> +                       return 0;
>>>> +       } else if (!static_branch_unlikely(&sched_asym_cpucapacity)) {
>>>> +               /*
>>>> +                * It's not gonna help if stacking non-idle tasks on one
>>>> +                * cpu while leaving some idle.
>>>> +                */
>>>> +               if (cfs_rq_busy(env->src_rq) && !need_pull_cfs_task(env->dst_rq))
>>>> +                       return 0;
>>>
>>> These checks don't involve the task at all, so this kind of check
>>> should be pushed into the more general load balance function. But, I'm
>>> not totally clear on the motivation here. If we have cpu A with 1
>>> non-idle task and 100 idle tasks, and cpu B with 1 non-idle task, we
>>> should definitely try to load balance some of the idle tasks from A to
>>> B. idle tasks _do_ get time to run (although little), and this can add
>>> up and cause antagonism to the non-idle task if there are a lot of
>>> idle threads.
>>
>> CPU_SCHED_IDLE means triggered by sched_idle_balance() in which pulls
>> a non-idle task for the unoccupied cpu from the overloaded ones, so
>> idle tasks are not the target and should be skipped.
>>
>> The second part is: if we have cpu A with 1 non-idle task and 100 idle
>> tasks, and B with >=1 non-idle task, we don't migrate the last non-idle
>> task on A to B.
> 
> It could be possible that we do want to migrate the last non-idle task
> from A to B, if the weight sum of idle tasks on A is very high (easily
> possible with affinity restrictions). So I think we should leave
> regular load balance alone here if it really wants to move the
> non-idle task, and wrap this entire block in an if (env->idle ==
> CPU_SCHED_IDLE).

Makes sense. I will fix it in next version.

Thanks & BR,
Abel