Re: [PATCH] sched/uclamp: Let each sched_class handle uclamp

[Hongyan Xia <hongyan.xia2@arm.com>]

On 10/03/2025 12:24, Dietmar Eggemann wrote:
> On 10/03/2025 12:56, Hongyan Xia wrote:
>> On 10/03/2025 11:22, Dietmar Eggemann wrote:
>>> On 10/03/2025 12:03, Xuewen Yan wrote:
>>>> Hi Dietmar,
>>>>
>>>> On Mon, Mar 10, 2025 at 6:53 PM Dietmar Eggemann
>>>> <dietmar.eggemann@arm.com> wrote:
>>>>>
>>>>> On 10/03/2025 03:41, Xuewen Yan wrote:
>>>>>> On Sat, Mar 8, 2025 at 2:32 AM Dietmar Eggemann
>>>>>> <dietmar.eggemann@arm.com> wrote:
>>>>>>>
>>>>>>> On 06/03/2025 13:01, Xuewen Yan wrote:
>>>>>>>> On Thu, Mar 6, 2025 at 2:24 AM Dietmar Eggemann
>>>>>>>> <dietmar.eggemann@arm.com> wrote:
>>>>>>>>>
>>>>>>>>> On 27/02/2025 14:54, Hongyan Xia wrote:
> 
> [...]
> 
>>>> I submitted a patch similar to yours before:
>>>>
>>>> https://lore.kernel.org/all/CAB8ipk_AvaOWp9QhmnFDdbFSWcKLhCH151=no6kRO2z+pSJfyQ@mail.gmail.com/
>>>>
>>>> And Hongyan fears that as more complexity goes into each sched_class
>>>> like delayed dequeue,
>>>> so it's better to just let the sched_class handle how uclamp is
>>>> enqueued and dequeued within itself rather than leaking into core.c.
>>>
>>> Ah, OK. Your patch didn't have 'sched' in the subject so I didn't see it
>>> immediately.
>>>
>>> I would prefer that uclamp stays in core.c. ENQUEUE_DELAYED among all
>>> the other flags is already used there (ttwu_runnable()).
>>>
>>> task_struct contains  sched_{,rt_,dl_}entity}. We just have to be
>>> careful when switching policies.
>>
>> I lean towards letting each class handle uclamp. We've seen the trouble
>> with delayed dequeue. Just like the if condition we have for util_est,
>> if uclamp is in each class then we can re-use the condition easily,
>> otherwise we need to carefully synchronize the enqueue/dequeue between
>> core.c and the sub class.
>>
>> Also I think so far we are assuming delayed dequeue is the only trouble
>> maker. If RT and sched_ext have their own corner cases (I think maybe
>> sched_ext is likely because it may eventually want the ext scheduler to
>> be able to decide on uclamp itself) then the uclamp inc/dec in core.c
>> need to cater for that as well. Once a task is in a class, the variables
>> in another class may be in an undefined state, so checking corner cases
>> for all the sub-classes in a centralized place like core.c may not even
>> be easy to get right.
> 
> I do understand your concern with sched_ext but I still prefer the less
> invasive change to get uclamp & util_est aligned for fair.c aligned.
> 
> AFAICS, related to policy changes, we have a
> SCHED_WARN_ON(p->se.sched_delayed) in switched_to_fair() so far.

I'm okay with staying in core.c, but care is needed to make sure uclamp 
inc and dec are balanced. We have been bitten by the unbalanced uclamp 
and util_est during delayed dequeue fixes and we finally made sure 
util_est is balanced (at least so far it seems). Reusing the same logic 
and moving uclamp into fair.c makes sure we don't get bitten again and 
is one reason why I prefer moving into each class.