Re: [PATCH 2/3] powerpc: support dynamic preemption

Le 26/11/2024 à 12:15, Shrikanth Hegde a écrit :

On 11/26/24 16:18, Christophe Leroy wrote:

quoted

Hi Christophe, Thanks for taking a look at this.

quoted

Le 25/11/2024 à 05:22, Shrikanth Hegde a écrit :

quoted

Once the lazy preemption is supported, it would be desirable to change
the preemption models at runtime. So this change adds support for dynamic
preemption using DYNAMIC_KEY.

In irq-exit to kernel path, use preempt_model_preemptible for decision.
Other way would be using static key based decision. Keeping it
simpler since key based change didn't show performance improvement.

Tested lightly on Power10 LPAR. Performance numbers indicate that,
preempt=none(no dynamic) and preempt=none(dynamic) are similar.
Only hackbench pipe shows a regression. There is slight overhead of code
check if it is preemptible kernel. hackbench pipe is prone to such
patterns[1]

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

echo lazy > /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

[1]: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F1a973dda-c79e-4d95-935b-&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cf0474c2567834b69dfd908dd0e0bb554%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638682165690258507%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=Gcw6nRSPkp78lGEkG8NX04KWW%2FjCZm0oA%2BTGTjpUZUc%3D&reserved=0 e4b93eb077b8@linux.ibm.com/

Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
---
  arch/powerpc/Kconfig               | 1 +
  arch/powerpc/include/asm/preempt.h | 1 +
  arch/powerpc/kernel/interrupt.c    | 6 +++++-
  arch/powerpc/lib/vmx-helper.c      | 2 +-
  4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6d6bbd93abab..01c58f5258c9 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -270,6 +270,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
index 51f8f3881523..c0a19ff3f78c 100644
--- a/arch/powerpc/include/asm/preempt.h
+++ b/arch/powerpc/include/asm/preempt.h

@@ -84,6 +84,7 @@ extern asmlinkage void preempt_schedule_notrace(void);

  #if defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)
+DECLARE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
  void dynamic_preempt_schedule(void);
  void dynamic_preempt_schedule_notrace(void);
  #define __preempt_schedule()        dynamic_preempt_schedule()

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/ interrupt.c
index 8f4acc55407b..0fb01019d7e0 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c

@@ -38,6 +38,10 @@ static inline bool exit_must_hard_disable(void)

  }
  #endif
+#ifdef CONFIG_PREEMPT_DYNAMIC
+DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
+#endif

Why is that needed at all ? It isn't used.

This is needed else compilation fails.

It has be defined by arch if it doesn't use kernel infra of entry/exit.
So if an arch does enable, CONFIG_HAVE_PREEMPT_DYNAMIC_KEY it has to be define this key has well. The generic sched/core enables this flag.

This was one of the point I was requesting answer for. Either to use preempt_model_preemptible or define macros based on this key. Other archs are doing the later and hence the generic code enables this key.

It can be done in either way. if we do the later way, then this variable will be used as well.

Ah right, I did a grep on sk_dynamic_irqentry_exit_cond_resched but indeed it is triggered by static_key_enable(&sk_dynamic_##f.key)

Christophe