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

[Wen Gu <guwen@linux.alibaba.com>]

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.