Date:   Fri, 29 Sep 2023 16:08:43 +0200
From:   Alexandra Winter <wintera@...ux.ibm.com>
To:     Wen Gu <guwen@...ux.alibaba.com>, Jan Karcher <jaka@...ux.ibm.com>,
        dust.li@...ux.alibaba.com, kgraul@...ux.ibm.com,
        wenjia@...ux.ibm.com, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com
Cc:     schnelle@...ux.ibm.com, gbayer@...ux.ibm.com, pasic@...ux.ibm.com,
        alibuda@...ux.alibaba.com, tonylu@...ux.alibaba.com,
        linux-s390@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v4 09/18] net/smc: introduce SMC-D loopback
 device



On 28.09.23 20:35, Wen Gu wrote:
> 
> 
> On 2023/9/28 11:16, Jan Karcher wrote:
>>
>>
>> On 26/09/2023 09:24, Alexandra Winter wrote:
>>>
>>>
>>> On 25.09.23 17:18, Dust Li wrote:
>>>>> Hello Wen Gu,
>>>>>
>>>>> thank you for adding the Kconfig, so the distributions can decide when to offer this feature.
>>>>>
>>>>> I propose you add some kind of runtime switch as well. Not every user who loads the SMC module
>>>>> may want to exploit smcd-loopback. Especially in native environements without containers.
>>>>>
>>>>> If no RoCE interfaces or no ISM interfaces exist, the respective handling is skipped in SMC.
>>>>> If loopback is always created unconditionally, there is no way to opt-out.
>>>> Hi Sandy,
>>>>
>>>> After talking to Wen Gu offline, I think the real issue here might be
>>>> we don't have an abstract layer in SMC, something like net/core/dev.c
>>>>
>>>> Without this, we cannot do:
>>>>
>>>> 1. Enable/disable those devices dynamically
>>>>     Currently, If we want to disable a SMC-R device to communicate with
>>>>     others, we need to refer to 'ip link set dev xxx down' to disable the
>>>>     netdevice, then Infiniband subsystem will notify SMC that the state of
>>>>     the IB device has changed. We cannot explicitly choose not to use some
>>>>     specific IB/RoCE devices without disable totally.
>>>>     If the loopback device need to support enable/disable itself, I
>>>>     think it might be better to enable this feature for all SMC devices.
>>>>
>>>> 2. Do statistics per device
>>>>     Now, we have to relay on IB/RoCE devices' hardware statistics to see
>>>>     how many packets/bytes we have sent through this device.
>>>>
>>>> Both the above issues get worse when the IB/RoCE device is shared by SMC
>>>> and userspace RDMA applications. If SMC-R and userspace RDMA applications
>>>> run at the same time, we can't enable the device to run userspace RDMA
>>>> applications while block it from running SMC. For statistics, we cannot
>>>> tell how many packets/bytes were sent by SMC and how many were sent by
>>>> userspace RDMA applications.
>>>>
>>>> So I think those are better to support in the SMC layer.
>>>>
>>>> Best regards!
>>>> Dust
>>>
>>> Thank you very much for your considerations. I also think a generic handling
>>> of these requirements in the smc layer would be best. Especially, if we want
>>> to add virtio-ism support soon. There we will face the same issues again.
>>> Let's hear what others think about this.
>>>
>>>
>>
>> Thanks you Sandy for bringing it up and Dust Li & Wen Gu for your thoughts.
>> I agree that such a runtime switch is needed and also that this generic handling would be good in the smc layer.
> 
> Right. runtime switch is necessary. I'm trying some ways to see which one is more suitable.
> 
> 
> As for implementing a abstract layer that capable of handling 1) enable/disable SMC usage of
> RDMA/ISM devices. 2) count packets/bytes of RDMA/ISM devices that generated/consumed by SMC,
> I believe it would be helpful, and IMHO its architecture may be:
> 
> ----------------------------------------------
>                   SMC protocol
>     (af_smc.c / smc_core.c / smc_clc.c ...)
> ----------------------------------------------
>           Abstract layer of SMC device
>       (define SMC device common operations)
> ----------------------------------------------
>   RDMA device |        (virt) ISM device
>   (smc_ib.c)  |   (smc_ism.c / smc_loopback.c)
> ----------------------------------------------
> 
> But I also believe this may require a lot of works and may be a long-term job.
> 

I like that concept a lot. If we can agree on a direction, we can define
meaningful pieces and approach it piece by piece.


> If only for the virtual ISM device, e.g.loopback-ism, I am considering adding it to the Linux
> device tree (/sys/devices/virtual/) to make it more 'device-like', and controlling its
> enable/disable and get the statistics through some files, such as
> echo 1 > /sys/devices/virtual/loopback-ism/alive
> or
> cat /sys/devices/virtual/loopback-ism/statistics/{rx|tx}_{bytes|packets}
> (similar to what tcp lo have in /sys/devices/virtual/net/lo)
> 
> What are your thoughts on it? Thanks.
> 

Makes sense to me, but I don't have too much experience in that area.
I have never seen an attribute called 'alive' before. 
I think attributes like 'power', 'enable' or 'online' are used for other device types.

> 
> -- 
> A little off-topic, it's currently China's National Day holiday, which lasts for about a week,
> so we are now on vacation. As a result, my responses might be a bit slower, but I will still
> make time to check/reply the mail and prepare for my new version. Thank you all very much!
> 
> Regards,
> Wen Gu

Next week is Germany's national holiday, so many of us are out as well.

Powered by blists - more mailing lists