linux-kernel - Re: [PATCH v2 1/2] powerpc: Add preempt lazy support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9B8CF71F0A7A05E9+Z1afmUprV5hKycvr@HX09040029.powercore.com.cn>
Date: Mon, 9 Dec 2024 15:43:21 +0800
From: Luming Yu <luming.yu@...ngroup.cn>
To: Shrikanth Hegde <sshegde@...ux.ibm.com>
Cc: Christophe Leroy <christophe.leroy@...roup.eu>, npiggin@...il.com,
	maddy@...ux.ibm.com, bigeasy@...utronix.de,
	ankur.a.arora@...cle.com, linux-kernel@...r.kernel.org,
	mpe@...erman.id.au, linuxppc-dev@...ts.ozlabs.org,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v2 1/2] powerpc: Add preempt lazy support

On Mon, Dec 02, 2024 at 12:58:59AM +0530, Shrikanth Hegde wrote:
> 
> 
> On 11/26/24 16:23, Christophe Leroy wrote:
> > 
> > 
> > Le 16/11/2024 à 20:23, Shrikanth Hegde a écrit :
> > > Define preempt lazy bit for Powerpc. Use bit 9 which is free and within
> > > 16 bit range of NEED_RESCHED, so compiler can issue single andi.
> > > 
> > > Since Powerpc doesn't use the generic entry/exit, add lazy check at exit
> > > to user. CONFIG_PREEMPTION is defined for lazy/full/rt so use it for
> > > return to kernel.
> > 
> > FWIW, there is work in progress on using generic entry/exit for powerpc,
> > if you can help testing it that can help, see https://
> > patchwork.ozlabs.org/project/linuxppc-dev/patch/
> > F0AE0A4571CE3126+20241111031934.1579-2-luming.yu@...ngroup.cn/
> > 
> 
> I gave that series a try. After a lot of manual patching on tip tree and
> removal of multiple definition of regs_irqs_disabled, i was able to compile
> and boot.
> 
> I am skimming through the series, but as far as i understand from the
> comments, it needs to be redesigned. I probably get it why. I will go
> through it in more detail to figure out how to do it better. I believe the
> changes have to stem from interrupt_64.S

Thanks for your time for the work.

when you do it, besides compile, boot, and ppc linux-ci github workflow, 
please also do selftest 
make -C tools/testing/selftest TARGETS=seccomp run_tests to make sure
all secommp 98 unit tests passed. 

> 
> As far as the bits of this patch with that patch concerned, it is with
> NEED_RESCHED bits. There atleast couple of major issues in that patch series
> that are wrong.
> 
> 1. It only tries to move exit to user to generic. exit to kernel is not.
> there is generic irqentry_exit that handles it for generic code. powerpc
> exit to kernel still there.
> 
> 2. Even for exit to user, it ends up calling exit_to_user_mode_loop twice
> for the same syscall. that seems wrong. once in
> interrupt_exit_user_prepare_main and once through syscall_exit_to_user_mode
> in syscall_exit_prepare.

I knew this as ppc system call uses interrupt path. I planned to delete
ppc speicifc interrupt_exit_user_prepare_main later for this series.

Now it might be good timing to complete it as there is a real use case need this
to be done. :-)
    
