Re: [RFC PATCH 0/2] RDMA/rxe: Add RDMA Atomic Write operation

["yangx.jy@fujitsu.com" <yangx.jy@fujitsu.com>]

On 2022/1/4 23:17, Tom Talpey wrote:
>
> On 1/4/2022 4:28 AM, yangx.jy@fujitsu.com wrote:
>> On 2021/12/31 14:30, yangx.jy@fujitsu.com wrote:
>>> On 2021/12/31 5:42, Tom Talpey wrote:
>>>> On 12/30/2021 2:21 PM, Gromadzki, Tomasz wrote:
>>>>> 1)
>>>>>> rdma_post_atomic_writev(struct rdma_cm_id *id, void *context, struct
>>>>>> ibv_sge *sgl,
>>>>>>               int nsge, int flags, uint64_t remote_addr, uint32_t 
>>>>>> rkey)
>>>>> Do we need this API at all?
>>>>> Other atomic operations (compare_swap/add) do not use struct ibv_sge
>>>>> at all but have a dedicated place in
>>>>> struct ib_send_wr {
>>>>> ...
>>>>>           struct {
>>>>>               uint64_t    remote_addr;
>>>>>               uint64_t    compare_add;
>>>>>               uint64_t    swap;
>>>>>               uint32_t    rkey;
>>>>>           } atomic;
>>>>> ...
>>>>> }
>>>>>
>>>>> Would it be better to reuse (extend) any existing field?
>>>>> i.e.
>>>>>           struct {
>>>>>               uint64_t    remote_addr;
>>>>>               uint64_t    compare_add;
>>>>>               uint64_t    swap_write;
>>>>>               uint32_t    rkey;
>>>>>           } atomic;
>>>> Agreed. Passing the data to be written as an SGE is unnatural
>>>> since it is always exactly 64 bits. Tweaking the existing atomic
>>>> parameter block as Tomasz suggests is the best approach.
>>> Hi Tomasz, Tom
>>>
>>> Thanks for your quick reply.
>>>
>>> If we want to pass the 8-byte value by tweaking struct atomic on user
>>> space, why don't we
>>> tranfer the 8-byte value by ATOMIC Extended Transport Header 
>>> (AtomicETH)
>>> on kernel space?
>>> PS: IBTA defines that the 8-byte value is tranfered by RDMA Extended
>>> Transport Heade(RETH) + payload.
>>>
>>> Is it inconsistent to use struct atomic on user space and RETH + 
>>> payload
>>> on kernel space?
>> Hi Tomasz, Tom
>>
>> I think the following rules are right:
>> RDMA READ/WRITE should use struct rdma in libverbs and RETH + payload in
>> kernel.
>> RDMA Atomic should use struct atomic in libverbs and AtomicETH in 
>> kernel.
>>
>> According to IBTA's definition, RDMA Atomic Write should use struct rdma
>> in libibverbs.
>
> I don't quite understand this statement, the IBTA defines the protocol
> but does not define the API at such a level.
Hi Tom,

1) In kernel, current SoftRoCE copies the content of struct rdma to RETH 
and copies the content of struct atomic to AtomicETH.
2) IBTA defines that RDMA Atomic Write uses RETH + payload.
According to these two reasons, I perfer to tweak the existing struct rdma.

>
> I do however agree with this:
>
>> How about adding a member in struct rdma? for example:
>> struct {
>>       uint64_t    remote_addr;
>>       uint32_t    rkey;
>>       uint64_t    wr_value:
>> } rdma;
>
> Yes, that's what Tomasz and I were suggesting - a new template for the
> ATOMIC_WRITE request payload. The three fields are to be supplied by
> the verb consumer when posting the work request.

OK, I will update the patch in this way.

Best Regards,
Xiao Yang
>
> Tom.