Re: [RFC PATCH 3/5] sched/fair: Update overloaded mask in presence of pushable task

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Shrikanth,

On 4/21/2025 10:50 AM, Shrikanth Hegde wrote:
> 
> 
> On 4/9/25 16:45, K Prateek Nayak wrote:
> 
> Hi Prateek. Feel free to cc me in the future versions.


Will do! Thank you for taking a look at the series.

> This seems interesting way if it can get us rid of newidle balance overheads.
> 
>> In presence of pushable tasks on the CPU, set it on the newly introduced
>> "overloaded+mask" in sched_domain_shared struct. This will be used by
>> the newidle balance to limit the scanning to these overloaded CPUs since
>> they contain tasks that could be run on the newly idle target.
>>
>> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
>> ---
>>   kernel/sched/fair.c | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 98d3ed2078cd..834fcdd15cac 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -8559,6 +8559,24 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
>>       return target;
>>   }
>> +static inline void update_overloaded_mask(int cpu, bool contains_pushable)
>> +{
>> +    struct sched_domain_shared *sd_share = rcu_dereference(per_cpu(sd_llc_shared, cpu));
>> +    cpumask_var_t overloaded_mask;
>> +
>> +    if (!sd_share)
>> +        return;
>> +
>> +    overloaded_mask = sd_share->overloaded_mask;
>> +    if (!overloaded_mask)
>> +        return;
>> +
>> +    if (contains_pushable)
>> +        cpumask_set_cpu(cpu, overloaded_mask);
>> +    else
>> +        cpumask_clear_cpu(cpu, overloaded_mask);
>> +}
>> +
> 
> I was getting below error when compiling. Noticed that overloaded_mask is a local update and it wouldn't
> update the actual overloaded_mask.

Interesting! Question: Do you have "CONFIG_CPUMASK_OFFSTACK" enabled in
your config? (me makes a note to test this too in the next iteration)
Looking at the arch specific Kconfigs, there is a slight difference in
how this is toggled on x86 vs powerpc and I'm wondering if that is why
I haven't seen this warning in my testing.

> 
> Compilation Error:
> kernel/sched/fair.c: In function ‘update_overloaded_mask’:
> kernel/sched/fair.c:8570:25: error: assignment to expression with array type
>   8570 |         overloaded_mask = sd_share->overloaded_mask;
>        |                         ^
> kernel/sched/fair.c:8571:13: warning: the address of ‘overloaded_mask’ will always evaluate as ‘true’ [-Waddress]
>   8571 |         if (!overloaded_mask)
> 
> 
> 
> Made the below change. Also you would need rcu protection for sd_share i think.

You are right! Thank you for the pointers and spotting my oversight.
Aaron too pointed some shortcomings here. I'll make sure to test
these bits more next time (LOCKDEP, hotplug, and
!CONFIG_CPUMASK_OFFSTACK)

-- 
Thanks and Regards,
Prateek

> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index bca3db5d0ac0..818d4769ec55 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8562,19 +8562,18 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
>   static inline void update_overloaded_mask(int cpu, bool contains_pushable)
>   {
>          struct sched_domain_shared *sd_share = rcu_dereference(per_cpu(sd_llc_shared, cpu));
> -       cpumask_var_t overloaded_mask;
> 
>          if (!sd_share)
>                  return;
> 
> -       overloaded_mask = sd_share->overloaded_mask;
> -       if (!overloaded_mask)
> +       if (!sd_share->overloaded_mask)
>                  return;
> 
> +       guard(rcu)();
>          if (contains_pushable)
> -               cpumask_set_cpu(cpu, overloaded_mask);
> +               cpumask_set_cpu(cpu, sd_share->overloaded_mask);
>          else
> -               cpumask_clear_cpu(cpu, overloaded_mask);
> +               cpumask_clear_cpu(cpu, sd_share->overloaded_mask);
>   }
> 
>>   static inline bool fair_push_task(struct task_struct *p)
>>   {
>>       if (!task_on_rq_queued(p))
>> @@ -8606,11 +8624,17 @@ static inline void fair_queue_pushable_tasks(struct rq *rq)
>>   static void fair_remove_pushable_task(struct rq *rq, struct task_struct *p)
>>   {
>>       plist_del(&p->pushable_tasks, &rq->cfs.pushable_tasks);
>> +
>> +    if (!has_pushable_tasks(rq))
>> +        update_overloaded_mask(rq->cpu, false);
>>   }
>>   static void fair_add_pushable_task(struct rq *rq, struct task_struct *p)
>>   {
>>       if (fair_push_task(p)) {
>> +        if (!has_pushable_tasks(rq))
>> +            update_overloaded_mask(rq->cpu, true);
>> +
>>           plist_del(&p->pushable_tasks, &rq->cfs.pushable_tasks);
>>           plist_node_init(&p->pushable_tasks, p->prio);
>>           plist_add(&p->pushable_tasks, &rq->cfs.pushable_tasks);
>