On Mon, Sep 18, 2023 at 02:46:02PM +0200, Mirsad Todorovac wrote:
...
> Ah, I see. This is definitely not good. But I managed to fix and test the find_next_bit()
> family, but this seems that simply
>
> -------------------------------------------
> include/linux/xarray.h | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/include/linux/xarray.h b/include/linux/xarray.h
> index 1715fd322d62..89918b65b00d 100644
> --- a/include/linux/xarray.h
> +++ b/include/linux/xarray.h
> @@ -1718,14 +1718,6 @@ static inline unsigned int xas_find_chunk(struct xa_state *xas, bool advance,
> if (advance)
> offset++;
> - if (XA_CHUNK_SIZE == BITS_PER_LONG) {
> - if (offset < XA_CHUNK_SIZE) {
> - unsigned long data = READ_ONCE(*addr) & (~0UL << offset);
> - if (data)
> - return __ffs(data);
> - }
> - return XA_CHUNK_SIZE;
> - }
> return find_next_bit(addr, XA_CHUNK_SIZE, offset);
> }
This looks correct. As per my understanding, the removed part is the
1-word bitmap optimization for find_next_bit. If so, it's not needed
because find_next_bit() bears this optimization itself.
...
> --------------------------------------------------------
> lib/find_bit.c | 33 +++++++++++++++++----------------
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/lib/find_bit.c b/lib/find_bit.c
> index 32f99e9a670e..56244e4f744e 100644
> --- a/lib/find_bit.c
> +++ b/lib/find_bit.c
> @@ -18,6 +18,7 @@
> #include <linux/math.h>
> #include <linux/minmax.h>
> #include <linux/swab.h>
> +#include <asm/rwonce.h>
> /*
> * Common helper for find_bit() function family
> @@ -98,7 +99,7 @@ out: \
> */
> unsigned long _find_first_bit(const unsigned long *addr, unsigned long size)
> {
> - return FIND_FIRST_BIT(addr[idx], /* nop */, size);
> + return FIND_FIRST_BIT(READ_ONCE(addr[idx]), /* nop */, size);
> }
> EXPORT_SYMBOL(_find_first_bit);
> #endif
...
That doesn't look correct. READ_ONCE() implies that there's another
thread modifying the bitmap concurrently. This is not the true for
vast majority of bitmap API users, and I expect that forcing
READ_ONCE() would affect performance for them.
Bitmap functions, with a few rare exceptions like set_bit(), are not
thread-safe and require users to perform locking/synchronization where
needed.
If you really need READ_ONCE, I think it's better to implement a new
flavor of the function(s) separately, like:
find_first_bit_read_once()
Thanks,
Yury
