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

[Haakon Bugge <haakon.bugge@oracle.com>]

> On 22 Jun 2021, at 09:34, Haakon Bugge <haakon.bugge@oracle.com> wrote:
> 
> 
> 
>> On 22 Jun 2021, at 01:29, Jason Gunthorpe <jgg@nvidia.com> wrote:
>> 
>> On Mon, Jun 21, 2021 at 03:37:10PM +0000, Haakon Bugge wrote:
>>> 
>>> 
>>>> On 21 Jun 2021, at 17:32, Jason Gunthorpe <jgg@nvidia.com> wrote:
>>>> 
>>>> 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.
>>> 
>>> 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.

Adding to that, I will make a series of this and include ("RDMA/cma: Remove unnecessary INIT->INIT transition") here. The reason is that the transitions of the QP state of a connected QP is not protected by a lock when called from rdma_create_qp() [what protects the cm_id from being destroyed whilst rdma_create_qp() executes?].

With commit ("RDMA/cma: Remove unnecessary INIT->INIT transition"), the QP state transitions on a connected QP is removed from rdma_create_qp(), and when called from  cma_modify_qp_rtr(), the qp_lock is held, which fits well with fixing the unprotected RMW to the bitfields.


Thxs, Håkon


> 
> 
> Thxs, Håkon