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?
