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

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

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!
>
> Interestingly, I developed a similar RFC code about a month ago while
> working on enhancing internal communication between guest and host
> systems. 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). To my knowledge, a
> "Shared Memory Device" better encapsulates the functionality we're
> aiming to implement. 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.
>
> 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.
>
> >
> >Request for comments:
> >---------------------
> >Any comments are welcome, but I am aware that this series needs more work.
> >It may not be worth your time to do an in-depth review of the details, I am
> >looking for feedback on the general idea.
> >I am mostly interested in your thoughts and recommendations about the general
> >concept, the location of net/ism, the structure of include/linux/ism.h, the
> >KConfig and makefiles.
> >
> >Status of this RFC:
> >-------------------
> >This is a very early RFC to ask you for comments on this general idea.
> >The RFC does not fullfill all criteria required for a patchset.
> >The whole set compiles and runs, but I did not try all combinations of
> >module and built-in yet. I did not check for checkpatch or any other checkers.
> >Also I have only done very rudimentary quick tests of SMC-D. More testing is
> >required.
> >
> >Background / Status quo:
> >------------------------
> >Currently s390 hardware provides virtual PCI ISM devices (ism_vpci). Their
> >driver is in drivers/s390/net/ism_drv.c. The main user is SMC-D (net/smc).
> >ism_vpci driver offers a client interface so other users/protocols
> >can also use them, but it is still heavily intermingled with the smc code.
> >Namely, the ISM vPCI module cannot be used without the SMC module, which
> >feels artificial.
> >
> >The ISM concept is being extended:
> >[1] proposed an ISM loopback interface (ism_lo), that can be used on non-s390
> >architectures (e.g. between containers or to test SMC-D). A minimal implementation
> >went upstream with [2]: ism_lo currently is a part of the smc protocol and rather
> >hidden.
> >
> >[3] proposed a virtio definition of ISM (ism_virtio) that can be used between
> >kvm guests.
> >
> >We will shortly send an RFC for an ISM client that uses ISM as transport for TTY.
> >
> >Concept:
> >--------
> >Create a shim layer in net/ism that contains common definitions and code for
> >all ism devices and all ism clients.
> >Any device or client module only needs to depend on this ism layer module and
> >any device or client code only needs to include the definitions in
> >include/linux/ism.h
> >
> >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.

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