[PATCH v3 0/1] powerpc: Enable dynamic preemption

[PATCH v3 0/1] powerpc: Enable dynamic preemption

[Shrikanth Hegde]

Now that preempt=lazy patches[1] are in powerpc-next tree, sending out the
patch to support dynamic preemption based on DYNAMIC_KEY. 

base: powerpc-next 

Once the arch supports static inline calls, it would be needed to
evaluate to see if that gives better performance. 

v2->v3:
- fixed a build error reported by linux test robot by including jump
  label header. 

v1->v2:
- Instead of copying asm-generic preempt.h content include it in
  arch/asm preempt.h. (Christophe Leroy)
- Merge the patches into one patch (Christophe Leroy)

v1: https://lore.kernel.org/all/20241125042212.1522315-1-sshegde@linux.ibm.com/
v2: https://lore.kernel.org/all/20250102191856.499424-1-sshegde@linux.ibm.com/
[1]: https://lore.kernel.org/all/173572211264.1875638.9927288574435880962.b4-ty@linux.ibm.com/

Shrikanth Hegde (1):
  powerpc: Enable dynamic preemption

 arch/powerpc/Kconfig               |  1 +
 arch/powerpc/include/asm/preempt.h | 12 ++++++++++++
 arch/powerpc/kernel/interrupt.c    |  6 +++++-
 arch/powerpc/kernel/traps.c        |  6 +++++-
 arch/powerpc/lib/vmx-helper.c      |  2 +-
 5 files changed, 24 insertions(+), 3 deletions(-)
 create mode 100644 arch/powerpc/include/asm/preempt.h

-- 
2.39.3

[PATCH v3 1/1] powerpc: Enable dynamic preemption

[Shrikanth Hegde]

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.

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.

Also print the right preemption model in __die.

::Tested lightly on Power10 LPAR
Performance numbers indicate that, preempt=none(no dynamic) and
preempt=none(dynamic) are similar.

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 <sshegde@linux.ibm.com>
---
 arch/powerpc/Kconfig               |  1 +
 arch/powerpc/include/asm/preempt.h | 12 ++++++++++++
 arch/powerpc/kernel/interrupt.c    |  6 +++++-
 arch/powerpc/kernel/traps.c        |  6 +++++-
 arch/powerpc/lib/vmx-helper.c      |  2 +-
 5 files changed, 24 insertions(+), 3 deletions(-)
 create mode 100644 arch/powerpc/include/asm/preempt.h

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index db9f7b2d07bf..b14218344e74 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -272,6 +272,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..eb1ed8295f13
--- /dev/null
+++ b/arch/powerpc/include/asm/preempt.h
@@ -0,0 +1,12 @@
+/* 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) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)
+#include <linux/jump_label.h>
+DECLARE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
+#endif
+
+#endif /* __ASM_POWERPC_PREEMPT_H */
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index 8f4acc55407b..8e2400ba208c 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) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)
+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 (preempt_model_preemptible()) {
 			/* Return to preemptible kernel context */
 			if (unlikely(read_thread_flags() & _TIF_NEED_RESCHED)) {
 				if (preempt_count() == 0)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index edf5cabe5dfd..2556fa8ec019 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -266,7 +266,11 @@ static int __die(const char *str, struct pt_regs *regs, long err)
 	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
 	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
 	       PAGE_SIZE / 1024, get_mmu_str(),
-	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
+	       preempt_model_none()      ? "none" :
+	       preempt_model_voluntary() ? "voluntary" :
+	       preempt_model_full()      ? "full" :
+	       preempt_model_lazy()      ? "lazy" :
+	       "",
 	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
 	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
 	       debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "",
diff --git a/arch/powerpc/lib/vmx-helper.c b/arch/powerpc/lib/vmx-helper.c
index 58ed6bd613a6..7b069c832ce2 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 (preempt_model_preemptible() && need_resched())
 		set_dec(1);
 	return 0;
 }
-- 
2.39.3

Re: [PATCH v3 0/1] powerpc: Enable dynamic preemption

[Shrikanth Hegde]

On 1/6/25 10:49, Shrikanth Hegde wrote:
> Now that preempt=lazy patches[1] are in powerpc-next tree, sending out the
> patch to support dynamic preemption based on DYNAMIC_KEY.
> 
> base: powerpc-next
> 

+ankur, sebastian; sorry for not cc'ing earlier.

> Once the arch supports static inline calls, it would be needed to
> evaluate to see if that gives better performance.
> 
> v2->v3:
> - fixed a build error reported by linux test robot by including jump
>    label header.
> 
> v1->v2:
> - Instead of copying asm-generic preempt.h content include it in
>    arch/asm preempt.h. (Christophe Leroy)
> - Merge the patches into one patch (Christophe Leroy)
> 
> v1: https://lore.kernel.org/all/20241125042212.1522315-1-sshegde@linux.ibm.com/
> v2: https://lore.kernel.org/all/20250102191856.499424-1-sshegde@linux.ibm.com/
> [1]: https://lore.kernel.org/all/173572211264.1875638.9927288574435880962.b4-ty@linux.ibm.com/
> 
> Shrikanth Hegde (1):
>    powerpc: Enable dynamic preemption
> 
>   arch/powerpc/Kconfig               |  1 +
>   arch/powerpc/include/asm/preempt.h | 12 ++++++++++++
>   arch/powerpc/kernel/interrupt.c    |  6 +++++-
>   arch/powerpc/kernel/traps.c        |  6 +++++-
>   arch/powerpc/lib/vmx-helper.c      |  2 +-
>   5 files changed, 24 insertions(+), 3 deletions(-)
>   create mode 100644 arch/powerpc/include/asm/preempt.h
> 

Hi maddy, mpe, christophe.

Now that preempt=lazy is merged upstream, is would be start to enable 
dynamic preemption to make use of all preemption models at runtime.

Are there any concerns with this patch?

Thanks.

Re: [PATCH v3 1/1] powerpc: Enable dynamic preemption

[Sebastian Andrzej Siewior]

On 2025-01-06 10:49:19 [+0530], Shrikanth Hegde wrote:
> --- 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) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)
> +DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
> +#endif

