Re: [PATCH v4 1/1] powerpc: enable dynamic preemption

  Hi Shrikanth, Christophe,

 I'm seeing a significant performance regression on ppc64le after this
 patch landed in 6.16, caused by CONFIG_PREEMPT_RCU becoming active
 once HAVE_PREEMPT_DYNAMIC_KEY is selected.

 Benchmark: stress-ng kill stressor (tight kill() syscall loop),
 single thread, POWER10 LPAR (8 vCPUs, 1 core SMT-8).

 Bisected across Fedora ELN kernel builds on ppc64le:

   kernel               CONFIG_PREEMPT_RCU   kill bogo-ops/sec
 ---  6.15-rc6 (eln148)    no                   103,207
   6.16     (eln150)          yes                   70,281    (-32%)
   6.18     (eln154)          yes                   72,552    (-30%)

 For comparison, x86_64 (AMD EPYC 9355P) with the same config change
 shows only a 2.8% regression:

   6.12 x86_64          37,436
   7.2  x86_64           36,392    (-2.8%)

 perf report shows the overhead comes from rcu_read_lock/unlock in the
 SELinux AVC path (check_kill_permission -> security_task_kill ->
 selinux_task_kill -> avc_has_perm -> avc_lookup):

   Function               6.15 (no PREEMPT_RCU)   6.16 (PREEMPT_RCU)
 ---  avc_lookup                          15.23%              24.79%
   __rcu_read_lock                      ~0%                 4.52%
   __rcu_read_unlock                    ~0%                 4.17%
   selinux_task_kill                    6.23%               7.35%
   audit_signal_info*                   0.94%               3.59%

 On x86_64, rcu_read_lock/unlock are cheap thanks to static calls
 (HAVE_PREEMPT_DYNAMIC_CALL). On ppc64le with the KEY-based
 implementation, the weak memory model requires real barriers
 (lwsync/isync) making each RCU read-side critical section
 significantly more expensive.

 This aligns with Christophe's earlier review comment that
 HAVE_PREEMPT_DYNAMIC_CALL should be more performant. Would
 implementing static calls for ppc64 be feasible to close this gap?

 Test details:
 - Machine: IBM POWER10 (pvr 0080 0200), pHyp virtualization
 - stress-ng 0.21.03, gcc 14.3.1, glibc 2.39
 - Tuned profile: virtual-guest
 - SELinux: enforcing (permissive recovers only ~7%)

 Happy to run additional tests if needed.


On Mon, Feb 10, 2025 at 7:46 PM Shrikanth Hegde <[email protected]> wrote:
>
> Once the lazy preemption is supported, it would be desirable to change
> the preemption models at runtime. So add support for dynamic preemption
> using DYNAMIC_KEY.
>
> ::Tested lightly on Power10 LPAR
> Performance numbers indicate that, preempt=none(no dynamic) and
> preempt=none(dynamic) are close.
>
> cat /sys/kernel/debug/sched/preempt
> (none) voluntary full lazy
> perf stat -e probe:__cond_resched -a sleep 1
>  Performance counter stats for 'system wide':
>              1,253      probe:__cond_resched
>
> echo full > /sys/kernel/debug/sched/preempt
> cat /sys/kernel/debug/sched/preempt
> none voluntary (full) lazy
> perf stat -e probe:__cond_resched -a sleep 1
>  Performance counter stats for 'system wide':
>                  0      probe:__cond_resched
>
> Signed-off-by: Shrikanth Hegde <[email protected]>
> ---
>  arch/powerpc/Kconfig               |  1 +
>  arch/powerpc/include/asm/preempt.h | 16 ++++++++++++++++
>  arch/powerpc/kernel/interrupt.c    |  6 +++++-
>  arch/powerpc/lib/vmx-helper.c      |  2 +-
>  4 files changed, 23 insertions(+), 2 deletions(-)
>  create mode 100644 arch/powerpc/include/asm/preempt.h
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 424f188e62d9..364edaea1f88 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -275,6 +275,7 @@ config PPC
>         select HAVE_PERF_EVENTS_NMI             if PPC64
>         select HAVE_PERF_REGS
>         select HAVE_PERF_USER_STACK_DUMP
> +       select HAVE_PREEMPT_DYNAMIC_KEY
>         select HAVE_RETHOOK                     if KPROBES
>         select HAVE_REGS_AND_STACK_ACCESS_API
>         select HAVE_RELIABLE_STACKTRACE
> diff --git a/arch/powerpc/include/asm/preempt.h b/arch/powerpc/include/asm/preempt.h
> new file mode 100644
> index 000000000000..000e2b9681f3
> --- /dev/null
> +++ b/arch/powerpc/include/asm/preempt.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_POWERPC_PREEMPT_H
> +#define __ASM_POWERPC_PREEMPT_H
> +
> +#include <asm-generic/preempt.h>
> +
> +#if defined(CONFIG_PREEMPT_DYNAMIC)
> +#include <linux/jump_label.h>
> +DECLARE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
> +#define need_irq_preemption() \
> +       (static_branch_unlikely(&sk_dynamic_irqentry_exit_cond_resched))
> +#else
> +#define need_irq_preemption()   (IS_ENABLED(CONFIG_PREEMPTION))
> +#endif
> +
> +#endif /* __ASM_POWERPC_PREEMPT_H */
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index 8f4acc55407b..e0c681d0b076 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -25,6 +25,10 @@
>  unsigned long global_dbcr0[NR_CPUS];
>  #endif
>
> +#if defined(CONFIG_PREEMPT_DYNAMIC)
> +DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
> +#endif
> +
>  #ifdef CONFIG_PPC_BOOK3S_64
>  DEFINE_STATIC_KEY_FALSE(interrupt_exit_not_reentrant);
>  static inline bool exit_must_hard_disable(void)
> @@ -396,7 +400,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs)
>                 /* Returning to a kernel context with local irqs enabled. */
>                 WARN_ON_ONCE(!(regs->msr & MSR_EE));
>  again:
> -               if (IS_ENABLED(CONFIG_PREEMPTION)) {
> +               if (need_irq_preemption()) {
>                         /* Return to preemptible kernel context */
>                         if (unlikely(read_thread_flags() & _TIF_NEED_RESCHED)) {
>                                 if (preempt_count() == 0)
> diff --git a/arch/powerpc/lib/vmx-helper.c b/arch/powerpc/lib/vmx-helper.c
> index 58ed6bd613a6..54340912398f 100644
> --- a/arch/powerpc/lib/vmx-helper.c
> +++ b/arch/powerpc/lib/vmx-helper.c
> @@ -45,7 +45,7 @@ int exit_vmx_usercopy(void)
>          * set and we are preemptible. The hack here is to schedule a
>          * decrementer to fire here and reschedule for us if necessary.
>          */
> -       if (IS_ENABLED(CONFIG_PREEMPTION) && need_resched())
> +       if (need_irq_preemption() && need_resched())
>                 set_dec(1);
>         return 0;
>  }
> --
> 2.39.3
>
>


-- 
-Jirka