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