I am uncertain here: Do you need to DEFINE it? It is set by the sched
core which also defines it. It should be same thing after all, right?

> +
>  #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 (preempt_model_preemptible()) {

CONFIG_HAVE_PREEMPT_DYNAMIC_KEY is the only option, right? Wouldn't

| #DEFINE need_irq_preemption() \
|          (static_branch_unlikely(&sk_dynamic_irqentry_exit_cond_resched))
| 
| 	if (need_irq_preemption()) {

be a bit smaller/ quicker? This could be a fast path ;)

>  			/* Return to preemptible kernel context */
>  			if (unlikely(read_thread_flags() & _TIF_NEED_RESCHED)) {
>  				if (preempt_count() == 0)
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index edf5cabe5dfd..2556fa8ec019 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -266,7 +266,11 @@ static int __die(const char *str, struct pt_regs *regs, long err)
>  	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
>  	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
>  	       PAGE_SIZE / 1024, get_mmu_str(),
> -	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
> +	       preempt_model_none()      ? "none" :
> +	       preempt_model_voluntary() ? "voluntary" :
> +	       preempt_model_full()      ? "full" :
> +	       preempt_model_lazy()      ? "lazy" :
> +	       "",

So intend to rework this part. I have patches stashed at
	https://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/staging.git/log/?h=preemption_string

which I didn't sent yet due to the merge window. Just a heads up ;)

>  	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
>  	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
>  	       debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "",

Sebastian

Re: [PATCH v3 1/1] powerpc: Enable dynamic preemption

[Christophe Leroy]

Le 30/01/2025 à 15:54, Sebastian Andrzej Siewior a écrit :
> On 2025-01-06 10:49:19 [+0530], Shrikanth Hegde wrote:
>> --- 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) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)
>> +DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
>> +#endif
> 
> I am uncertain here: Do you need to DEFINE it? It is set by the sched
> core which also defines it. It should be same thing after all, right?

As far as I can see it is not handled by sched core.

