Messages in this thread | | | Date | Fri, 16 Jul 2021 13:21:14 +0100 | From | Mark Rutland <> | Subject | Re: [PATCH 5/5] locking/atomic: add generic arch_*() bitops |
| |
On Fri, Jul 16, 2021 at 12:51:56PM +0200, Marco Elver wrote: > On Tue, Jul 13, 2021 at 11:52AM +0100, Mark Rutland wrote: > [...] > > As the generic non-atomic bitops use plain accesses, these will be > > implicitly instrumented unless they are inlined into noinstr functions > > (which is similar to arch_atomic*_read() when based on READ_ONCE()). > > The wrappers are modified so that where the underlying arch_*() function > > uses a plain access, no explicit instrumentation is added, as this is > > redundant and could result in confusing reports. > [...] > > diff --git a/include/asm-generic/bitops/instrumented-non-atomic.h b/include/asm-generic/bitops/instrumented-non-atomic.h > > index 37363d570b9b..e6c1540965d6 100644 > > --- a/include/asm-generic/bitops/instrumented-non-atomic.h > > +++ b/include/asm-generic/bitops/instrumented-non-atomic.h > [...] > > @@ -131,7 +137,8 @@ static inline bool __test_and_change_bit(long nr, volatile unsigned long *addr) > > */ > > static inline bool test_bit(long nr, const volatile unsigned long *addr) > > { > > - instrument_atomic_read(addr + BIT_WORD(nr), sizeof(long)); > > + if (!__is_defined(arch_test_bit_uses_plain_access)) > > + instrument_atomic_read(addr + BIT_WORD(nr), sizeof(long)); > > return arch_test_bit(nr, addr); > > } > [...] > > diff --git a/include/asm-generic/bitops/non-atomic.h b/include/asm-generic/bitops/non-atomic.h > > index 7e10c4b50c5d..c8149cd52730 100644 > > --- a/include/asm-generic/bitops/non-atomic.h > > +++ b/include/asm-generic/bitops/non-atomic.h > > @@ -5,7 +5,7 @@ > > #include <asm/types.h> > [...] > > /** > > - * test_bit - Determine whether a bit is set > > + * arch_test_bit - Determine whether a bit is set > > * @nr: bit number to test > > * @addr: Address to start counting from > > */ > > -static inline int test_bit(int nr, const volatile unsigned long *addr) > > +static __always_inline int > > +arch_test_bit(int nr, const volatile unsigned long *addr) > > { > > return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1))); > > } > > +#define arch_test_bit_uses_plain_access > > + > > +#include <asm-generic/bitops/instrumented-non-atomic.h> > > Why not just: > > #define test_bit arch_test_bit > > and similar for the ones that use plain accesses? > > And not include instrumented-non-atomic.h here nor do the > __is_defined(*_uses_plain_access) change to the instrumented header, > which seems to overcomplicate things as it effectively just aliases the > non-arch_ name to the arch_ name if *_uses_plain_access is defined.
I'd done that to still permit the compiler to out-of-line the non-arch forms if it wanted to. That said, I see that for the atomics we forced those to be __always_inline anyway, so maybe that's not a concern.
I'm happy either way.
Thanks, Mark.
| |