lkml.org 
[lkml]   [2020]   [Jul]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH v3 5/6] powerpc/pseries: implement paravirt qspinlocks for SPLPAR
    Date
    Nicholas Piggin <npiggin@gmail.com> writes:

    > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
    > ---
    > arch/powerpc/include/asm/paravirt.h | 28 ++++++++
    > arch/powerpc/include/asm/qspinlock.h | 66 +++++++++++++++++++
    > arch/powerpc/include/asm/qspinlock_paravirt.h | 7 ++
    > arch/powerpc/platforms/pseries/Kconfig | 5 ++
    > arch/powerpc/platforms/pseries/setup.c | 6 +-
    > include/asm-generic/qspinlock.h | 2 +

    Another ack?

    > diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
    > index 7a8546660a63..f2d51f929cf5 100644
    > --- a/arch/powerpc/include/asm/paravirt.h
    > +++ b/arch/powerpc/include/asm/paravirt.h
    > @@ -45,6 +55,19 @@ static inline void yield_to_preempted(int cpu, u32 yield_count)
    > {
    > ___bad_yield_to_preempted(); /* This would be a bug */
    > }
    > +
    > +extern void ___bad_yield_to_any(void);
    > +static inline void yield_to_any(void)
    > +{
    > + ___bad_yield_to_any(); /* This would be a bug */
    > +}

    Why do we do that rather than just not defining yield_to_any() at all
    and letting the build fail on that?

    There's a condition somewhere that we know will false at compile time
    and drop the call before linking?

    > diff --git a/arch/powerpc/include/asm/qspinlock_paravirt.h b/arch/powerpc/include/asm/qspinlock_paravirt.h
    > new file mode 100644
    > index 000000000000..750d1b5e0202
    > --- /dev/null
    > +++ b/arch/powerpc/include/asm/qspinlock_paravirt.h
    > @@ -0,0 +1,7 @@
    > +/* SPDX-License-Identifier: GPL-2.0-or-later */
    > +#ifndef __ASM_QSPINLOCK_PARAVIRT_H
    > +#define __ASM_QSPINLOCK_PARAVIRT_H

    _ASM_POWERPC_QSPINLOCK_PARAVIRT_H please.

    > +
    > +EXPORT_SYMBOL(__pv_queued_spin_unlock);

    Why's that in a header? Should that (eventually) go with the generic implementation?

    > diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig
    > index 24c18362e5ea..756e727b383f 100644
    > --- a/arch/powerpc/platforms/pseries/Kconfig
    > +++ b/arch/powerpc/platforms/pseries/Kconfig
    > @@ -25,9 +25,14 @@ config PPC_PSERIES
    > select SWIOTLB
    > default y
    >
    > +config PARAVIRT_SPINLOCKS
    > + bool
    > + default n

    default n is the default.

    > diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
    > index 2db8469e475f..747a203d9453 100644
    > --- a/arch/powerpc/platforms/pseries/setup.c
    > +++ b/arch/powerpc/platforms/pseries/setup.c
    > @@ -771,8 +771,12 @@ static void __init pSeries_setup_arch(void)
    > if (firmware_has_feature(FW_FEATURE_LPAR)) {
    > vpa_init(boot_cpuid);
    >
    > - if (lppaca_shared_proc(get_lppaca()))
    > + if (lppaca_shared_proc(get_lppaca())) {
    > static_branch_enable(&shared_processor);
    > +#ifdef CONFIG_PARAVIRT_SPINLOCKS
    > + pv_spinlocks_init();
    > +#endif
    > + }

    We could avoid the ifdef with this I think?

    diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
    index 434615f1d761..6ec72282888d 100644
    --- a/arch/powerpc/include/asm/spinlock.h
    +++ b/arch/powerpc/include/asm/spinlock.h
    @@ -10,5 +10,9 @@
    #include <asm/simple_spinlock.h>
    #endif

    +#ifndef CONFIG_PARAVIRT_SPINLOCKS
    +static inline void pv_spinlocks_init(void) { }
    +#endif
    +
    #endif /* __KERNEL__ */
    #endif /* __ASM_SPINLOCK_H */

    cheers

    \
     
     \ /
      Last update: 2020-07-09 12:51    [W:4.117 / U:0.760 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site