$ git grep sk_dynamic_irqentry_exit_cond_resched
arch/arm64/include/asm/preempt.h:DECLARE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
arch/arm64/kernel/entry-common.c:DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
arch/arm64/kernel/entry-common.c: 
(static_branch_unlikely(&sk_dynamic_irqentry_exit_cond_resched))
include/linux/entry-common.h:DECLARE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
kernel/entry/common.c:DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
kernel/entry/common.c:  if 
(!static_branch_unlikely(&sk_dynamic_irqentry_exit_cond_resched))

It is in common entry but arm64 and powerpc don't use common entry.

$ git grep GENERIC_ENTRY arch
arch/Kconfig:config GENERIC_ENTRY
arch/loongarch/Kconfig: select GENERIC_ENTRY
arch/riscv/Kconfig:     select GENERIC_ENTRY
arch/s390/Kconfig:      select GENERIC_ENTRY
arch/x86/Kconfig:       select GENERIC_ENTRY


Christophe

Re: [PATCH v3 1/1] powerpc: Enable dynamic preemption

[Sebastian Andrzej Siewior]

On 2025-01-30 16:03:09 [+0100], Christophe Leroy wrote:
> Le 30/01/2025 à 15:54, Sebastian Andrzej Siewior a écrit :
> > On 2025-01-06 10:49:19 [+0530], Shrikanth Hegde wrote:
> > > --- 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) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)
> > > +DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
> > > +#endif
> > 
> > I am uncertain here: Do you need to DEFINE it? It is set by the sched
> > core which also defines it. It should be same thing after all, right?
> 
> As far as I can see it is not handled by sched core.

…
> It is in common entry but arm64 and powerpc don't use common entry.

Okay. So it is defined in the generic part (so you have to define it on
power) but it is used by sched-core (search for
	preempt_dynamic_enable(irqentry_exit_cond_resched);
).
It might make sense to define it there (in the sched part where it is
used) but this is out of scope here, it just confused me :)
Thanks for the pointer.

> Christophe

Sebastian

Re: [PATCH v3 1/1] powerpc: Enable dynamic preemption

[Shrikanth Hegde]

On 1/30/25 20:24, Sebastian Andrzej Siewior wrote:
> On 2025-01-06 10:49:19 [+0530], Shrikanth Hegde wrote:
>> --- a/arch/powerpc/kernel/interrupt.c

> 

Thanks for taking a look.

>> +
>>   #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 (preempt_model_preemptible()) {
> 
> CONFIG_HAVE_PREEMPT_DYNAMIC_KEY is the only option, right? Wouldn't
> 
> | #DEFINE need_irq_preemption() \
> |          (static_branch_unlikely(&sk_dynamic_irqentry_exit_cond_resched))
> |
> | 	if (need_irq_preemption()) {
> 
> be a bit smaller/ quicker? This could be a fast path ;)

I am okay with either way. I did try both[1], there wasn't any significant difference,
hence chose a simpler one. May be system size, workload pattern might matter.

Let me do some more testing to see which one wins.
Is there any specific benchmark which might help here?

[1]: https://lore.kernel.org/all/b98b7795-070a-4d9c-9599-445c2ff55fd7@linux.ibm.com/

