> On 21 Jun 2021, at 16:35, Jason Gunthorpe <jgg@xxxxxxxxxx> 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.
> Please split this to one patch per variable
>
> Every variable should be evalulated to decide if we should hold the
> spinlock instead of relying on atomics.
OK. Will do.
Thxs, Håkon
