Re: [RFC PATCH net-next v2 1/5] net/smc: introduce SMC-D loopback device

[Wen Gu]

On 2023/1/20 00:25, Alexandra Winter wrote:
> On 20.12.22 04:21, Wen Gu wrote:
> > This patch introduces a kind of loopback device for SMC-D, thus
> > enabling the SMC communication between two local sockets in one
> > kernel.
> >
> > The loopback device supports basic capabilities defined by SMC-D,
> > including registering DMB, unregistering DMB and moving data.
> >
> > Considering that there is no ism device on other servers expect
> > IBM z13,
>
> Please use the wording 'on other architectures except s390'.
> That is how IBM Z is referred to in the Linux kernel.

Thanks, will use words consistent with this.

> > the loopback device can be used as a dummy device to
> > test SMC-D logic for the broad community.
> >
> > Signed-off-by: Wen Gu <guwen@xxxxxxxxxxxxxxxxx>
> > ---
>
> Hello Wen Gu,
>
> as the general design discussions are ongoing, I didn't
> do a thorough review. But here are some general remarks
> that you may want to consider for future versions.
> I would propose to add a module parameter (default off) to enable
> SMC-D loopback.

OK, will add a module parameter in the future version.

> >   include/net/smc.h      |   1 +
> >   net/smc/Makefile       |   2 +-
> >   net/smc/af_smc.c       |  12 ++-
> >   net/smc/smc_cdc.c      |   6 ++
> >   net/smc/smc_cdc.h      |   1 +
> >   net/smc/smc_loopback.c | 282 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   net/smc/smc_loopback.h |  59 +++++++++++
> >   7 files changed, 361 insertions(+), 2 deletions(-)
> >   create mode 100644 net/smc/smc_loopback.c
> >   create mode 100644 net/smc/smc_loopback.h
>
> I am not convinced that this warrants a separate file.

IMHO, the dummy device used by smcd loopback is corresponding to ISM device.
So I put the dummy device implementation in smc_loopback.c alone, imitating
drivers/s390/net/ism_drv.c. I think it maybe clearer to do so.

> [...]
> > +}
> > +
> > +static int lo_add_vlan_id(struct smcd_dev *smcd, u64 vlan_id)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int lo_del_vlan_id(struct smcd_dev *smcd, u64 vlan_id)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int lo_set_vlan_required(struct smcd_dev *smcd)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int lo_reset_vlan_required(struct smcd_dev *smcd)
> > +{
> > +	return 0;
> > +}
>
> The VLAN functions are only required for SMC-Dv1
> Seems you want to provide v1 support for loopback?
> May be nice for testing v1 VLAN support.
> But then you need proper VLAN support.
Based on the current discussion, I tend to only provide v2 support for loopback
since v2 is the general trend. So I will fix this in the future version.

> [...]
> > +
> > +static u8 *lo_get_system_eid(void)
> > +{
> > +	return &LO_SYSTEM_EID.seid_string[0];
> > +}
> SEID is for the whole system not per device.
> We probably need to register a different function
> for each architecture.

Yes, I agree.


> > +
> > +static u16 lo_get_chid(struct smcd_dev *smcd)
> > +{
> > +	return 0;
> > +}
> > +
>
> Shouldn't this return 0xFFFF in your current concept?

Yes, this should return 0xFFFF.
I supplemented a patch as attachment in this earlier reply:
https://lore.kernel.org/netdev/b25f56de-7913-2a56-950f-dfe0defd6936@xxxxxxxxxxxxxxxxx/
and have amended this.