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

[Dust Li <dust.li@linux.alibaba.com>]

On 2025-01-16 17:17:33, 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.

No, I just did a POC and didn't find the time to improve it.
So I think we can go on with your version.

>
>
>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?

Oh, I was trying to refer to SHM(Share memory file in the userspace, see man
shm_open(3)). SMD is also OK.

>
>
>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?

I believe that if we are going to separate and refine the code, and add
a common subsystem, we should choose the most appropriate name.

In my opinion, "ISM" doesn’t quite capture what the device provides.
Since we’re adding a "Device" that enables different entities (such as
processes or VMs) to perform shared memory communication, I think a more
fitting name would be better. If you have any alternative suggestions,
I’m open to them.


>
>
>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 believe that if we are going to rename it, there should be no
reference to "ISM" in this subsystem. IBM's PCI ISM can retain that
name, as it is an implementation of the Shared Memory device (assuming
we adopt that name).

>> 
>> 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).

This works for me as well. I think it would be better to implement this
within the common ISM layer, rather than duplicating the code in each
device. Similar to how it's done in netdevice.

Best regards,
Dust