Re: [PATCH for-next] RDMA/cma: Replace RMW with atomic bit-ops

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Jun 21, 2021 at 03:30:14PM +0000, Haakon Bugge wrote:
> 
> 
> > On 21 Jun 2021, at 16:35, Jason Gunthorpe <jgg@nvidia.com> wrote:
> > 
> > On Wed, Jun 16, 2021 at 04:35:53PM +0200, Håkon Bugge wrote:
> >> +#define BIT_ACCESS_FUNCTIONS(b)							\
> >> +	static inline void set_##b(unsigned long flags)				\
> >> +	{									\
> >> +		/* set_bit() does not imply a memory barrier */			\
> >> +		smp_mb__before_atomic();					\
> >> +		set_bit(b, &flags);						\
> >> +		/* set_bit() does not imply a memory barrier */			\
> >> +		smp_mb__after_atomic();						\
> >> +	}
> > 
> > This isn't needed, set_bit/test_bit are already atomic with
> > themselves, we should not need to introduce release semantics.
> 
> They are atomic, yes. But set_bit() does not provide a memory barrier (on x86_64, yes, but not as per the Linux definition of set_bit()).
> 
> We have (paraphrased):
> 
> 	id_priv->min_rnr_timer = min_rnr_timer;
> 	set_bit(MIN_RNR_TIMER_SET, &id_priv->flags);
> 
> Since set_bit() does not provide a memory barrier, another thread
> may observe the MIN_RNR_TIMER_SET bit in id_priv->flags, but the
> id_priv->min_rnr_timer value is not yet globally visible. Hence,
> IMHO, we need the memory barriers.

No, you need proper locks.

Jason