Re: [PATCH] wait_on_bit: add an acquire memory barrier

[Alan Stern <stern@rowland.harvard.edu>]

On Thu, Aug 25, 2022 at 05:03:40PM -0400, Mikulas Patocka wrote:
> From: Mikulas Patocka <mpatocka@redhat.com>
> 
> There are several places in the kernel where wait_on_bit is not followed
> by a memory barrier (for example, in drivers/md/dm-bufio.c:new_read). On
> architectures with weak memory ordering, it may happen that memory
> accesses that follow wait_on_bit are reordered before wait_on_bit and they
> may return invalid data.
> 
> Fix this class of bugs by introducing a new function "test_bit_acquire"
> that works like test_bit, but has acquire memory ordering semantics.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

...

> --- linux-2.6.orig/include/asm-generic/bitops/generic-non-atomic.h
> +++ linux-2.6/include/asm-generic/bitops/generic-non-atomic.h
> @@ -4,6 +4,7 @@
>  #define __ASM_GENERIC_BITOPS_GENERIC_NON_ATOMIC_H
>  
>  #include <linux/bits.h>
> +#include <asm/barrier.h>
>  
>  #ifndef _LINUX_BITOPS_H
>  #error only <linux/bitops.h> can be included directly
> @@ -127,6 +128,18 @@ generic_test_bit(unsigned long nr, const
>  	return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
>  }
>  
> +/**
> + * generic_test_bit - Determine whether a bit is set with acquire semantics

Trivial: Name in the kerneldoc isn't the same as the actual function name.

(Also, "with acquire semantics" is supposed to modify "Determine", not 
"is set" -- we don't set bits using acquire semantics.  You could change 
this to "Determine, with acquire semantics, whether a bit is set".)

Alan Stern

> + * @nr: bit number to test
> + * @addr: Address to start counting from
> + */
> +static __always_inline bool
> +generic_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr)
> +{
> +	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
> +	return 1UL & (smp_load_acquire(p) >> (nr & (BITS_PER_LONG-1)));
> +}