| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6a31ab6b-2482-74c4-5a90-fffc1be3e8dc@linux.alibaba.com>
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