> 
>>   			/* Return to preemptible kernel context */
>>   			if (unlikely(read_thread_flags() & _TIF_NEED_RESCHED)) {
>>   				if (preempt_count() == 0)
>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>> index edf5cabe5dfd..2556fa8ec019 100644
>> --- a/arch/powerpc/kernel/traps.c
>> +++ b/arch/powerpc/kernel/traps.c
>> @@ -266,7 +266,11 @@ static int __die(const char *str, struct pt_regs *regs, long err)
>>   	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
>>   	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
>>   	       PAGE_SIZE / 1024, get_mmu_str(),
>> -	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
>> +	       preempt_model_none()      ? "none" :
>> +	       preempt_model_voluntary() ? "voluntary" :
>> +	       preempt_model_full()      ? "full" :
>> +	       preempt_model_lazy()      ? "lazy" :
>> +	       "",
> 
> So intend to rework this part. I have patches stashed at
> 	https://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/staging.git/log/?h=preemption_string
> 
> which I didn't sent yet due to the merge window. Just a heads up ;)

Makes sense. I had seen at-least two places where this code was there, ftrace/powerpc.
There were way more places..

You want me to remove this part?

> 
>>   	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
>>   	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
>>   	       debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "",
> 
> Sebastian

Re: [PATCH v3 1/1] powerpc: Enable dynamic preemption

[Sebastian Andrzej Siewior]

On 2025-01-30 22:27:07 [+0530], Shrikanth Hegde wrote:
> > | #DEFINE need_irq_preemption() \
> > |          (static_branch_unlikely(&sk_dynamic_irqentry_exit_cond_resched))
> > |
> > | 	if (need_irq_preemption()) {
> > 
> > be a bit smaller/ quicker? This could be a fast path ;)
> 
> I am okay with either way. I did try both[1], there wasn't any significant difference,
> hence chose a simpler one. May be system size, workload pattern might matter.
> 
> Let me do some more testing to see which one wins.
> Is there any specific benchmark which might help here?

No idea. As per bean counting: preempt_model_preemptible() should
resolve in two function calls + conditional in the dynamic case. This
should be more expensive compared to a nop/ branch ;)
But you would still need preempt_model_preemptible() for the !DYN case.

> > > +	       preempt_model_voluntary() ? "voluntary" :
> > > +	       preempt_model_full()      ? "full" :
> > > +	       preempt_model_lazy()      ? "lazy" :
> > > +	       "",
> > 
> > So intend to rework this part. I have patches stashed at
> > 	https://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/staging.git/log/?h=preemption_string
> > 
> > which I didn't sent yet due to the merge window. Just a heads up ;)
> 
> Makes sense. I had seen at-least two places where this code was there, ftrace/powerpc.
> There were way more places..
> 
> You want me to remove this part?

No, just be aware.
I don't know how this will be routed I guess we merge the sched pieces
first and then I submit the other pieces via the relevant maintainer
tree. In that case please be aware that all parts get removed/ replaced
properly.

Sebastian

Re: [PATCH v3 1/1] powerpc: Enable dynamic preemption

[Christophe Leroy]

Le 30/01/2025 à 21:26, Sebastian Andrzej Siewior a écrit :
> On 2025-01-30 22:27:07 [+0530], Shrikanth Hegde wrote:
>>> | #DEFINE need_irq_preemption() \
>>> |          (static_branch_unlikely(&sk_dynamic_irqentry_exit_cond_resched))
>>> |
>>> | 	if (need_irq_preemption()) {
>>>
>>> be a bit smaller/ quicker? This could be a fast path ;)
>>
>> I am okay with either way. I did try both[1], there wasn't any significant difference,
>> hence chose a simpler one. May be system size, workload pattern might matter.
>>
>> Let me do some more testing to see which one wins.
>> Is there any specific benchmark which might help here?
> 
> No idea. As per bean counting: preempt_model_preemptible() should
> resolve in two function calls + conditional in the dynamic case. This
> should be more expensive compared to a nop/ branch ;)

I did a build comparison on pmac32_defconfig + dynamic preemption, the 
static branch looks definitely more optimal:

Without static branch:

...
  294:	7c 08 02 a6 	mflr    r0
  298:	90 01 00 24 	stw     r0,36(r1)
  29c:	48 00 00 01 	bl      29c <interrupt_exit_kernel_prepare+0x50>
			29c: R_PPC_REL24	preempt_model_full
  2a0:	2c 03 00 00 	cmpwi   r3,0
  2a4:	41 82 00 a4 	beq     348 <interrupt_exit_kernel_prepare+0xfc>
...
  2a8:	81 22 00 4c 	lwz     r9,76(r2)
  2ac:	71 29 00 04 	andi.   r9,r9,4
  2b0:	40 82 00 d4 	bne     384 <interrupt_exit_kernel_prepare+0x138>
...
  2b4:	7d 20 00 a6 	mfmsr   r9
  2b8:	55 29 07 fa 	rlwinm  r9,r9,0,31,29
