Received-SPF: pass (google.com: domain of linux-kernel+bounces-26075-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 15 Jan 2024 15:11:16 +0100
From: Jiri Pirko <jiri@resnulli.us>
To: Wen Gu <guwen@linux.alibaba.com>
Cc: 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, 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
Subject: Re: [PATCH net-next 00/15] net/smc: implement loopback-ism used by
 SMC-D
Message-ID: <ZaU9BGuVo6ru38sy@nanopsycho>
References: <20240111120036.109903-1-guwen@linux.alibaba.com>
 <ZaAAJDiQ3bPGGRFK@nanopsycho>
 <2d8e7aeb-06da-434c-b7fe-6c1bd2e0674a@linux.alibaba.com>
 <ZaEB-b-W9Jt77T98@nanopsycho>
 <d7f9684e-4115-4b04-9e92-dbb841a52935@linux.alibaba.com>
 <ZaFfqZbgpVtVt4XM@nanopsycho>
 <895a0da3-bf6c-4e76-a672-d73481b4e58b@linux.alibaba.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <895a0da3-bf6c-4e76-a672-d73481b4e58b@linux.alibaba.com>

Sat, Jan 13, 2024 at 10:22:15AM CET, guwen@linux.alibaba.com wrote:
>
>
>On 2024/1/12 23:50, Jiri Pirko wrote:
>> Fri, Jan 12, 2024 at 01:32:14PM CET, guwen@linux.alibaba.com wrote:
>> > 
>> > 
>> > On 2024/1/12 17:10, Jiri Pirko wrote:
>> > > Fri, Jan 12, 2024 at 09:29:35AM CET, guwen@linux.alibaba.com wrote:
>> > > > 
>
><...>
>
>> > > > > > # Benchmark Test
>> > > > > > 
>> > > > > > * Test environments:
>> > > > > >         - VM with Intel Xeon Platinum 8 core 2.50GHz, 16 GiB mem.
>> > > > > >         - SMC sndbuf/DMB size 1MB.
>> > > > > >         - /sys/devices/virtual/smc/loopback-ism/dmb_copy is set to default 0,
>> > > > > >           which means sndbuf and DMB are merged and no data copied between them.
>> > > > > >         - /sys/devices/virtual/smc/loopback-ism/dmb_type is set to default 0,
>> > > > > 
>> > > > > Exposing any configuration knobs and statistics over sysfs for
>> > > > > softdevices does not look correct at all :/ Could you please avoid
>> > > > > sysfs?
>> > > > > 
>> > > > 
>> > > > In previous reviews and calls, we think loopback-ism needs to be more
>> > > > like a device and be visible under /sys/devices.
>> > > > 
>> > > > Would you mind explaining why using sysfs for loopback-ism is not correct?
>> > > > since I saw some other configurations or statistics exists under /sys/devices,
>> > > > e.g. /sys/devices/virtual/net/lo. Thank you!
>> > > 
>> > > You have smc_netlink.c exposing clear netlink api for the subsystem.
>> > > Can't you extend it to contain the configuration knobs and expose stats
>> > > instead of sysfs?
>> > > 
>> > 
>> > Thank you for the suggestion. I've also considered this approach.
>> > 
>> > But I didn't choose to extend the smc netlink because for now smc netlink
>> > are used for SMC protocol related attributes, for example:
>> > 
>> > SMC_NETLINK_GET_SYS_INFO:     SMC version, release, v2-capable..
>> > SMC_NETLINK_GET_LGR_SMC{R|D}: SMC-{R|D} link group inform (lgr id, lgr conn num, lgr role..)
>> > SMC_NETLINK_GET_LINK_SMCR:    SMC-R link inform (link id, link state, conn cnt..)
>> > SMC_NETLINK_GET_DEV_SMCD:     SMC-D device generic inform (user cnt, pci_fid, pci_chid, pci_vendor..)
>> > SMC_NETLINK_GET_DEV_SMCR:     SMC-R device generic inform (dev name, port pnet_id, port valid, port state..)
>> > SMC_NETLINK_GET_STATS:        SMC generic stats (RMB cnt, Tx size, Rx size, RMB size...)
>> > 
>> > And the knobs and stats in this patchset are loopback-ism device specific
>> > attributes, for example:
>> > 
>> > active:        loopback-ism runtime switch
>> > dmb_type:      type of DMB provided by loopback-ism
>> > dmb_copy:      support for DMB merge of loopback-ism
>> > xfer_bytes:    data transferred by loopback-ism
>> > dmbs_cnt:      DMB num provided by loopback-ism
>> > 
>> > The layer will be:
>> > 
>> >                +--------------------------------------+
>> >                |                                      |
>> >                |             SMC protocol             |
>> >                |  (attrs by netlink in smc_netlink.c) |
>> >                |                                      |
>> >                +--------------------------------------+
>> >              ------------------smcd_ops------------------
>> >      +---------------+  +---------------------+  +--------------+
>> >      | loopback-ism  |  |  s390 firmware ISM  |  | Possible     |
>> >      +---------------+  |                     |  | other        |
>> >      (attrs by sysfs    |                     |  | virtual ISM  |
>> >     in smc_loopback.c)  |                     |  |              |
>> >                         |                     |  |              |
>> >                         +---------------------+  +--------------+
>> 
>> So nest it:
>> SMC_NETLINK_BACKEND_GET_INFO
>> SMC_NETLINK_BACKEND_GET_STATS
>> ?
>> I mean, isn't it better to have the backend knobs and stats in one place
>> under same netlink commands and attributes than random sysfs path ?
>> 
>Thank you for suggestion.
>
>I think it is not about nesting or gathering knobs and stats. It is
>about not coupling underlying device details to upper layer SMC stack.
>
>From SMC perspective, it cares about the abstract operations defined
>by smcd_ops, regardless of which underlying devices provide these
>functions and how they provide. So IMO the details or configurations
>of underlying devices shouldn't be involved in SMC.

So you rather keep the device configuration and info exposed over random
sysfs files? Sorry, that makes not sense to me.


>
>Besides, the knobs and stats here are specific for loopback-ism device,
>they include runtime switch, buffer type choice and mode choice of
>loopback-ism (basically they won't change after being set once). The
>other kinds of devices used by SMC-D, e.g. s390 firmware ISM or other
>virtual ISMs have no similar things.

Okay, it is normal that different drivers implement different parts of
UAPI. No problem.


>
>So I prefer to keep the current solution instead of expanding upper
>layer SMC netlink.

Makes no sense to me. This is UAPI from 20 years ago. Is this a time
machine?


>
>Thanks,
>Wen Gu
>
>> 
>> 
>> > 
>> > So I choose to use current way to provide this lower layer loopback-ism
>> > device's attributes, restrict loopback-ism specific code to smc_loopback.c
>> > and try to make a clear layer architecture.
>> > 
>> > Thanks,
>> > Wen Gu
>> > > 
>> > > > 
>> > > > 
>> > > > 
>> > > > Thanks again,
>> > > > Wen Gu
>> > > > 
>> > > > > 
>> > > > > >           which means DMB is physically contiguous buffer.
>> > > > > > 
>> > > > > > * Test object:
>> > > > > >         - TCP: run on TCP loopback.
>> > > > > >         - SMC lo: run on SMC loopback device.
>> > > > > > 
>> > > > > > 1. ipc-benchmark (see [3])
>> > > > > > 
>> > > > > > - ./<foo> -c 1000000 -s 100
>> > > > > > 
>> > > > > >                               TCP                  SMC-lo
>> > > > > > Message
>> > > > > > rate (msg/s)              80636                  149515(+85.42%)
>> > > > > > 
>> > > > > > 2. sockperf
>> > > > > > 
>> > > > > > - serv: <smc_run> taskset -c <cpu> sockperf sr --tcp
>> > > > > > - clnt: <smc_run> taskset -c <cpu> sockperf { tp | pp } --tcp --msg-size={ 64000 for tp | 14 for pp } -i 127.0.0.1 -t 30
>> > > > > > 
>> > > > > >                               TCP                  SMC-lo
>> > > > > > Bandwidth(MBps)         4909.36                 8197.57(+66.98%)
>> > > > > > Latency(us)               6.098                   3.383(-44.52%)
>> > > > > > 
>> > > > > > 3. nginx/wrk
>> > > > > > 
>> > > > > > - serv: <smc_run> nginx
>> > > > > > - clnt: <smc_run> wrk -t 8 -c 1000 -d 30 http://127.0.0.1:80
>> > > > > > 
>> > > > > >                              TCP                   SMC-lo
>> > > > > > Requests/s           181685.74                246447.77(+35.65%)
>> > > > > > 
>> > > > > > 4. redis-benchmark
>> > > > > > 
>> > > > > > - serv: <smc_run> redis-server
>> > > > > > - clnt: <smc_run> redis-benchmark -h 127.0.0.1 -q -t set,get -n 400000 -c 200 -d 1024
>> > > > > > 
>> > > > > >                              TCP                   SMC-lo
>> > > > > > GET(Requests/s)       85855.34                118553.64(+38.09%)
>> > > > > > SET(Requests/s)       86824.40                125944.58(+45.06%)
>> > > > > > 
>> > > > > > 
>> > > > > > Change log:
>> > > > > > 
>> > > > > > v1->RFC:
>> > > > > > - Patch #9: merge rx_bytes and tx_bytes as xfer_bytes statistics:
>> > > > > >     /sys/devices/virtual/smc/loopback-ism/xfer_bytes
>> > > > > > - Patch #10: add support_dmb_nocopy operation to check if SMC-D device supports
>> > > > > >     merging sndbuf with peer DMB.
>> > > > > > - Patch #13 & #14: introduce loopback-ism device control of DMB memory type and
>> > > > > >     control of whether to merge sndbuf and DMB. They can be respectively set by:
>> > > > > >     /sys/devices/virtual/smc/loopback-ism/dmb_type
>> > > > > >     /sys/devices/virtual/smc/loopback-ism/dmb_copy
>> > > > > >     The motivation for these two control is that a performance bottleneck was
>> > > > > >     found when using vzalloced DMB and sndbuf is merged with DMB, and there are
>> > > > > >     many CPUs and CONFIG_HARDENED_USERCOPY is set [4]. The bottleneck is caused
>> > > > > >     by the lock contention in vmap_area_lock [5] which is involved in memcpy_from_msg()
>> > > > > >     or memcpy_to_msg(). Currently, Uladzislau Rezki is working on mitigating the
>> > > > > >     vmap lock contention [6]. It has significant effects, but using virtual memory
>> > > > > >     still has additional overhead compared to using physical memory.
>> > > > > >     So this new version provides controls of dmb_type and dmb_copy to suit
>> > > > > >     different scenarios.
>> > > > > > - Some minor changes and comments improvements.
>> > > > > > 
>> > > > > > RFC->old version([1]):
>> > > > > > Link: https://lore.kernel.org/netdev/1702214654-32069-1-git-send-email-guwen@linux.alibaba.com/
>> > > > > > - Patch #1: improve the loopback-ism dump, it shows as follows now:
>> > > > > >     # smcd d
>> > > > > >     FID  Type  PCI-ID        PCHID  InUse  #LGs  PNET-ID
>> > > > > >     0000 0     loopback-ism  ffff   No        0
>> > > > > > - Patch #3: introduce the smc_ism_set_v2_capable() helper and set
>> > > > > >     smc_ism_v2_capable when ISMv2 or virtual ISM is registered,
>> > > > > >     regardless of whether there is already a device in smcd device list.
>> > > > > > - Patch #3: loopback-ism will be added into /sys/devices/virtual/smc/loopback-ism/.
>> > > > > > - Patch #8: introduce the runtime switch /sys/devices/virtual/smc/loopback-ism/active
>> > > > > >     to activate or deactivate the loopback-ism.
>> > > > > > - Patch #9: introduce the statistics of loopback-ism by
>> > > > > >     /sys/devices/virtual/smc/loopback-ism/{{tx|rx}_tytes|dmbs_cnt}.
>> > > > > > - Some minor changes and comments improvements.
>> > > > > > 
>> > > > > > [1] https://lore.kernel.org/netdev/1695568613-125057-1-git-send-email-guwen@linux.alibaba.com/
>> > > > > > [2] https://lore.kernel.org/netdev/20231219142616.80697-1-guwen@linux.alibaba.com/
>> > > > > > [3] https://github.com/goldsborough/ipc-bench
>> > > > > > [4] https://lore.kernel.org/all/3189e342-c38f-6076-b730-19a6efd732a5@linux.alibaba.com/
>> > > > > > [5] https://lore.kernel.org/all/238e63cd-e0e8-4fbf-852f-bc4d5bc35d5a@linux.alibaba.com/
>> > > > > > [6] https://lore.kernel.org/all/20240102184633.748113-1-urezki@gmail.com/
>> > > > > > 
>> > > > > > Wen Gu (15):
>> > > > > >     net/smc: improve SMC-D device dump for virtual ISM
>> > > > > >     net/smc: decouple specialized struct from SMC-D DMB registration
>> > > > > >     net/smc: introduce virtual ISM device loopback-ism
>> > > > > >     net/smc: implement ID-related operations of loopback-ism
>> > > > > >     net/smc: implement some unsupported operations of loopback-ism
>> > > > > >     net/smc: implement DMB-related operations of loopback-ism
>> > > > > >     net/smc: register loopback-ism into SMC-D device list
>> > > > > >     net/smc: introduce loopback-ism runtime switch
>> > > > > >     net/smc: introduce loopback-ism statistics attributes
>> > > > > >     net/smc: add operations to merge sndbuf with peer DMB
>> > > > > >     net/smc: attach or detach ghost sndbuf to peer DMB
>> > > > > >     net/smc: adapt cursor update when sndbuf and peer DMB are merged
>> > > > > >     net/smc: introduce loopback-ism DMB type control
>> > > > > >     net/smc: introduce loopback-ism DMB data copy control
>> > > > > >     net/smc: implement DMB-merged operations of loopback-ism
>> > > > > > 
>> > > > > > drivers/s390/net/ism_drv.c |   2 +-
>> > > > > > include/net/smc.h          |   7 +-
>> > > > > > net/smc/Kconfig            |  13 +
>> > > > > > net/smc/Makefile           |   2 +-
>> > > > > > net/smc/af_smc.c           |  28 +-
>> > > > > > net/smc/smc_cdc.c          |  58 ++-
>> > > > > > net/smc/smc_cdc.h          |   1 +
>> > > > > > net/smc/smc_core.c         |  61 +++-
>> > > > > > net/smc/smc_core.h         |   1 +
>> > > > > > net/smc/smc_ism.c          |  71 +++-
>> > > > > > net/smc/smc_ism.h          |   5 +
>> > > > > > net/smc/smc_loopback.c     | 718 +++++++++++++++++++++++++++++++++++++
>> > > > > > net/smc/smc_loopback.h     |  88 +++++
>> > > > > > 13 files changed, 1026 insertions(+), 29 deletions(-)
>> > > > > > create mode 100644 net/smc/smc_loopback.c
>> > > > > > create mode 100644 net/smc/smc_loopback.h
>> > > > > > 
>> > > > > > -- 
>> > > > > > 2.32.0.3.g01195cf9f
>> > > > > > 
>> > > > > >