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

["Julian Ruess" <julianr@linux.ibm.com>]

On Thu Jan 16, 2025 at 5:17 PM CET, 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.

My current implementation represents the devices as following
in '/sys/class/ism':

ism_lo -> ../../devices/virtual/ism/ism_lo
lismvpci0 -> ../../devices/pci0124:00/0124:00:00.0/ism/ismvpci0

The driver is repsonsible for the naming of its devices.

And yes, because the s390 ism_vpci is backed by a PCI device,
'/sys/class/ism/ismvpci0/device/enable' exists.

I think we could implement a device attribute for ism_lo
to implement this functionality. I already have a 
device attribute implemented in ism_main
to access the gid of each ISM device. This leads
to the following sysfs entries:

'/sys/class/ism/ism_lo/gid'
'/sys/class/ism/ismvpci0/gid'

Julian

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