Received-SPF: pass (google.com: domain of linux-kernel+bounces-71448-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <8a333c58-4a74-4a1c-9680-a0b9b4020a62@linux.alibaba.com>
Date: Mon, 19 Feb 2024 22:04:32 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next 00/15] net/smc: implement loopback-ism used by
 SMC-D
To: Alexandra Winter <wintera@linux.ibm.com>, wenjia@linux.ibm.com,
 hca@linux.ibm.com, gor@linux.ibm.com, agordeev@linux.ibm.com,
 davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
 pabeni@redhat.com, jaka@linux.ibm.com
Cc: borntraeger@linux.ibm.com, svens@linux.ibm.com,
 alibuda@linux.alibaba.com, tonylu@linux.alibaba.com,
 linux-s390@vger.kernel.org, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240111120036.109903-1-guwen@linux.alibaba.com>
 <83b9d600-339c-4c9f-860d-ab4539a0ae6b@linux.ibm.com>
From: Wen Gu <guwen@linux.alibaba.com>
In-Reply-To: <83b9d600-339c-4c9f-860d-ab4539a0ae6b@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 2024/2/6 20:18, Alexandra Winter wrote:
> 
> 
> On 11.01.24 13:00, Wen Gu wrote:
>> This patch set acts as the second part of the new version of [1] (The first
>> part can be referred from [2]), the updated things of this version are listed
>> at the end.
>>
>> # Background
>>
>> SMC-D is now used in IBM z with ISM function to optimize network interconnect
>> for intra-CPC communications. Inspired by this, we try to make SMC-D available
>> on the non-s390 architecture through a software-implemented virtual ISM device,
>> that is the loopback-ism device here, to accelerate inter-process or
>> inter-containers communication within the same OS instance.
> 
> 
> Hello Wen Gu,
> 
> thank you very much for this patchset. I have been looking at it a bit.
> I installed in on a testserver, but did not yet excercise the loopback-ism device.
> I want to continue investigations, but daily work interferes, so I thought I
> send you some comments now. So this is not at all a code review, but some
> thoughts and observations about the general concept.
> 

Thank you very much, Sandy.

> 
> In [1] there was a discussion about an abstraction layer between smc-d and the
> ism devices.
> I am not sure what you are proposing now, is it an smc-d feature or independent of smc?
> In 3/15 you say it is part of the SMC module, but then it has its own device entry.
> Didn't you want to use it for other things as well? Or is it an SMC-D only feature?
> Is it a device (Config help: "kind of virtual device")? Or an SMC-D feature?
> 

This patchset aims to propose an SMC feature, which is SMC-D loopback. The main work
to achieve this feature is to implement an Emulated-ISM, which is loopback-ism. The
loopback-ism is a 'built-in' dummy device of SMC and only serves SMC.

SMC-D protocol + 'built-in dummy device' (loopback-ism device) = SMC-D loopback feature.

To provide the runtime switch and statistics of loopback-ism, I need to find a sysfs
entry for it, since it doesn't belong to any class (e.g. pci_bus), I created an 'smc'
entry under /sys/devices/virtual/ and put loopback-ism under it.

The other SMC devices, such as RoCE, s390 ISM, virtio-ism will be in their own sysfs
entry, not under the /sys/devices/*virtual*/smc/.

The Config help is somewhat inaccurate. To be more precise, the SMC_LO config is used to
configure whether to enable this built-in dummy device for intra-OS communication.

> Will we have a class of ism devices (s390 ism, ism-loopback, virtio-ism)
> That share common properties (internal API?)
> and smc-d will work with any of those? > But they all can exist without smc ?! BTW: This is what we want for s390-ism.
> The client-registration interface [2] is currently the way to achieve this.
> But maybe we need a more general concept?
> 

I didn't mean to create a class to cover all the ISM devices. It is only for
loopback-ism. Because loopback-ism can not be classified, so I create an entry
under /sys/devices/virtual/.

> Maybe first a preparation patchset that introduces a class/ism
> Together with an improved API?
> In case you want to use ISM devices for other purposes as well..
> But then the whole picture of ism-loopback in one patchset (RFC?)
> has its benefits as well.
> 

Sorry for causing, I didn't mean to create a class to cover all the ISM devices.
They should be in their own sysfs entries (e.g. pci_bus), since they will be used
out of SMC. Only loopback-ism belongs only to SMC.

> 
> Other points that I noticed:
> 
> Naming: smc loopback, ism-loopback, loopback-ism ?
> 
> config: why not tristate? Why under net/smc?
> 

'SMC-D loopback' or 'SMC loopback' is used to indicate the feature or capability.
'loopback-ism' is the emulated-ISM device that 'SMC/SMC-D loopback' used.
('ism-loopback' doesn't seem to appear in my patchset)
If we all agree with these, I will check all the terms in the patch and unify them.

SMC_LO is used to configure whether SMC is allowed to use loopback-ism (CONFIG_SMC_LO),
it acts as a check in the code, so I defined it as a bool.
And loopback-ism only serves SMC-D loopback, as a feature of SMC, so the implementation
(net/smc/smc_loopback.{c|h}) is under net/smc.

> /sys/devices/virtual/smc  does not initially show up in my installation!!!
> root@t35lp50:/sys/devices/virtual/> ls
> 3270/  bdi/  block/  graphics/  iommu/  mem/  memory_tiering/  misc/  net/  tty/  vc/  vtconsole/  workqueue/
> root@t35lp50:/sys/devices/virtual/> ls smc/loopback-ism
> active  dmb_copy  dmbs_cnt  dmb_type  subsystem@  uevent  xfer_bytes
> root@t35lp50:/sys/devices/virtual/> ls
> 3270/  bdi/  block/  graphics/  iommu/  mem/  memory_tiering/  misc/  net/  smc/  tty/  vc/  vtconsole/  workqueue/
> Is that normal behaviour?
> 

/sys/devices/virtual/smc is created after SMC module initialization.
During the SMC module initialization, smc_loopback_init() is called, and the
/sys/devices/virtual/smc entry is created.

> You introduced a class/smc
> Maybe class/ism would be better?
> The other smc interfaces do not show up in class/smc!! Not so good
> 

Sorry for causing, I didn't mean to create a class to cover all the ISM devices.
They should be in their own sysfs entries (e.g. pci_bus), since they can be used
out of SMC. But loopback-ism is a SMC 'built-in' dummy device, it belongs only
to SMC and can't be classified to other entries.


> Why doesn't it show in smc_rnics?
> (Maybe some deficiency of smc_rnics?)
> 
smc_rnics can't be used on the arch other than s390.

# ./smc_rnics  -a
Error: s390/s390x supported only


> But then it shows in other smc-tools:
> root@t35lp50:/sys/> smcd device
> FID  Type  PCI-ID        PCHID  InUse  #LGs  PNET-ID
> 0000 0     loopback-ism  ffff   No        0
> 0029 ISM   0000:00:00.0  07c1   No        0  NET1
> Nice!
> 

Yes, this is did on patch 01/15.

Best regards,
Wen Gu

> Kind regards
> Sandy
> 
> 
> [1] https://lore.kernel.org/lkml/e3819550-7b10-4f9c-7347-dcf1f97b8e6b@linux.alibaba.com/
> [2] 89e7d2ba61b7 ("net/ism: Add new API for client registration")