Re: [RFC net-next 0/7] Provide an ism layer

Wen Gu
2025-02-16T15:39:12Z

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