...
  348:	48 00 00 01 	bl      348 <interrupt_exit_kernel_prepare+0xfc>
			348: R_PPC_REL24	preempt_model_lazy
  34c:	2c 03 00 00 	cmpwi   r3,0
  350:	40 a2 ff 58 	bne     2a8 <interrupt_exit_kernel_prepare+0x5c>
  354:	4b ff ff 60 	b       2b4 <interrupt_exit_kernel_prepare+0x68>

With the static branch:

...
  294:	48 00 00 58 	b       2ec <interrupt_exit_kernel_prepare+0xa0>
...
  298:	7d 20 00 a6 	mfmsr   r9
  29c:	55 29 07 fa 	rlwinm  r9,r9,0,31,29
...
  2ec:	81 22 00 4c 	lwz     r9,76(r2)
  2f0:	71 29 00 04 	andi.   r9,r9,4
  2f4:	41 82 ff a4 	beq     298 <interrupt_exit_kernel_prepare+0x4c>
...

With the static branch it is just an unconditional branch being later 
replaced by a NOP. It must be more efficient.

So in first case we need to get and save LR, call preempt_model_full(), 
compare with 0, call preempt_model_lazy() and compare again, and at some 
point read and restore LR.

And the preempt_model_() functions are not tiny functions:

00003620 <preempt_model_full>:
     3620:	3d 20 00 00 	lis     r9,0
			3622: R_PPC_ADDR16_HA	preempt_dynamic_mode
     3624:	94 21 ff f0 	stwu    r1,-16(r1)
     3628:	80 69 00 00 	lwz     r3,0(r9)
			362a: R_PPC_ADDR16_LO	preempt_dynamic_mode
     362c:	2c 03 ff ff 	cmpwi   r3,-1
     3630:	41 82 00 18 	beq     3648 <preempt_model_full+0x28>
     3634:	68 63 00 02 	xori    r3,r3,2
     3638:	38 21 00 10 	addi    r1,r1,16
     363c:	7c 63 00 34 	cntlzw  r3,r3
     3640:	54 63 d9 7e 	srwi    r3,r3,5
     3644:	4e 80 00 20 	blr
...
     3648:	0f e0 00 00 	twui    r0,0
     364c:	68 63 00 02 	xori    r3,r3,2
     3650:	38 21 00 10 	addi    r1,r1,16
     3654:	7c 63 00 34 	cntlzw  r3,r3
     3658:	54 63 d9 7e 	srwi    r3,r3,5
     365c:	4e 80 00 20 	blr

00003660 <preempt_model_lazy>:
     3660:	3d 20 00 00 	lis     r9,0
			3662: R_PPC_ADDR16_HA	preempt_dynamic_mode
     3664:	94 21 ff f0 	stwu    r1,-16(r1)
     3668:	80 69 00 00 	lwz     r3,0(r9)
			366a: R_PPC_ADDR16_LO	preempt_dynamic_mode
     366c:	2c 03 ff ff 	cmpwi   r3,-1
     3670:	41 82 00 18 	beq     3688 <preempt_model_lazy+0x28>
     3674:	68 63 00 03 	xori    r3,r3,3
     3678:	38 21 00 10 	addi    r1,r1,16
     367c:	7c 63 00 34 	cntlzw  r3,r3
     3680:	54 63 d9 7e 	srwi    r3,r3,5
     3684:	4e 80 00 20 	blr
...
     3688:	0f e0 00 00 	twui    r0,0
     368c:	68 63 00 03 	xori    r3,r3,3
     3690:	38 21 00 10 	addi    r1,r1,16
     3694:	7c 63 00 34 	cntlzw  r3,r3
     3698:	54 63 d9 7e 	srwi    r3,r3,5
     369c:	4e 80 00 20 	blr



