Re: Re: [RFC PATCH 1/2] Revert "sched/fair: Make sched-idle CPU selection consistent throughout"

[Abel Wu <wuyun.abel@bytedance.com>]

On 3/19/25 5:17 PM, Vincent Guittot wrote:
> On Thu, 13 Mar 2025 at 08:18, Abel Wu <wuyun.abel@bytedance.com> wrote:
>>
>> Hi Vincent,
>>
>> On 3/12/25 12:58 AM, Vincent Guittot wrote:
>>> On Mon, 10 Mar 2025 at 08:41, Abel Wu <wuyun.abel@bytedance.com> wrote:
>>>>
>>>> This reverts commit 17346452b25b98acfb395d2a82ec2e4ad0cb7a01.
>>>>
>>>> The above commit tried to unify selection policy between idle cpus
>>>> and SCHED_IDLE ones in fast- and slow-path of select_task_rq_fair()
>>>> by treating them equally (although the SCHED_IDLE cpus are turned
>>>> to be given more preference in slowpath). The test results seemed
>>>> solid, but the setup didn't take cgroup hierarchy into account,
>>>> which actually made some of our important services get affected.
>>>>
>>>> The cgroup hierarchy in our production environment looks like below,
>>>> which might be common in modern containerized setup:
>>>>
>>>>                             root
>>>>                           /       \
>>>>                   kubepods        system.slice
>>>>                   /       \\              \
>>>>           guaranteed      besteffort      containerd
>>>>
>>>>           (where 'X=A' means A is SCHED_IDLE cgroup)
>>>>
>>>> The cpu is treated as SCHED_IDLE if only besteffort is running, which
>>>> is given at least equal preference as the idle cpus when deciding where
>>>> to run a newly woken task. But the SCHED_IDLE cpus do not necessarily
>>>> mean they can be preempted soon enough to start serving the wakee, and
>>>
>>> Could you give us more details why the SCHED_IDLE cpu which runs only
>>> besteffort can't be preempted soon enough ?
>>>
>>> because kubepods vs system.slice is not sched_idle when comparing the
>>
>> Yes, exactly. What's worse is that kubepods.weight is the sum of all its
>> children and can easily reach ~3000, while system.weight is default to
>> 100. The weight configuration can be optimized, but it's another topic.
>>
>>> entities ? some maybe the definition of sched_idle_cpu should be fixed
>>> instead
>>
>> Yes, there are at least two ways to fix it:
>>
>>    1) Let sched_idle_cpu() depends on a specific task, just like Josh
>>       mentioned in the reply of 2nd patch. So if sched_idle_cpu(cpu, p)
>>       returns true, then
>>
>>          a) this rq only contains hierarchical idle tasks, and
>>          b) p can preempt current immediately
>>
>>       Please see my reply to Josh to check the details.
> 
> yeah, that should be the solution which covers all cases but at the
> cost of walking cgroup hierarchy which is far from being ideal

Only comparing curr vs wakee doesn't solve the problem. A cpu can be
treated as SCHED_IDLE iff *all* its SCHED_IDLE entities can be preempted
by the wakee.

> 
> Could we change h_nr_idle to only track fully sched idle tasks; I mean
> tasks with a full sched_idle cgroup hierarchy ? so we would be sure to
> preempt those sched idle cpus
> 
> Then, for other case of sched idle task or sched idle group childs of
> a non sched idle group then we don't consider those cpu as sched idle
> cpu

Ahthough this is correct, I think it would be too much since this kind
of setup is rare to the best of my knowledge.

> 
> 
>>
>>    2) Or get rid of sched_idle_cpu() entirely. This needs some careful
>>       rework. Now the users of cfs_rq::h_nr_idle are two parts:
>>
>>          a) select_task_rq, including sched_balance_find_dst_group_cpu()
>>             and select_idle_*(). The former is handled by this series by
>>             simply ignoring sched_idle_cpus, which should be safe since
>>             sched_idle_cpus do not always follow the goal of the slowpath
>>             to find a least loaded cpu to help load balancing. While the
>>             latter is roughly designed as following:
>>
>>             - Must search within target LLC domain, since L3$ miss is
>>               much more costly than L1/L2$
>>             - To use cache more wisely, start searching from the L1/L2$
>>               cache hot cpus like prev/recent_used/..
>>             - Cheers if found an idle cpu that can preempt immediately.
>>               This helps maximize using cpu bandwidth, hence improving
>>               total throughput
>>             - (?)
>>             - Return target anyway, at least it might be cache hot
>>
>>             It could be less optimal if simply remove sched_idle_cpu and
>>             return @target if no idle cpu available, because @target can
>>             be heavy loaded and the cache may not hot any longer when the
>>             wakee finally hit cpu. So in order not to fight with the load
>>             balancing, shall we tiebreak on cpu_load() for the non-idle
>>             cpus?

What do you think to choose a less loaded cpu if no idle ones available?
So the wakee will probably get better served, and also helps load balance.

>>
>>          b) load balance: sched_balance_domains() and dequeue_entities().
>>             We could leave this as-is, but I would propose using h_idle
>>             instead: if the on_cpu task is hierarchically idle when
>>             triggering normal load balancing, then we guess it's a less
>>             loaded cpu and can reduce balance interval. The rationale
>>             behind is that the idle entities usually get very limited
>>             bandwidth if any hierarchically non-idle tasks available.
>>             The heuristics may have false positives which can generally
>>             be divided into 3 cases:
>>
>>                  (The numbers represents hierarchical shares%)
>>
>>                                                     C
>>                     A               B              / \
>>                   /  \\           /  \\           80  20
>>                  99    1         50   50         // \
>>                                                100  (X)
> 
> 
> How the sched_idle group in B can have the same share/weight as the
> not sched idle one in case B ?

It can't, but theoretically several SCHED_IDLE siblings can be summed up
to match a niced SCHED_NORMAL entity.

				    B
				    |
		 ---------------------------------------
		 |     ||      ||      ||      ||      ||
		15	3	3	3	3	3

> 
> 
>>
>>             - Case A) The hierarchical share of h_idle tasks is indeed
>>               small. So in this case they are just get scheduled to take
>>               some breath, and the possibility of false positive is low
>>               enough to be safely ignored.
>>
>>             - Case B) The h_idle & !h_idle tasks equally share bandwidth
>>               which usually means the !h_idle part becomes less loaded
>>               and pulling some load might be preferred.
>>
>>             - Case C) The hierarchical share of h_idle tasks dominates
>>               which usually means their !h_idle parents are allowed to
>>               use a big portion of bandwidth. In this case speedup the
>>               balance is still fine because we could pull some !h_idle
>>               tasks for the most 'important' cgroup.
>>
>>             So the heuristics of using rq->curr's h_idle to judge the need
>>             of pulling (load balancing) seems fine.
>>
>>       And as a result cfs_rq::h_nr_idle can be removed and its maintenance
>>       cost in hotpath can be saved.
>>
>> Which way do you prefer? It would be much appreciated if you can shed some
>> light on this.
>>
>>>
>>> a sched_idle_cpu should be preempted immediately otherwise it's not a
>>> sched idle cpu and the definition is meaningless
>>
>> Agree.
>>
>> Thanks!
>>          Abel
>>