Re: [PATCH 03/34] powerpc: Use CONFIG_PREEMPTION

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2019-10-16 06:57:48 [+0200], Christophe Leroy wrote:
> 
> 
> Le 15/10/2019 à 21:17, Sebastian Andrzej Siewior a écrit :
> > From: Thomas Gleixner <tglx@linutronix.de>
> > 
> > CONFIG_PREEMPTION is selected by CONFIG_PREEMPT and by CONFIG_PREEMPT_RT.
> > Both PREEMPT and PREEMPT_RT require the same functionality which today
> > depends on CONFIG_PREEMPT.
> > 
> > Switch the entry code over to use CONFIG_PREEMPTION. Add PREEMPT_RT
> > output in __die().
> 
> powerpc doesn't select ARCH_SUPPORTS_RT, so this change is useless as
> CONFIG_PREEMPT_RT cannot be selected.

No it is not. It makes it possible for PowerPC to select it one day and
I have patches for it today. Also, if other ARCH copies code from
PowerPC it will copy the correct thing (as in distinguish between the
flavour PREEMPT and the functionality PREEMPTION).

> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index 82f43535e6867..23d2f20be4f2e 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -252,14 +252,19 @@ NOKPROBE_SYMBOL(oops_end);
> >   static int __die(const char *str, struct pt_regs *regs, long err)
> >   {
> > +	const char *pr = "";
> > +
> 
> Please follow the same approach as already existing. Don't add a local var
> for that.

I would leave it to the maintainer to comment on that and decide which
one they want. My eyes find it more readable and the compiles does not
create more code.

> >   	printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
> > +	if (IS_ENABLED(CONFIG_PREEMPTION))
> > +		pr = IS_ENABLED(CONFIG_PREEMPT_RT) ? " PREEMPT_RT" : " PREEMPT";
> > +
> 
> drop
> 
> >   	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s%s %s\n",
> 
> Add one %s
> 
> >   	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
> >   	       PAGE_SIZE / 1024,
> >   	       early_radix_enabled() ? " MMU=Radix" : "",
> >   	       early_mmu_has_feature(MMU_FTR_HPTE_TABLE) ? " MMU=Hash" : "",
> > -	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
> 
> Replace by: 	IS_ENABLED(CONFIG_PREEMPTION) ? " PREEMPT" : ""
> 
> > +	       pr,
> 
> add something like: IS_ENABLED(CONFIG_PREEMPT_RT) ? "_RT" : ""

this on the other hand will create more code which is not strictly
required.

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

Sebastian