Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 16 Jul 2021 12:51:56 +0200
From:   Marco Elver <elver@google.com>
To:     Mark Rutland <mark.rutland@arm.com>
Cc:     linux-kernel@vger.kernel.org, Boqun Feng <boqun.feng@gmail.com>,
        Daniel Axtens <dja@axtens.net>, Ingo Molnar <mingo@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Will Deacon <will@kernel.org>
Subject: Re: [PATCH 5/5] locking/atomic: add generic arch_*() bitops
Message-ID: <YPFkzNvUFUfc1vVp@elver.google.com>
References: <20210713105253.7615-1-mark.rutland@arm.com>
 <20210713105253.7615-6-mark.rutland@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210713105253.7615-6-mark.rutland@arm.com>
User-Agent: Mutt/2.0.5 (2021-01-21)
Precedence: bulk

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.

Thanks,
-- Marco