lkml.org 
[lkml]   [2022]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [V3 09/11] tools: Add atomic_test_and_set_bit()
    On Wed, Aug 10, 2022, Peter Gonda wrote:
    > atomic_test_and_set_bit() allows for atomic bitmap usage from KVM
    > selftests.
    >
    > Signed-off-by: Peter Gonda <pgonda@google.com>
    > Suggested-by: Sean Christopherson <seanjc@google.com>
    > ---
    > tools/arch/x86/include/asm/atomic.h | 7 +++++++
    > tools/include/asm-generic/atomic-gcc.h | 15 +++++++++++++++
    > 2 files changed, 22 insertions(+)
    >
    > diff --git a/tools/arch/x86/include/asm/atomic.h b/tools/arch/x86/include/asm/atomic.h
    > index 1f5e26aae9fc..01cc27ec4520 100644
    > --- a/tools/arch/x86/include/asm/atomic.h
    > +++ b/tools/arch/x86/include/asm/atomic.h
    > @@ -8,6 +8,7 @@
    >
    > #define LOCK_PREFIX "\n\tlock; "
    >
    > +#include <asm/asm.h>
    > #include <asm/cmpxchg.h>
    >
    > /*
    > @@ -70,4 +71,10 @@ static __always_inline int atomic_cmpxchg(atomic_t *v, int old, int new)
    > return cmpxchg(&v->counter, old, new);
    > }
    >
    > +static inline int atomic_test_and_set_bit(long nr, unsigned long *addr)
    > +{
    > + GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), *addr, "Ir", nr, "%0", "c");
    > +

    Unnecessary newline.

    > +}
    > +
    > #endif /* _TOOLS_LINUX_ASM_X86_ATOMIC_H */
    > diff --git a/tools/include/asm-generic/atomic-gcc.h b/tools/include/asm-generic/atomic-gcc.h
    > index 4c1966f7c77a..8d9b2d1768bf 100644
    > --- a/tools/include/asm-generic/atomic-gcc.h
    > +++ b/tools/include/asm-generic/atomic-gcc.h
    > @@ -4,6 +4,7 @@
    >
    > #include <linux/compiler.h>
    > #include <linux/types.h>
    > +#include <linux/bitops.h>
    >
    > /*
    > * Atomic operations that C can't guarantee us. Useful for
    > @@ -69,4 +70,18 @@ static inline int atomic_cmpxchg(atomic_t *v, int oldval, int newval)
    > return cmpxchg(&(v)->counter, oldval, newval);
    > }
    > +static inline int atomic_test_and_set_bit(long nr, unsigned long *addr)
    > +{
    > + long old, val;
    > + unsigned long mask = BIT_MASK(nr);
    > +
    > + addr += BIT_WORD(nr);
    > + val = READ_ONCE(*addr);
    > + if (val & mask)
    > + return 1;

    Probably should drop the READ_ONCE() shortcut to stay consistent with the kernel
    proper.

    https://lore.kernel.org/all/CAHk-=wgSNiT5qJX53RHtWECsUiFq6d6VWYNAvu71ViOEan07yw@mail.gmail.com

    > +
    > + old = cmpxchg(addr, val, val & mask);

    This is wrong on two fronts: 1) cmpxchg() writes the entire new value, and 2) it
    fails if the old value is not an exact match with what's in memory. Bug #1 means
    that setting a bit will clear all existing bits, and bug #2 means that this will
    fail to set the bit if another atomic_test_and_set_bit() sneaks in between reading
    into "val" and doing the cmpxchg.

    And obviously dropping the READ_ONCE() above makes cmpxchg impossible (not a
    coincidence, it's simply the wrong operation to use).

    I believe what we want is:

    unsigned long mask = BIT_MASK(nr);
    long old;

    old = __sync_fetch_and_or(addr, mask);
    return !!(old & mask);

    > + return !!(old & mask);
    > +}
    > +
    > #endif /* __TOOLS_ASM_GENERIC_ATOMIC_H */
    > --
    > 2.37.1.559.g78731f0fdb-goog
    >

    \
     
     \ /
      Last update: 2022-08-16 16:27    [W:4.432 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site