Re: [PATCH for-next] RDMA/rxe: Guard mr state with spin lock

[Bob Pearson <rpearsonhpe@gmail.com>]

On 7/28/22 12:54, Bob Pearson wrote:
> On 7/28/22 11:42, Jason Gunthorpe wrote:
>> On Mon, Jul 25, 2022 at 03:01:15PM -0500, Bob Pearson wrote:
>>> Currently the rxe driver does not guard changes to the mr state
>>> against race conditions which can arise from races between
>>> local operations and remote invalidate operations. This patch
>>> adds a spinlock to the mr object and makes the state changes
>>> atomic.
>>
>> This doesn't make it atomic..
>>
>>> +	state = smp_load_acquire(&mr->state);
>>> +
>>>  	if (unlikely((type == RXE_LOOKUP_LOCAL && mr->lkey != key) ||
>>>  		     (type == RXE_LOOKUP_REMOTE && mr->rkey != key) ||
>>>  		     mr_pd(mr) != pd || (access && !(access & mr->access)) ||
>>> -		     mr->state != RXE_MR_STATE_VALID)) {
>>> +		     state != RXE_MR_STATE_VALID)) {
>>>  		rxe_put(mr);
>>
>> This is still just differently racy
>>
>> The whole point of invalidate is to say that when the invalidate
>> completion occurs there is absolutely no touching of the memory that
>> MR points to.
>>
>> I don't see how this acheives this like this. You need a proper lock
>> spanning from the lookup here until all the "dma" is completed.
>>
>> Jason
> 
> Interesting. Then things are in a bit of a mess. Before this patch of course there
> was nothing. And, rxe_resp.c currently looks up an mr from the rkey and saves it
> in the qp and then uses it for additional packets as required for e.g. rdma write
> operations. A local invalidate before a multipacket write finishes will have the wrong
> effect. It will continue to use the mr to perform the data copies. And the data copy
> routine does not validate the mr state. We would have to save the rkey instead and
> re-lookup the mr for each packet.
> 
> For a single packet we complete the dma in a single tasklet call. We would have a choice
> of holding a spinlock (for a fairly long time) or marking the mr as busy and deferring a
> local invalidate. A remote invalidate would fall between the packets of an rdma op.
> 
> Bob

Just rechecked all this and it isn't bad. The rxe responder only saves the mr during a single
packet processing cycle. Still not locked against races but at least no major surgery needed
for rdma writes.

Bob