> But you would still need preempt_model_preemptible() for the !DYN case.
> 
>>>> +	       preempt_model_voluntary() ? "voluntary" :
>>>> +	       preempt_model_full()      ? "full" :
>>>> +	       preempt_model_lazy()      ? "lazy" :
>>>> +	       "",
>>>
>>> So intend to rework this part. I have patches stashed at
>>> 	https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fbigeasy%2Fstaging.git%2Flog%2F%3Fh%3Dpreemption_string&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7C553a8f640a9c4514597d08dd416c6534%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638738655988556429%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=ZfVQi1iRYaVCTrZ5vIhOQ7yAKaDnOwFi0NRjycfrI5A%3D&reserved=0
>>>
>>> which I didn't sent yet due to the merge window. Just a heads up ;)
>>
>> Makes sense. I had seen at-least two places where this code was there, ftrace/powerpc.
>> There were way more places..
>>
>> You want me to remove this part?
> 
> No, just be aware.
> I don't know how this will be routed I guess we merge the sched pieces
> first and then I submit the other pieces via the relevant maintainer
> tree. In that case please be aware that all parts get removed/ replaced
> properly.
> 
> Sebastian

Re: [PATCH v3 1/1] powerpc: Enable dynamic preemption

[Shrikanth Hegde]

On 1/31/25 11:39, Christophe Leroy wrote:
> 
> 
> Le 30/01/2025 à 21:26, Sebastian Andrzej Siewior a écrit :
>> On 2025-01-30 22:27:07 [+0530], Shrikanth Hegde wrote:
>>>> | #DEFINE need_irq_preemption() \
>>>> |          
>>>> (static_branch_unlikely(&sk_dynamic_irqentry_exit_cond_resched))
>>>> |
>>>> |     if (need_irq_preemption()) {
>>>>
>>>> be a bit smaller/ quicker? This could be a fast path ;)
>>>
>>> I am okay with either way. I did try both[1], there wasn't any 
>>> significant difference,
>>> hence chose a simpler one. May be system size, workload pattern might 
>>> matter.
>>>
>>> Let me do some more testing to see which one wins.
>>> Is there any specific benchmark which might help here?
>>
>> No idea. As per bean counting: preempt_model_preemptible() should
>> resolve in two function calls + conditional in the dynamic case. This
>> should be more expensive compared to a nop/ branch ;)
> 
> I did a build comparison on pmac32_defconfig + dynamic preemption, the 
> static branch looks definitely more optimal:
> 
> Without static branch:
> 
> ...
>   294:    7c 08 02 a6     mflr    r0
>   298:    90 01 00 24     stw     r0,36(r1)
>   29c:    48 00 00 01     bl      29c <interrupt_exit_kernel_prepare+0x50>
>              29c: R_PPC_REL24    preempt_model_full
>   2a0:    2c 03 00 00     cmpwi   r3,0
>   2a4:    41 82 00 a4     beq     348 <interrupt_exit_kernel_prepare+0xfc>
> ...
>   2a8:    81 22 00 4c     lwz     r9,76(r2)
>   2ac:    71 29 00 04     andi.   r9,r9,4
>   2b0:    40 82 00 d4     bne     384 <interrupt_exit_kernel_prepare+0x138>
> ...
>   2b4:    7d 20 00 a6     mfmsr   r9
>   2b8:    55 29 07 fa     rlwinm  r9,r9,0,31,29
> ...
>   348:    48 00 00 01     bl      348 <interrupt_exit_kernel_prepare+0xfc>
>              348: R_PPC_REL24    preempt_model_lazy
>   34c:    2c 03 00 00     cmpwi   r3,0
>   350:    40 a2 ff 58     bne     2a8 <interrupt_exit_kernel_prepare+0x5c>
>   354:    4b ff ff 60     b       2b4 <interrupt_exit_kernel_prepare+0x68>
> 
> With the static branch:
> 
> ...
>   294:    48 00 00 58     b       2ec <interrupt_exit_kernel_prepare+0xa0>
> ...
>   298:    7d 20 00 a6     mfmsr   r9
>   29c:    55 29 07 fa     rlwinm  r9,r9,0,31,29
> ...
>   2ec:    81 22 00 4c     lwz     r9,76(r2)
>   2f0:    71 29 00 04     andi.   r9,r9,4
>   2f4:    41 82 ff a4     beq     298 <interrupt_exit_kernel_prepare+0x4c>
> ...
> 
> With the static branch it is just an unconditional branch being later 
> replaced by a NOP. It must be more efficient.
> 
> So in first case we need to get and save LR, call preempt_model_full(), 
> compare with 0, call preempt_model_lazy() and compare again, and at some 
> point read and restore LR.
> 
> And the preempt_model_() functions are not tiny functions:
> 
> 00003620 <preempt_model_full>:
>      3620:    3d 20 00 00     lis     r9,0
>              3622: R_PPC_ADDR16_HA    preempt_dynamic_mode
>      3624:    94 21 ff f0     stwu    r1,-16(r1)
>      3628:    80 69 00 00     lwz     r3,0(r9)
>              362a: R_PPC_ADDR16_LO    preempt_dynamic_mode
>      362c:    2c 03 ff ff     cmpwi   r3,-1
>      3630:    41 82 00 18     beq     3648 <preempt_model_full+0x28>
>      3634:    68 63 00 02     xori    r3,r3,2
>      3638:    38 21 00 10     addi    r1,r1,16
>      363c:    7c 63 00 34     cntlzw  r3,r3
>      3640:    54 63 d9 7e     srwi    r3,r3,5
>      3644:    4e 80 00 20     blr
> ...
>      3648:    0f e0 00 00     twui    r0,0
>      364c:    68 63 00 02     xori    r3,r3,2
>      3650:    38 21 00 10     addi    r1,r1,16
>      3654:    7c 63 00 34     cntlzw  r3,r3
>      3658:    54 63 d9 7e     srwi    r3,r3,5
>      365c:    4e 80 00 20     blr
> 
> 00003660 <preempt_model_lazy>:
>      3660:    3d 20 00 00     lis     r9,0
>              3662: R_PPC_ADDR16_HA    preempt_dynamic_mode
>      3664:    94 21 ff f0     stwu    r1,-16(r1)
>      3668:    80 69 00 00     lwz     r3,0(r9)
>              366a: R_PPC_ADDR16_LO    preempt_dynamic_mode
>      366c:    2c 03 ff ff     cmpwi   r3,-1
>      3670:    41 82 00 18     beq     3688 <preempt_model_lazy+0x28>
>      3674:    68 63 00 03     xori    r3,r3,3
>      3678:    38 21 00 10     addi    r1,r1,16
>      367c:    7c 63 00 34     cntlzw  r3,r3
>      3680:    54 63 d9 7e     srwi    r3,r3,5
>      3684:    4e 80 00 20     blr
> ...
>      3688:    0f e0 00 00     twui    r0,0
>      368c:    68 63 00 03     xori    r3,r3,3
>      3690:    38 21 00 10     addi    r1,r1,16
>      3694:    7c 63 00 34     cntlzw  r3,r3
>      3698:    54 63 d9 7e     srwi    r3,r3,5
>      369c:    4e 80 00 20     blr
> 
> 
> 

