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

Wen Gu
2023-01-30T16:31:03Z

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.