Re: [PATCH 1/2] sched/fair: Add EAS checks before updating overutilized

[Shrikanth Hegde]

On 2/27/24 10:15 PM, Chen Yu wrote:

> On 2024-02-23 at 20:37:06 +0530, Shrikanth Hegde wrote:
>> Overutilized field of root domain is only used for EAS(energy aware scheduler)
>> to decide whether to do regular load balance or EAS aware load balance. It
>> is not used if EAS not possible.
>>
>> Currently enqueue_task_fair and task_tick_fair accesses, sometime updates
>> this field. In update_sd_lb_stats it is updated often.
>> Which causes cache contention due to load/store tearing and burns
>> a lot of cycles.
> 
> Looks like a typical cache false sharing: CPU1 updates the rd->overutilized,
> which invalid the cache line when CPU2 access adjacent rd->overload.
> This changes looks good to me, just some minor questions:

Thanks for taking a look and reviewing it. 

> 
>> Hence add EAS check before updating this field.
>> EAS check is optimized at compile time or it is static branch.
>> Hence it shouldn't cost much.
>>
>> With the patch, both enqueue_task_fair and newidle_balance don't show
>> up as hot routines in perf profile.
>>
>> 6.8-rc4:
>> 7.18%  swapper          [kernel.vmlinux]              [k] enqueue_task_fair
>> 6.78%  s                [kernel.vmlinux]              [k] newidle_balance
>> +patch:
>> 0.14%  swapper          [kernel.vmlinux]              [k] enqueue_task_fair
>> 0.00%  swapper          [kernel.vmlinux]              [k] newidle_balance
>>
>> While here, Fix updating overutilized as either SG_OVERUTILIZED or 0
>> instead. Current code can make it 0, 1 or 2. This shouldn't alter the
>> functionality.
> 
> Just wonder where 1 comes from? In current code we either write SG_OVERUTILIZED
> or sg_status & SG_OVERUTILIZED.

Thanks for catching this, Silly mistake. 
Because of if conditions around I wrongly thought it would be 1. 

I will correct that and send a next version soon.

> 
>>
>> Fixes: 2802bf3cd936 ("sched/fair: Add over-utilization/tipping point indicator")
>> Signed-off-by: Shrikanth Hegde <sshegde@xxxxxxxxxxxxx>
>> ---
>>  kernel/sched/fair.c | 36 +++++++++++++++++++++++++-----------
>>  1 file changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 8e30e2bb77a0..9529d9ef2c5b 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -6670,15 +6670,30 @@ static inline bool cpu_overutilized(int cpu)
>>  	return !util_fits_cpu(cpu_util_cfs(cpu), rq_util_min, rq_util_max, cpu);
>>  }
>>
>> -static inline void update_overutilized_status(struct rq *rq)
>> +static inline void update_rd_overutilized_status(struct root_domain *rd,
>> +						 int status)
>>  {
>> -	if (!READ_ONCE(rq->rd->overutilized) && cpu_overutilized(rq->cpu)) {
>> -		WRITE_ONCE(rq->rd->overutilized, SG_OVERUTILIZED);
>> -		trace_sched_overutilized_tp(rq->rd, SG_OVERUTILIZED);
>> +	if (sched_energy_enabled()) {
>> +		WRITE_ONCE(rd->overutilized, status);
>> +		trace_sched_overutilized_tp(rd, !!status);
> 
> Is this !!status intentional? The original one is SG_OVERUTILIZED = 2,
> now it is either 0 or 1.
> 

Yes. this is intentional. To convert into to bool.
The tracepoint hook currently defines the second argument as bool.

include/trace/events/sched.h
DECLARE_TRACE(sched_overutilized_tp,                                               
        TP_PROTO(struct root_domain *rd, bool overutilized),                       
        TP_ARGS(rd, overutilized));  

> thanks,
> Chenyu