Re: [PATCH net-next 09/15] net/smc: introduce loopback-ism statistics attributes

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

On 2024/2/23 22:13, Wenjia Zhang wrote:
> 
> 
> On 20.02.24 03:45, Wen Gu wrote:
>>
>>
>> On 2024/2/16 22:24, Wenjia Zhang wrote:
>>>
>>>
>>> On 11.01.24 13:00, Wen Gu wrote:
>>>> This introduces some statistics attributes of loopback-ism. They can be
>>>> read from /sys/devices/virtual/smc/loopback-ism/{xfer_tytes|dmbs_cnt}.
>>>>
>>>> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
>>>> ---
>>>>   net/smc/smc_loopback.c | 74 ++++++++++++++++++++++++++++++++++++++++++
>>>>   net/smc/smc_loopback.h | 22 +++++++++++++
>>>>   2 files changed, 96 insertions(+)
>>>>
>>>
>>> I've read the comments from Jiri and your answer. I can understand your thought. However, from the perspective of the 
>>> end user, it makes more sense to integetrate the stats info into 'smcd stats'. Otherwise, it would make users 
>>> confused to find out with which tool to check which statisic infornation. Sure, some improvement of the smc-tools is 
>>> also needed
>>
>> Thank you Wenjia.
>>
>> Let's draw an analogy with RDMA devices, which is used in SMC-R. If we want
>> to check the RNIC status or statistics, we may use rdma statistic command, or
>> ibv_devinfo command, or check file under /sys/class/infiniband/mlx5_0. These
>> provide details or attributes related to *devices*.
>>
>> Since s390 ISM can be used out of SMC, I guess it also has its own way (other
>> than smc-tools) to check the statistic?
>>
>> What we can see in smcr stats or smcd stats command is about statistic or
>> status of SMC *protocol* layer, such as DMB status, Tx/Rx, connections, fallbacks.
>>
>> If we put the underlying devices's statistics into smc-tools, should we also
>> put RNIC statistics or s390 ISM statistics into smcr stat or smcd stat? and
>> for each futures device that can be used by SMC-R/SMC-D, should we update them
>> into smcr stat and smcd stat? And the attributes of each devices may be different,
>> should we add entries in smcd stat for each of them?
>>
>> After considering the above things, I believe that the details of the underlying
>> device should not be exposed to smc(smc-tools). What do you think?
>>
>> Thanks!
>>
> That is a very good point. It really depends on how we understand *devices* and how we want to use it. The more we are 
> thinking, the more complicated the thing is getting. I'm trying to find accurate definitions on modeling virtual devices 
> hoping that would make things eaiser. Unfortunately, it is not easy. Finally, I found this article: 
> https://lwn.net/Articles/645810/ (Heads up! It is even from nine years ago, I'm not sure how reliable it is.) With the 
> insight of this article, I'm trying to summarize my thought:
> 
> It looks good to put the loopback-ism under the /sys/devices/virtual, especially according to the article
> "
> ... it is simply a place to put things that don't belong anywhere else.
> "

Yes, it can also be reflected from the implementation of get_device_parent():

static struct kobject *get_device_parent(struct device *dev,
					 struct device *parent)
{
<...>
		/*
		 * If we have no parent, we live in "virtual".
		 * Class-devices with a non class-device as parent, live
		 * in a "glue" directory to prevent namespace collisions.
		 */
		if (parent == NULL)
			parent_kobj = virtual_device_parent(dev);
		else if (parent->class && !dev->class->ns_type) {
			subsys_put(sp);
			return &parent->kobj;
		} else {
			parent_kobj = &parent->kobj;
		}
<...>
}

> However, in practice we use this in the term of simulated ism, which includes not only loopback-ism, but also other 
> ones. Thus, does it not make sense to classify all of them together? E.g. same bus (just a half-baked idea)
> 
> Then the following questions are comig up:
> - How should we organize them?
> - Should it show up in the smc_rnics?
> - How should it be seen from the perspective of the container?
> - If we see this loopback-ism as a *device*, should we not only put the device related information under the /sys? Thus, 
> dmbs_cnt seems ok, but xfer_tytes not. Besides, we have a field in smd stat naming "Data transmitted (Bytes)", which 
> should be suitable for this information.

Actually I created 'smc' class under /sys/devices/virtual just to place
loopback-ism, since it doesn't seem to belong to a certain class of device
and serves only SMC. Other 'smc devices', e.g. RDMA device, s390 ISM and
other Emulated-ISM like virtio-ism, all belong to a certain class or bus,
so I have no intention of putting them under the same path.

But now looks like that the 'smc' class and /sys/devices/virtual/smc path
will lead people to mistakenly think that there is a class of 'SMC devices',
but in fact these 'SMC devices' belongs to different classes or buses. They
can be used by SMC and any other users. So I think it is better to avoid
creating such 'smc' class.

Alternatively, after referring to other examples in the kernel, I think
another choice is to to put loopback-ism under /sys/devices/virtual/misc/,
for devices which can't fit in a specific class. What do you think?

Thanks a lot!