Re: [PATCH net-next 0/4] Make SMC-R can work with rxe devices

[Jan Karcher <jaka@linux.ibm.com>]

On 24/08/2024 12:04, liujian (CE) wrote:
> 
> 
> 在 2024/8/21 9:03, Dust Li 写道:
>> On 2024-08-20 15:16:57, Jan Karcher wrote:
>>>
>>>
>>> On 09/08/2024 10:31, Liu Jian wrote:
>>>> Make SMC-R can work with rxe devices. This allows us to easily test and
>>>> learn the SMC-R protocol without relying on a physical RoCE NIC.
>>>
>>> Hi Liu,
>>>
>>> sorry for taking quite some time to answer.
>>>
>>> Looking into this i cannot accept this series at the given point of 
>>> time.
>>>
>>> FWIU, RXE is mainly for testing and development and i agree that it 
>>> would be
>>> a nice thing to have for SMC-R.
>>> The problem is that there is no clean layer for different RoCE devices
>>> currently. Adding RXE to it works but isn't clean.
>>
>> Hi jan,
>>
>>> Also we have no way to do a "test" build which would have such a device
>>> supported and a "prod" build which would not support it.
>>  > I don't quite understand what you mean here, Maybe I missed 
>> something ?
>> IIUC, we can control whether to use RXE by simpling insmod or rmmod 
>> rdma_rxe.ko

Hi,

Yes that enables RXE in general, but not the use of RXE in SMC.

>>
> Yes, in the "prod" environment, we can completely turn off CONFIG_RDMA_RXE.

Same as above + this is a compile time switch that is enabled for 
distros like rh. Simply disabling it won't work here.

> 
>> I believe having RXE support is beneficial for testing, especially in
>> simple physical networking setups where many corner cases are unlikely
>> to occur. By using RXE, we can easily configure unusual scenarios with
>> the existing iptables/netfilter infrastructure to simulate real-world
>> situations, such as packet dropping or network retransmission. This
>> approach can be advantageous for finding hidden bugs.
>>
> Yes, one of my main original intentions was to make testing smc-r 
> easier. This change is relatively simple, mainly patch2 and patch4, and 
> there are no logical changes.

I agree with you. It would be beneficial for testing.
This is not a never, this is a not right now.

If you want to push this forward as something you need now, feel free to 
encapsulate it and introduce a vendor specific experimental option as 
defined in the v2.1 protocol version [1] for it. This would be 
compromise for me at the current time.

Thanks
- Jan

[1] 
https://www.ibm.com/support/pages/system/files/inline-files/IBM%20Shared%20Memory%20Communications%20Version%202.1%20Emulated-ISM_0.pdf

>> Best regards,
>> Dust
>>
>>
>>>
>>> Please give us time to investigate how to solve this in a neat way 
>>> without
>>> building up to much technical debt.
>>>
>>> Thanks for your contribution and making us aware of this area of 
>>> improvment.
>>> - Jan
>>>
>>>>
>>>> Liu Jian (4):
>>>>     rdma/device: export ib_device_get_netdev()
>>>>     net/smc: use ib_device_get_netdev() helper to get netdev info
>>>>     net/smc: fix one NULL pointer dereference in 
>>>> smc_ib_is_sg_need_sync()
>>>>     RDMA/rxe: Set queue pair cur_qp_state when being queried
>>>>
>>>>    drivers/infiniband/core/core_priv.h   |  3 ---
>>>>    drivers/infiniband/core/device.c      |  1 +
>>>>    drivers/infiniband/sw/rxe/rxe_verbs.c |  2 ++
>>>>    include/rdma/ib_verbs.h               |  2 ++
>>>>    net/smc/smc_ib.c                      | 10 +++++-----
>>>>    net/smc/smc_pnet.c                    |  6 +-----
>>>>    6 files changed, 11 insertions(+), 13 deletions(-)
>>>>