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

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

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