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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aMIrgI9J4fuXntRz@yury>
Date: Wed, 10 Sep 2025 21:53:04 -0400
From: Yury Norov <yury.norov@...il.com>
To: Shrikanth Hegde <sshegde@...ux.ibm.com>
Cc: mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com,
	vincent.guittot@...aro.org, tglx@...utronix.de, maddy@...ux.ibm.com,
	linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
	gregkh@...uxfoundation.org, vschneid@...hat.com, iii@...ux.ibm.com,
	huschle@...ux.ibm.com, rostedt@...dmis.org,
	dietmar.eggemann@....com, vineeth@...byteword.org, jgross@...e.com,
	pbonzini@...hat.com, seanjc@...gle.com
Subject: Re: [RFC PATCH v3 03/10] sched: Static key to check paravirt cpu push

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.

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.

> Signed-off-by: Shrikanth Hegde <sshegde@...ux.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