Message-ID: <ac35c3ce-f568-4d1d-b9a5-2e5c51bd4494@linux.alibaba.com>
Date: Sun, 16 Feb 2025 23:38:47 +0800
From: Wen Gu <guwen@...ux.alibaba.com>
To: Alexandra Winter <wintera@...ux.ibm.com>,
 Julian Ruess <julianr@...ux.ibm.com>, dust.li@...ux.alibaba.com,
 Wenjia Zhang <wenjia@...ux.ibm.com>, Jan Karcher <jaka@...ux.ibm.com>,
 Gerd Bayer <gbayer@...ux.ibm.com>, Halil Pasic <pasic@...ux.ibm.com>,
 "D. Wythe" <alibuda@...ux.alibaba.com>, Tony Lu <tonylu@...ux.alibaba.com>,
 Peter Oberparleiter <oberpar@...ux.ibm.com>,
 David Miller <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
 Andrew Lunn <andrew+netdev@...n.ch>
Cc: Niklas Schnelle <schnelle@...ux.ibm.com>,
 Thorsten Winkler <twinkler@...ux.ibm.com>, netdev@...r.kernel.org,
 linux-s390@...r.kernel.org, Heiko Carstens <hca@...ux.ibm.com>,
 Vasily Gorbik <gor@...ux.ibm.com>, Alexander Gordeev
 <agordeev@...ux.ibm.com>, Christian Borntraeger <borntraeger@...ux.ibm.com>,
 Sven Schnelle <svens@...ux.ibm.com>, Simon Horman <horms@...nel.org>
Subject: Re: [RFC net-next 0/7] Provide an ism layer



On 2025/1/17 00:17, Alexandra Winter wrote:
> 
> 
> On 16.01.25 12:55, Julian Ruess wrote:
>> On Thu Jan 16, 2025 at 10:32 AM CET, Dust Li wrote:
>>> On 2025-01-15 20:55:20, Alexandra Winter wrote:
>>>
>>> Hi Winter,
>>>
>>> I'm fully supportive of the refactor!
> 
> 
> Thank you very much Dust Li for joining the discussion.
> 
> 
>>> Interestingly, I developed a similar RFC code about a month ago while
>>> working on enhancing internal communication between guest and host
>>> systems.
> 
> 
> But you did not send that out, did you?
> I hope I did not overlook an earlier proposal by you.
> 
> 
> Here are some of my thoughts on the matter:
>>>
>>> Naming and Structure: I suggest we refer to it as SHD (Shared Memory
>>> Device) instead of ISM (Internal Shared Memory).
> 
> 
> So where does the 'H' come from? If you want to call it Shared Memory _D_evice?
> 
> 
> To my knowledge, a
>>> "Shared Memory Device" better encapsulates the functionality we're
>>> aiming to implement.
> 
> 
> Could you explain why that would be better?
> 'Internal Shared Memory' is supposed to be a bit of a counterpart to the
> Remote 'R' in RoCE. Not the greatest name, but it is used already by our ISM
> devices and by ism_loopback. So what is the benefit in changing it?
> 
> 
> It might be beneficial to place it under
>>> drivers/shd/ and register it as a new class under /sys/class/shd/. That
>>> said, my initial draft also adopted the ISM terminology for simplicity.
>>
>> I'm not sure if we really want to introduce a new name for
>> the already existing ISM device. For me, having two names
>> for the same thing just adds additional complexity.
>>
>> I would go for /sys/class/ism
>>
>>>
>>> Modular Approach: I've made the ism_loopback an independent kernel
>>> module since dynamic enable/disable functionality is not yet supported
>>> in SMC. Using insmod and rmmod for module management could provide the
>>> flexibility needed in practical scenarios.
> 
> 
> With this proposal ism_loopback is just another ism device and SMC-D will
> handle removal just like ism_client.remove(ism_dev) of other ism devices.
> 
> But I understand that net/smc/ism_loopback.c today does not provide enable/disable,
> which is a big disadvantage, I agree. The ism layer is prepared for dynamic
> removal by ism_dev_unregister(). In case of this RFC that would only happen
> in case of rmmod ism. Which should be improved.
> One way to do that would be a separate ism_loopback kernel module, like you say.
> Today ism_loopback is only 10k LOC, so I'd be fine with leaving it in the ism module.
> I also think it is a great way for testing any ISM client, so it has benefit for
> anybody using the ism module.
> Another way would be e.g. an 'enable' entry in the sysfs of the loopback device.
> (Once we agree if and how to represent ism devices in genera in sysfs).
> 
>>>
>>> Abstraction of ISM Device Details: I propose we abstract the ISM device
>>> details by providing SMC with helper functions. These functions could
>>> encapsulate ism->ops, making the implementation cleaner and more
>>> intuitive. This way, the struct ism_device would mainly serve its
>>> implementers, while the upper helper functions offer a streamlined
>>> interface for SMC.
>>>
>>> Structuring and Naming: I recommend embedding the structure of ism_ops
>>> directly within ism_dev rather than using a pointer. Additionally,
>>> renaming it to ism_device_ops could enhance clarity and consistency.
>>>
>>>
>>>> This RFC is about providing a generic shim layer between all kinds of
>>>> ism devices and all kinds of ism users.
>>>>
>>>> Benefits:
>>>> - Cleaner separation of ISM and SMC-D functionality
>>>> - simpler and less module dependencies
>>>> - Clear interface definition.
>>>> - Extendable for future devices and clients.
>>>
>>> Fully agree.
>>>
>>>>
> [...]
>>>>
>>>> Ideas for next steps:
>>>> ---------------------
>>>> - sysfs representation? e.g. as /sys/class/ism ?
>>>> - provide a full-fledged ism loopback interface
>>>>     (runtime enable/disable, sysfs device, ..)
>>>
>>> I think it's better if we can make this common for all ISM devices.
>>> but yeah, that shoud be the next step.
> 
> 
> The s390 ism_vpci devices are already backed by struct pci_dev.
> And I assume that would be represented in sysfs somehow like:
> /sys/class/ism/ism_vp0/device -> /sys/devices/<pci bus no>/<pci dev no>
> so there is an
> /sys/class/ism/<ism dev name>/device/enable entry already,
> because there is /sys/devices/<pci bus no>/<pci dev no>/enable today.
> 
> I remember Wen Gu's first proposal for ism_loopback had a device
> in /sys/devices/virtual/ and had an 'active' entry to enable/disable.
> Something like that could be linked to /sys/class/ism/ism_lo/device.
> 

Yes, the previous proposal can be refered to [1]. And the hierarchy
you mentioned makes sense to me.

[1] https://lore.kernel.org/netdev/20240111120036.109903-1-guwen@linux.alibaba.com/

> 
>>
>> I already have patches based on this series that introduce
>> /sys/class/ism and show ism-loopback as well as
>> s390/ism devices. I can send this soon.
>>
>>
>> Julian

Powered by blists - more mailing lists