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

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

On 2025-01-20 11:28:41, Alexandra Winter wrote:
>
>
>On 17.01.25 14:00, Alexandra Winter wrote:
>> 
>> 
>> On 17.01.25 03:13, Dust Li wrote:
>>>>>> 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
>> 
>> 
>> Is there a specific example for enable/disable in the netdevice code, you have in mind?
>> Or do you mean in general how netdevice provides a common layer?
>> Yes, everything that is common for all devices should be provided by the network layer.
>
>
>Dust for some reason, you did not 'Reply-all':

Oh, sorry I didn't notice that

>Dust Li wrote:
>> I think dev_close()/dev_open() are the high-level APIs, while
>> ndo_stop()/ndo_open() are the underlying device operations that we
>> can reference.
>
>
>I hear you, it can be beneficial to have a way for upper layers to
>enable/disable an ism device.
>But all this is typically a tricky area. The device driver can also have
>reasons to enable/disable a device, then hardware could do that or even
>hotplug a device. Error recovery on different levels may want to run a
>disable/enable sequence as a reset, etc. And all this has potential for
>deadlocks.
>All this is rather trivial for ism-loopback, as there is not much of a
>lower layer.
>ism-vpci already has 'HW' / device driver configure on/off and device
>add/remove.
>For a future ism-virtio, the Hipervisor may want to add/remove devices.
>
>I wonder what could be the simplest definition of an enable/disable for
>the ism layer, that we can start with? More sophisticated functionality
>can always be added later.
>Maybe support for add/remove ism-device by the device driver is
>sufficient as  starting point?

I agree; this can be added later. For now, we can simply support
unregistering a device from the device driver. Which is already handled
by ism_dev_unregister() IIUC.

However, I believe we still need an API and the ability to enable or
disable ISM devices from the upper layer. For example, if we want to
disable a specific ISM device (such as the loopback device) in SMC, we
should not do so by disabling the loopback device at the device layer,
as it may also serve other clients beyond SMC.

Further more, I think removing the loopback from the loopback device
driver seems unnecessory ? Since we should support that from the upper
layer in the future.

Best regards,
Dust