On 22 Jun 2021, at 01:29, Jason Gunthorpe [off-list ref] wrote:

On Mon, Jun 21, 2021 at 03:37:10PM +0000, Haakon Bugge wrote:

quoted

quoted

On 21 Jun 2021, at 17:32, Jason Gunthorpe [off-list ref] wrote:

On Mon, Jun 21, 2021 at 03:30:14PM +0000, Haakon Bugge wrote:

quoted

quoted

On 21 Jun 2021, at 16:35, Jason Gunthorpe [off-list ref] wrote:

On Wed, Jun 16, 2021 at 04:35:53PM +0200, Håkon Bugge wrote:

quoted

+#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

With locking, there is no need for changing the bit fields to a flags variable and set/test_bit. But, for the fix to be complete, the locking must then be done all three places. Hence. I'll send one commit with locking.


Thxs, Håkon