Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release()

[Philipp Stanner]

On Wed, 2026-10-21 at 13:06 +0100, Gary Guo wrote:


[…]

> > +/**
> > + * set_bit_release - Set a bit with release ordering
> > + * @nr: Bit to set
> > + * @addr: Address to start counting from
> > + *
> > + * This is an atomic ordered operation (implied write memory barrier).
> > + */
> > +static __always_inline void set_bit_release(long nr, volatile unsigned long *addr)
> > +{
> > +	kcsan_wmb();
> > +	instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long));
> > +	arch_set_bit(nr, addr);
> 
> This is just wrong. There's no code here that gives you the release semantics.
> 
> kcsan_wmb() is just an annotation and does nothing. wmb is also not release
> semantics as it does not order LOAD->STORE.

What are the other functions here then even about?

/**
 * test_and_set_bit - Set a bit and return its old value
 * @nr: Bit to set
 * @addr: Address to count from
 *
 * This is an atomic fully-ordered operation (implied full memory barrier).
 */
static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
{
	kcsan_mb();
	instrument_atomic_read_write(addr + BIT_WORD(nr), sizeof(long));
	return arch_test_and_set_bit(nr, addr);
}

Where does the "full ordering" come from? I suppose implied barrier
then means it comes from the architecture's implementation. But what's
then the purpose of kcsan_XX ?

> Given that set_bit is a relaxed RMW atomic, you can implement set_bit_release
> with just
> 
>     smp_mb__before_atomic();
>     set_bit(nr, addr);
> 
> Which should be correct, but is too strong on some architectures (like RISC-V),
> becuase it will produce a full MB instead. So we can either add release barriers
> (hint: I really like it), or we can add arch_set_bit_release and implement it
> per arch; on RISC-V for example it can then be translated to amoor.rl (which is
> not being produced by any existing bit ops).c


All fine by me.


P.