Date:   Wed, 8 Feb 2023 19:59:23 +0800
From:   Wen Gu <guwen@...ux.alibaba.com>
To:     Wenjia Zhang <wenjia@...ux.ibm.com>,
        Jan Karcher <jaka@...ux.ibm.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>
Cc:     netdev@...r.kernel.org, linux-s390@...r.kernel.org,
        Heiko Carstens <hca@...ux.ibm.com>,
        Alexandra Winter <wintera@...ux.ibm.com>,
        Thorsten Winkler <twinkler@...ux.ibm.com>,
        Stefan Raspl <raspl@...ux.ibm.com>,
        Karsten Graul <kgraul@...ux.ibm.com>,
        Nils Hoppmann <niho@...ux.ibm.com>,
        Halil Pasic <pasic@...ux.ibm.com>,
        Tony Lu <tonylu@...ux.alibaba.com>
Subject: Re: [net-next v2 0/8] drivers/s390/net/ism: Add generalized interface



On 2023/2/6 18:47, Wenjia Zhang wrote:
> 
> 
> On 02.02.23 14:53, Wen Gu wrote:
>>
>>
>> On 2023/1/24 02:17, Jan Karcher wrote:
>>
>>> Previously, there was no clean separation between SMC-D code and the ISM
>>> device driver.This patch series addresses the situation to make ISM available
>>> for uses outside of SMC-D.
>>> In detail: SMC-D offers an interface via struct smcd_ops, which only the
>>> ISM module implements so far. However, there is no real separation between
>>> the smcd and ism modules, which starts right with the ISM device
>>> initialization, which calls directly into the SMC-D code.
>>> This patch series introduces a new API in the ISM module, which allows
>>> registration of arbitrary clients via include/linux/ism.h: struct ism_client.
>>> Furthermore, it introduces a "pure" struct ism_dev (i.e. getting rid of
>>> dependencies on SMC-D in the device structure), and adds a number of API
>>> calls for data transfers via ISM (see ism_register_dmb() & friends).
>>> Still, the ISM module implements the SMC-D API, and therefore has a number
>>> of internal helper functions for that matter.
>>> Note that the ISM API is consciously kept thin for now (as compared to the
>>> SMC-D API calls), as a number of API calls are only used with SMC-D and
>>> hardly have any meaningful usage beyond SMC-D, e.g. the VLAN-related calls.
>>>
>>
>> Hi,
>>
>> Thanks for the great work!
>>
>> We are tring to adapt loopback and virtio-ism device into SMC-D based on the new
>> interface and want to confirm something. (cc: Alexandra Winter, Jan Karcher, Wenjia Zhang)
>>
>>  From my understanding, this patch set is from the perspective of ISM device driver
>> and aims to make ISM device not only used by SMC-D, which is great!
>>
>> But from the perspective of SMC, SMC-D protocol now binds with the helper in
>> smc_ism.c (smc_ism_* helper) and some part of smc_ism.c and smcd_ops seems to be
>> dedicated to only serve ISM device.
>>
>> For example,
>>
>> - The input param of smcd_register_dev() and smcd_unregister_dev() is ism_dev,
>>    instead of abstract smcd_dev like before.
>>
>> - the smcd->ops->register_dmb has param of ism_client, exposing specific underlay.
>>
>> So I want to confirm that, which of the following is our future direction of the
>> SMC-D device expansion?
>>
>> (1) All extended devices (eg. virtio-ism and loopback) are ISM devices and SMC-D
>>      only supports ISM type device.
>>
>>      SMC-D protocol -> smc_ism_* helper in smc_ism.c -> only ISM device.
>>
>>      Future extended device must under the definition of ism_dev, in order to share
>>      the ism-specific helper in smc_ism.c (such as smcd_register_dev(), smcd_ops->register_dmbs..).
>>
>>      With this design intention, futher extended SMC-D used device may be like:
>>
>>                      +---------------------+
>>                      |    SMC-D protocol   |
>>                      +---------------------+
>>                        | current helper in|
>>                        |    smc_ism.c     |
>>           +--------------------------------------------+
>>           |              Broad ISM device              |
>>           |             defined as ism_dev             |
>>           |  +----------+ +------------+ +----------+  |
>>           |  | s390 ISM | | virtio-ism | | loopback |  |
>>           |  +----------+ +------------+ +----------+  |
>>           +--------------------------------------------+
>>
>> (2) All extended devices (eg. virtio-ism and loopback) are abstracted as smcd_dev and
>>      SMC-D protocol use the abstracted capabilities.
>>
>>      SMC-D does not care about the type of the underlying device, and only focus on the
>>      capabilities provided by smcd_dev.
>>
>>      SMC-D protocol use a kind of general helpers, which only invoking smcd_dev->ops,
>>      without underlay device exposed. Just like most of helpers now in smc_ism.c, such as
>>      smc_ism_cantalk()/smc_ism_get_chid()/smc_ism_set_conn()..
>>
>>      With this design intention, futher extended SMC-D used device should be like:
>>
>>                       +----------------------+
>>                       |     SMC-D protocol   |
>>                       +----------------------+
>>                        |   general helper   |
>>                        |invoke smcd_dev->ops|
>>                        | hiding underlay dev|
>>             +-----------+  +------------+  +----------+
>>             | smc_ism.c |  | smc_vism.c |  | smc_lo.c |
>>             |           |  |            |  |          |
>>             | s390 ISM  |  | virtio-ism |  | loopback |
>>             |  device   |  |   device   |  |  device  |
>>             +-----------+  +------------+  +----------+
>>
>> IMHO, (2) is more clean and beneficial to the flexible expansion of SMC-D devices, with no
>> underlay devices exposed.
>>
>> So (2) should be our target. Do you agree? :)
>>
>> If so, maybe we should make some part of helpers or ops of SMC-D device (such as smcd_register/unregister_dev
>> and smcd->ops->register_dmb) more generic？
>>
>> Thanks,
>> Wen Gu
> 
> Currently we tend a bit more towards the first solution. The reasoning behind it is the following:
> If we create a full blown interface, we would have an own file for every new device which on the one hand is clean, but 
> on the other hand raises the risk of duplicated code.
> So if we go down that path (2) we have to take care that we avoid duplicated code.
> 
> In the context of the currently discussed changes this could mean:
> - ISM is the only device right now using indirect copy,
> - lo & vism should (AFAIU) copy directly.
> 
> As you may see this leaves us with the big question: How much abstraction is enough vs. when do we go overboard?

I see.

I can understand the difficulty in designing proper abstract and generic helpers, especially when the user's
(lo and vism) implementation code has not been finalized.

I think we can keep optimizing this based on the update of smcd-lo and smcd-vism patches (which will be sent
out as soon as possible).

Thanks,
Wen

Powered by blists - more mailing lists