Messages in this thread | | | Date | Tue, 31 Jan 2023 00:30:41 +0800 | Subject | Re: [RFC PATCH net-next v2 1/5] net/smc: introduce SMC-D loopback device | From | 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@linux.alibaba.com> >> --- > > 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@linux.alibaba.com/ and have amended this.
| |