Re: [PATCH v2 3/5] powerpc: Use preemption model accessors

From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Valentin Schneider <valentin.schneider@arm.com>,
	linux-kernel@vger.kernel.org, kasan-dev@googlegroups.com,
	linuxppc-dev@lists.ozlabs.org, linux-kbuild@vger.kernel.org
Cc: Marco Elver <elver@google.com>,
	Michal Marek <michal.lkml@markovi.net>,
	Peter Zijlstra <peterz@infradead.org>,
	Frederic Weisbecker <frederic@kernel.org>,
	Mike Galbraith <efault@gmx.de>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Paul Mackerras <paulus@samba.org>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Dmitry Vyukov <dvyukov@google.com>
Subject: Re: [PATCH v2 3/5] powerpc: Use preemption model accessors
Date: Tue, 16 Nov 2021 14:41:03 +0100	[thread overview]
Message-ID: <431fb6da-fe21-c5a6-bfb3-4e26bdc153b4@csgroup.eu> (raw)
In-Reply-To: <20211110202448.4054153-4-valentin.schneider@arm.com>

Le 10/11/2021 à 21:24, Valentin Schneider a écrit :
> Per PREEMPT_DYNAMIC, checking CONFIG_PREEMPT doesn't tell you the actual
> preemption model of the live kernel. Use the newly-introduced accessors
> instead.

Is that change worth it for now ? As far as I can see powerpc doesn't 
have DYNAMIC PREEMPT, a lot of work needs to be done before being able 
to use it:
- Implement GENERIC_ENTRY
- Implement STATIC_CALLS (already done on PPC32, to be done on PPC64)

> 
> sched_init() -> preempt_dynamic_init() happens way before IRQs are set up,
> so this should be fine.

It looks like you are mixing up interrupts and IRQs (also known as 
"external interrupts").

ISI (Instruction Storage Interrupt) and DSI (Data Storage Interrupt) for 
instance are also interrupts. They happen everytime there is a page 
fault so may happen pretty early.

Traps generated by WARN_ON() are also interrupts that may happen at any 
time.

> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>   arch/powerpc/kernel/interrupt.c | 2 +-
>   arch/powerpc/kernel/traps.c     | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index de10a2697258..c56c10b59be3 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -552,7 +552,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_PREEMPT)) {
> +		if (is_preempt_full()) {

I think the cost of that additionnal test should be analysed. Maybe it's 
worth not doing the test at all and check _TIF_NEED_RESCHED everytime, 
unless that recurrent test is changed into a jump label as suggested in 
patch 2.

>   			/* Return to preemptible kernel context */
>   			if (unlikely(current_thread_info()->flags & _TIF_NEED_RESCHED)) {
>   				if (preempt_count() == 0)
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index aac8c0412ff9..1cb31bbdc925 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -265,7 +265,7 @@ 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" : "",
> +	       is_preempt_full() ? " PREEMPT" : "",
>   	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
>   	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
>   	       debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "",
> 

Would it be interesting as well to know that we are indeed in a DYNAMIC 
PREEMPT context when dying ?

Christophe