Message-ID: <0f96574a-567e-495a-b815-6aef336f12e6@linux.ibm.com>
Date: Thu, 16 Jan 2025 17:17:33 +0100
From: Alexandra Winter <wintera@...ux.ibm.com>
To: Julian Ruess <julianr@...ux.ibm.com>, dust.li@...ux.alibaba.com,
        Wenjia Zhang <wenjia@...ux.ibm.com>, Jan Karcher <jaka@...ux.ibm.com>,
        Gerd Bayer <gbayer@...ux.ibm.com>, Halil Pasic <pasic@...ux.ibm.com>,
        "D. Wythe" <alibuda@...ux.alibaba.com>,
        Tony Lu <tonylu@...ux.alibaba.com>, Wen Gu <guwen@...ux.alibaba.com>,
        Peter Oberparleiter
 <oberpar@...ux.ibm.com>,
        David Miller <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
        Andrew Lunn <andrew+netdev@...n.ch>
Cc: Niklas Schnelle <schnelle@...ux.ibm.com>,
        Thorsten Winkler <twinkler@...ux.ibm.com>, netdev@...r.kernel.org,
        linux-s390@...r.kernel.org, Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev
 <agordeev@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>, Simon Horman <horms@...nel.org>
Subject: Re: [RFC net-next 0/7] Provide an ism layer



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.


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

Powered by blists - more mailing lists