Re: [PATCH v4 3/5] sched/fair: Switch to task based throttle model

[Benjamin Segall <bsegall@google.com>]

K Prateek Nayak <kprateek.nayak@amd.com> writes:

> Hello Ben,
>
> On 9/4/2025 2:16 AM, Benjamin Segall wrote:
>> K Prateek Nayak <kprateek.nayak@amd.com> writes:
>> 
>>> Hello Peter,
>>>
>>> On 9/3/2025 8:21 PM, Peter Zijlstra wrote:
>>>>>  static bool dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>>>>>  {
>>>>> +	if (task_is_throttled(p)) {
>>>>> +		dequeue_throttled_task(p, flags);
>>>>> +		return true;
>>>>> +	}
>>>>> +
>>>>>  	if (!p->se.sched_delayed)
>>>>>  		util_est_dequeue(&rq->cfs, p);
>>>>>  
>>>>
>>>> OK, so this makes it so that either a task is fully enqueued (all
>>>> cfs_rq's) or full not. A group cfs_rq is only marked throttled when all
>>>> its tasks are gone, and unthrottled when a task gets added. Right?
>>>
>>> cfs_rq (and the hierarchy below) is marked throttled when the quota
>>> has elapsed. Tasks on the throttled hierarchies will dequeue
>>> themselves completely via task work added during pick. When the last
>>> task leaves on a cfs_rq of throttled hierarchy, PELT is frozen for
>>> that cfs_rq.
>>>
>>> When a new task is added on the hierarchy, the PELT is unfrozen and
>>> the task becomes runnable. The cfs_rq and the hierarchy is still
>>> marked throttled.
>>>
>>> Unthrottling of hierarchy is only done at distribution.
>>>
>>>>
>>>> But propagate_entity_cfs_rq() is still doing the old thing, and has a
>>>> if (cfs_rq_throttled(cfs_rq)) break; inside the for_each_sched_entity()
>>>> iteration.
>>>>
>>>> This seems somewhat inconsistent; or am I missing something ? 
>>>
>>> Probably an oversight. But before that, what was the reason to have
>>> stopped this propagation at throttled_cfs_rq() before the changes?
>>>
>> 
>> Yeah, this was one of the things I was (slowly) looking at - with this
>> series we currently still abort in:
>> 
>> 1) update_cfs_group
>> 2) dequeue_entities's set_next_buddy
>> 3) check_preempt_fair
>> 4) yield_to
>> 5) propagate_entity_cfs_rq
>> 
>> In the old design on throttle immediately remove the entire cfs_rq,
>> freeze time for it, and stop adjusting load. In the new design we still
>> pick from it, so we definitely don't want to stop time (and don't). I'm
>> guessing we probably also want to now adjust load for it, but it is
>> arguable - since all the cfs_rqs for the tg are likely to throttle at the
>> same time, so we might not want to mess with the shares distribution,
>> since when unthrottle comes around the most likely correct distribution
>> is the distribution we had at the time of throttle.
>
> So we were having a discussion in the parallel thread here
> https://lore.kernel.org/lkml/20250903101102.GB42@bytedance/ on whether
> we should allow tasks on throttled hierarchies to be load balanced or
> not.
>
> If we do want them to be migrated, I think we need update_cfs_group()
> cause otherwise we might pick off most task from the hierarchy but
> the sched entity of the cfs_rq will still be contributing the same
> amount of weight to the root making the CPU look busier than it
> actually is.
>
> The alternate is to ensure we don't migrate the tasks on throttled
> hierarchies and let them exit to userspace in-place on the same CPU
> but that too is less than ideal.
>

Yeah - if we don't update group se load then we shouldn't load balance
throttled-hierarchy because the amount of root load migrated in the
moment is always 0. Once we do all of that properly we should be fine to
migrate in/out of a throttled hierarchy.

Much like wakeup there's an argument for not migrating into a throttled
hierarchy, at least from an unthrottled one, where there's presumably a
high likelyhood of the thread just being preempted in userspace. (And my
gut feeling is that this case is probably even more common than wakeup,
that need_rescheds land in (return to) userspace more often than wakeups
go straight to userspace with no significant kernel work)