On Mon, Jun 21, 2021 at 03:37:10PM +0000, Haakon Bugge wrote:
>
>
> > On 21 Jun 2021, at 17:32, Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> >
> > On Mon, Jun 21, 2021 at 03:30:14PM +0000, Haakon Bugge wrote:
> >>
> >>
> >>> 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.
> >
> > No, you need proper locks.
>
> Either will work in my opinion. If you prefer locking, I can do
> that. This is not performance critical.
Yes, use locks please
Jason
