Re: [RFC PATCH v3 03/10] sched: Static key to check paravirt cpu push

[Shrikanth Hegde <sshegde@linux.ibm.com>]

On 9/11/25 7:23 AM, Yury Norov wrote:
> On Wed, Sep 10, 2025 at 11:12:03PM +0530, Shrikanth Hegde wrote:
>> CPUs are marked paravirt when there is contention for underlying
>> physical CPU.
>>
>> The push mechanism and check for paravirt CPUs are in sched tick
>> and wakeup. It should be close to no-op when there is no need for it.
>> Achieve that using static key.
>>
>> Architecture needs to enable this key when it decides there are
>> paravirt CPUs. Disable it when there are no paravirt CPUs.
> 

Hi Yury, Thanks for looking into this series.

> Testing a bit is quite close to a no-op, isn't it? Have you measured
> the performance impact that would advocate the static key? Please
> share some numbers then. I believe I asked you about it on the previous
> round.

The reasons I thought to keep are:

1. In load balance there is cpumask_and which does a loop.
Might be better to avoid it when it is not necessary.

2. Since __cpu_paravirt_mask is going to in one of the memory node in large NUMA systems
(likely on boot cpu node), access to it from other nodes might take time and costly when
it is not in cache. one could say same for static key too. but cpumask can be large when
NR_CPUS=8192 or so.


In most of the cases hackbench,schbench didn't show much difference.

> 
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>>   kernel/sched/core.c  |  1 +
>>   kernel/sched/sched.h | 17 +++++++++++++++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 0f1e36bb5779..b8a84e4691c8 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -10967,4 +10967,5 @@ void sched_enq_and_set_task(struct sched_enq_and_set_ctx *ctx)
>>   #ifdef CONFIG_PARAVIRT
>>   struct cpumask __cpu_paravirt_mask __read_mostly;
>>   EXPORT_SYMBOL(__cpu_paravirt_mask);
>> +DEFINE_STATIC_KEY_FALSE(cpu_paravirt_push_tasks);
>>   #endif
>> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
>> index b5367c514c14..8f9991453d36 100644
>> --- a/kernel/sched/sched.h
>> +++ b/kernel/sched/sched.h
>> @@ -3880,4 +3880,21 @@ void sched_enq_and_set_task(struct sched_enq_and_set_ctx *ctx);
>>   
>>   #include "ext.h"
>>   
>> +#ifdef CONFIG_PARAVIRT
>> +DECLARE_STATIC_KEY_FALSE(cpu_paravirt_push_tasks);
>> +
>> +static inline bool is_cpu_paravirt(int cpu)
>> +{
>> +	if (static_branch_unlikely(&cpu_paravirt_push_tasks))
>> +		return cpu_paravirt(cpu);
>> +
>> +	return false;
>> +}
> 
> So is_cpu_paravirt and cpu_paravirt are exactly the same in terms of
> functionality. If you really believe that static branch benefits the
> performance, it should go straight to the cpu_paravirt().
> 
>> +#else	/* !CONFIG_PARAVIRT */
>> +static inline bool is_cpu_paravirt(int cpu)
>> +{
>> +	return false;
>> +}
>> +#endif	/* !CONFIG_PARAVIRT */
>> +
>>   #endif /* _KERNEL_SCHED_SCHED_H */
>> -- 
>> 2.47.3