> 
> 
> > Christophe
> > 
> 
> So I guess, when we do eventually if move, this checks would be removed at
> that point along with rest of the code.
> 
> > 
> > > 
> > > Ran a few benchmarks and db workload on Power10. Performance is close to
> > > preempt=none/voluntary.
> > > Since Powerpc systems can have large core count and large memory,
> > > preempt lazy is going to be helpful in avoiding soft lockup issues.
> > > 
> > > Reviewed-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> > > Reviewed-by: Ankur Arora <ankur.a.arora@...cle.com>
> > > Signed-off-by: Shrikanth Hegde <sshegde@...ux.ibm.com>
> > > ---
> > >   arch/powerpc/Kconfig                   | 1 +
> > >   arch/powerpc/include/asm/thread_info.h | 9 ++++++---
> > >   arch/powerpc/kernel/interrupt.c        | 4 ++--
> > >   3 files changed, 9 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > > index 8094a01974cc..2f625aecf94b 100644
> > > --- a/arch/powerpc/Kconfig
> > > +++ b/arch/powerpc/Kconfig
> > > @@ -145,6 +145,7 @@ config PPC
> > >       select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> > >       select ARCH_HAS_PHYS_TO_DMA
> > >       select ARCH_HAS_PMEM_API
> > > +    select ARCH_HAS_PREEMPT_LAZY
> > >       select ARCH_HAS_PTE_DEVMAP        if PPC_BOOK3S_64
> > >       select ARCH_HAS_PTE_SPECIAL
> > >       select ARCH_HAS_SCALED_CPUTIME        if
> > > VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
> > > diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/
> > > include/asm/thread_info.h
> > > index 6ebca2996f18..2785c7462ebf 100644
> > > --- a/arch/powerpc/include/asm/thread_info.h
> > > +++ b/arch/powerpc/include/asm/thread_info.h
> > > @@ -103,6 +103,7 @@ void arch_setup_new_exec(void);
> > >   #define TIF_PATCH_PENDING    6    /* pending live patching update */
> > >   #define TIF_SYSCALL_AUDIT    7    /* syscall auditing active */
> > >   #define TIF_SINGLESTEP        8    /* singlestepping active */
> > > +#define TIF_NEED_RESCHED_LAZY    9       /* Scheduler driven lazy
> > > preemption */
> > >   #define TIF_SECCOMP        10    /* secure computing */
> > >   #define TIF_RESTOREALL        11    /* Restore all regs (implies
> > > NOERROR) */
> > >   #define TIF_NOERROR        12    /* Force successful syscall return */
> > > @@ -122,6 +123,7 @@ void arch_setup_new_exec(void);
> > >   #define _TIF_SYSCALL_TRACE    (1<<TIF_SYSCALL_TRACE)
> > >   #define _TIF_SIGPENDING        (1<<TIF_SIGPENDING)
> > >   #define _TIF_NEED_RESCHED    (1<<TIF_NEED_RESCHED)
> > > +#define _TIF_NEED_RESCHED_LAZY    (1<<TIF_NEED_RESCHED_LAZY)
> > >   #define _TIF_NOTIFY_SIGNAL    (1<<TIF_NOTIFY_SIGNAL)
> > >   #define _TIF_POLLING_NRFLAG    (1<<TIF_POLLING_NRFLAG)
> > >   #define _TIF_32BIT        (1<<TIF_32BIT)
> > > @@ -142,9 +144,10 @@ void arch_setup_new_exec(void);
> > >                    _TIF_SYSCALL_EMU)
> > >   #define _TIF_USER_WORK_MASK    (_TIF_SIGPENDING | _TIF_NEED_RESCHED | \
> > > -                 _TIF_NOTIFY_RESUME | _TIF_UPROBE | \
> > > -                 _TIF_RESTORE_TM | _TIF_PATCH_PENDING | \
> > > -                 _TIF_NOTIFY_SIGNAL)
> > > +                 _TIF_NEED_RESCHED_LAZY | _TIF_NOTIFY_RESUME | \
> > > +                 _TIF_UPROBE | _TIF_RESTORE_TM | \
> > > +                 _TIF_PATCH_PENDING | _TIF_NOTIFY_SIGNAL)
> > > +
> > >   #define _TIF_PERSYSCALL_MASK    (_TIF_RESTOREALL|_TIF_NOERROR)
> > >   /* Bits in local_flags */
> > > diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/
> > > interrupt.c
> > > index af62ec974b97..8f4acc55407b 100644
> > > --- a/arch/powerpc/kernel/interrupt.c
> > > +++ b/arch/powerpc/kernel/interrupt.c
> > > @@ -185,7 +185,7 @@ interrupt_exit_user_prepare_main(unsigned long
> > > ret, struct pt_regs *regs)
> > >       ti_flags = read_thread_flags();
> > >       while (unlikely(ti_flags & (_TIF_USER_WORK_MASK &
> > > ~_TIF_RESTORE_TM))) {
> > >           local_irq_enable();
> > > -        if (ti_flags & _TIF_NEED_RESCHED) {
> 
> 
> 
> > > +        if (ti_flags & (_TIF_NEED_RESCHED | _TIF_NEED_RESCHED_LAZY)) {
> > >               schedule();
> > >           } else {
> > >               /*
> > > @@ -396,7 +396,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_ENABLED(CONFIG_PREEMPTION)) {
> > >               /* Return to preemptible kernel context */
> > >               if (unlikely(read_thread_flags() & _TIF_NEED_RESCHED)) {
> > >                   if (preempt_count() == 0)
>