Re: [PATCH v3 0/1] RDMA/rxe: Bump up default maximum values used via uverbs

[Bob Pearson <rpearsonhpe@gmail.com>]

On 9/12/21 7:50 PM, Shoaib Rao wrote:
> 
> On 8/6/21 6:49 AM, Jason Gunthorpe wrote:
>> On Wed, Aug 04, 2021 at 11:11:15PM -0700, Shoaib Rao wrote:
>>> Bob,
>>>
>>> Your third patch has an issue.
>>>
>>> In rxe_cq_post()
>>>
>>>
>>> addr = producer_addr(cq->queue, QUEUE_TYPE_TO_CLIENT);
>>>
>>> It should be
>>>
>>> addr = producer_addr(cq->queue, QUEUE_TYPE_FROM_CLIENT);
>>>
>>> After making this change, I have tested my patch and rping works.
>>>
>>> Bob can you please point me to the discussion which lead to the current
>>> changes, particularly the need for user barrier.
>>>
>>> Zhu can you apply Bob's 3 patches + the change above + my patch and report
>>> back. In my testing it works.
>> I'll expect Bob to resend
>>
>>     [for-next,v2,3/3] RDMA/rxe: Add memory barriers to kernel queues
>>
>> Jason
> 
> I have not seen a reply to this email thread. Has the issue been resolved and I missed it?
> 
> Shoaib
> 

Shoaib,

Thanks for this. I think I figured out what was causing the problem you tried to fix by
changing _TO_CLIENT to _FROM_CLIENT. That change isn't the whole solution.

The inline functions in rxe_queue.h all take a type parameter to let the compiler remove the switch
statement since the case is known at compile time. The types currently refer to the direction
of data flow in the queues from the point of view of the internals of the rxe driver. I.e.
for WQs data flows from the CLIENT to the DRIVER and for CQs the data flows to the CLIENT from
the DRIVER. This lets the routines in rxe_queue.h selectively use smp_load_acquire or
smp_store_release to 'protect' the queue indices owned by the client and private q->index for the
indices owned by the internals of the driver which is then copied to q->buf->producer/consumer_index.
The reason for this it that the driver can't trust the client in user space to not touch its data.

In rxe_cq.c where you made the change data is flowing to the client and the original type is the correct one. I believe the problem lies in rxe_verbs.c where verbs code manipulates the 'client'
end of the queues. This occurs in post_one_recv(), post_one_send(), rxe_poll_cq(), and rxe_peek_cq().
This code was using the same APIs as the driver internals which had two problems. First it had
the direction wrong in terms of which indices needed protection and worse it used the private
copies of the indices that should not be visible to clients. You put memory barriers in rxe_cq
that had the same effect ias putting the correct barriers in rxe_verbs.c.

I have fixed this by adding two new types for these verbs routines to use that put the correct
memory barriers on the correct indices. It is going to be resent as v4 of the patch series shortly.
I would like you to try it on rping and see of it cleans up what you were seeing.

Bob