That's convincing. Thanks for the pointers. I will move it into static 
branch method.

That makes the code same for arm64/powerpc. Maybe its worth moving it 
into sched/core. Let me try that as well.

PS: on leave next week, so will be sending the patches after i get back.

>> But you would still need preempt_model_preemptible() for the !DYN case.
>>
>>>>> +           preempt_model_voluntary() ? "voluntary" :
>>>>> +           preempt_model_full()      ? "full" :
>>>>> +           preempt_model_lazy()      ? "lazy" :
>>>>> +           "",
>>>>
>>>> So intend to rework this part. I have patches stashed at
>>>>     https://eur01.safelinks.protection.outlook.com/? 
>>>> url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fbigeasy%2Fstaging.git%2Flog%2F%3Fh%3Dpreemption_string&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7C553a8f640a9c4514597d08dd416c6534%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638738655988556429%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=ZfVQi1iRYaVCTrZ5vIhOQ7yAKaDnOwFi0NRjycfrI5A%3D&reserved=0
>>>>
>>>> which I didn't sent yet due to the merge window. Just a heads up ;)
>>>
>>> Makes sense. I had seen at-least two places where this code was 
>>> there, ftrace/powerpc.
>>> There were way more places..
>>>
>>> You want me to remove this part?
>>
>> No, just be aware.
>> I don't know how this will be routed I guess we merge the sched pieces
>> first and then I submit the other pieces via the relevant maintainer
>> tree. In that case please be aware that all parts get removed/ replaced
>> properly.
>>
>> Sebastian
>