Re: [RFC PATCH net-next v3 9/9] net/smc: Add interface implementation of loopback device

Re: [RFC PATCH net-next v3 9/9] net/smc: Add interface implementation of loopback device

[Wen Gu]

On 2023/2/22 22:09, Hillf Danton wrote:

> On Thu, 16 Feb 2023 00:18:25 +0800 Wen Gu <guwen@linux.alibaba.com>
>> @@ -124,12 +126,76 @@ static int smc_lo_unregister_dmb(struct smcd_dev *smcd, struct smcd_dmb *dmb)
>>   	write_unlock(&ldev->dmb_ht_lock);
>>   
>>   	clear_bit(dmb_node->sba_idx, ldev->sba_idx_mask);
>> +
>> +	/* wait for dmb refcnt to be 0 */
>> +	if (!refcount_dec_and_test(&dmb_node->refcnt))
>> +		wait_event(ldev->dmbs_release, !refcount_read(&dmb_node->refcnt));
> 
> Wait for zero refcnt with dmb node deleted from hash table.
> 
>>   	kfree(dmb_node->cpu_addr);
>>   	kfree(dmb_node);
>>   
>> +	if (atomic_dec_and_test(&ldev->dmb_cnt))
>> +		wake_up(&ldev->ldev_release);
>> +	return 0;
>> +}
> 
> [...]
> 
>> +static int smc_lo_detach_dmb(struct smcd_dev *smcd, u64 token)
>> +{
>> +	struct smc_lo_dmb_node *dmb_node = NULL, *tmp_node;
>> +	struct smc_lo_dev *ldev = smcd->priv;
>> +
>> +	/* find dmb_node according to dmb->dmb_tok */
>> +	read_lock(&ldev->dmb_ht_lock);
>> +	hash_for_each_possible(ldev->dmb_ht, tmp_node, list, token) {
>> +		if (tmp_node->token == token) {
>> +			dmb_node = tmp_node;
>> +			break;
>> +		}
>> +	}
>> +	if (!dmb_node) {
>> +		read_unlock(&ldev->dmb_ht_lock);
>> +		return -EINVAL;
>> +	}
>> +	read_unlock(&ldev->dmb_ht_lock);
>> +
>> +	if (refcount_dec_and_test(&dmb_node->refcnt))
>> +		wake_up_all(&ldev->dmbs_release);
>>   	return 0;
>>   }
> 
> Given no wakeup without finding dmb node in hash table, the chance for
> missing wakeup is not zero.

Good catch! Will fix it.

Thanks,
Wen Gu

[RFC PATCH net-next v3 0/9] net/smc: Introduce SMC-D-based OS internal communication acceleration

[Wen Gu]

Hi, all

# Background

The background and previous discussion can be referred from [1].

We found SMC-D can be used to accelerate OS internal communication, such as
loopback or between two containers within the same OS instance. So this patch
set provides a kind of SMC-D dummy device (we call it the SMC-D loopback device)
to emulate an ISM device, so that SMC-D can also be used on architectures
other than s390. The SMC-D loopback device are designed as a system global
device, visible to all containers.

This version is implemented based on the generalized interface provided by [2].
And there is an open issue of this version, which will be mentioned later.

# Design

This patch set basically follows the design of the previous version.

Patch #1/9 ~ #3/9 attempt to decouple ISM-related structures from the SMC-D
generalized code and extract some helpers to make SMC-D protocol compatible
with devices other than s390 ISM device.

Patch #4/9 introduces a kind of loopback device, which is defined as SMC-D v2
device and designed to provide communication between SMC sockets in the same OS
instance.

 +-------------------------------------------+
 |  +--------------+       +--------------+  |
 |  | SMC socket A |       | SMC socket B |  |
 |  +--------------+       +--------------+  |
 |       ^                         ^         |
 |       |    +----------------+   |         |
 |       |    |   SMC stack    |   |         |
 |       +--->| +------------+ |<--|         |
 |            | |   dummy    | |             |
 |            | |   device   | |             |
 |            +-+------------+-+             |
 |                   OS                      |
 +-------------------------------------------+

Patch #5/9 ~ #8/9 expand SMC-D protocol interface (smcd_ops) for scenarios where
SMC-D is used to communicate within VM (loopback here) or between VMs on the same
host (based on virtio-ism device, see [3]). What these scenarios have in common
is that the local sndbuf and peer RMB can be mapped to same physical memory region,
so the data copy between the local sndbuf and peer RMB can be omitted. Performance
improvement brought by this extension can be found in # Benchmark Test.

 +----------+                     +----------+
 | socket A |                     | socket B |
 +----------+                     +----------+
       |                               ^
       |         +---------+           |
  regard as      |         | ----------|
  local sndbuf   |  B's    |     regard as
       |         |  RMB    |     local RMB
       |-------> |         |
                 +---------+

Patch #9/9 realizes the support of loopback device for the above-mentioned expanded
SMC-D protocol interface.

# Benchmark Test

 * Test environments:
      - VM with Intel Xeon Platinum 8 core 2.50GHz, 16 GiB mem.
      - SMC sndbuf/RMB size 1MB.

 * Test object:
      - TCP lo: run on TCP loopback.
      - domain: run on UNIX domain.
      - SMC lo: run on SMC loopback device with patch #1/9 ~ #4/9.
      - SMC lo-nocpy: run on SMC loopback device with patch #1/9 ~ #9/9.

1. ipc-benchmark (see [4])

 - ./<foo> -c 1000000 -s 100

                    TCP-lo              domain              SMC-lo          SMC-lo-nocpy
Message
rate (msg/s)         79025      115736(+46.45%)    146760(+85.71%)       149800(+89.56%)

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-lo                  SMC-lo             SMC-lo-nocpy
Bandwidth(MBps)   4822.388        4940.918(+2.56%)         8086.67(+67.69%)
Latency(us)          6.298          3.352(-46.78%)            3.35(-46.81%)

3. iperf3

 - serv: <smc_run> taskset -c <cpu> iperf3 -s
 - clnt: <smc_run> taskset -c <cpu> iperf3 -c 127.0.0.1 -t 15

                    TCP-lo                  SMC-lo             SMC-lo-nocpy
Bitrate(Gb/s)         40.7            40.5(-0.49%)            72.4(+77.89%)

4. nginx/wrk

 - serv: <smc_run> nginx
 - clnt: <smc_run> wrk -t 8 -c 500 -d 30 http://127.0.0.1:80

                    TCP-lo                  SMC-lo             SMC-lo-nocpy
Requests/s       155994.57      214544.79(+37.53%)       215538.55(+38.17%)


# Open issue

The open issue has not been resolved now is about how to detect that the source
and target of CLC proposal are within the same OS instance and can communicate
through the SMC-D loopback device. Similar issue also exists when using virtio-ism
devices (the background and details of virtio-ism device can be referred from [3]).
In previous discussions, multiple options were proposed (see [5]). Thanks again for
the help of the community. cc Alexandra Winter :)

But as we discussed, these solutions have some imperfection. So this version of RFC
continues to use previous workaround, that is, a 64-bit random GID is generated for
SMC-D loopback device. If the GIDs of the devices found by two peers are the same,
then they are considered to be in the same OS instance and can communicate with each
other by the loopback device.

This approach has very small risk. Assume the following situations:

(1) Assume that the SMC-D loopback devices of the two OS instances happen to
    generate the same 64-bit GID.

    For the convenience of description, we refer to the sockets on these two
    different OS instance as server A and client B.

    A will misjudge that the two are on the same OS instance because the same GID
    in CLC proposal message. Then A creates its RMB and sends 64-bit token-A to B
    in CLC accept message.

    B receives the CLC accept message. And according to patch #7/9, B tries to
    attach its sndbuf to A's RMB by token-A.

(2) Assume that the OS instance where B is located happens to have an unattached
    RMB whose 64-bit token is same as token-A.

    Then B successfully attaches its sndbuf to the wrong RMB, and creates its RMB,
    sends token-B to A in CLC confirm message.

    Similarly, A receives the message and tries to attach its sndbuf to B's RMB by
    token-B.

(3) Similar to (2), assume that the OS instance where A is located happens to have
    an unattached RMB whose 64-bit token is same as token-B.

    Then A successfully attach its sndbuf to the wrong RMB. Both sides mistakenly
    believe that an SMC-D connection based on the loopback device is established
    between them.

If the above 3 coincidences all happen, that is, 64-bit random number conflicts occur
3 times, then an unreachable SMC-D connection will be established, which is nasty.
If one of above is not satisfied, it will safely fallback to TCP.

Since the chances of these happening are very small, I wonder if this risk of 1/2^(64*3)
probability can be tolerated ?

Another way to solve this open issue is using a 128-bit UUID to identify SMC-D loopback
device or virtio-ism device, because the probability of a 128-bit UUID collision is
considered negligible. But it may need to extend the CLC message to carry a longer GID,
which is the last option.

v3->v2
 1. Adapt new generalized interface provided by [2];
 2. Select loopback device through SMC-D v2 protocol;
 3. Split the loopback-related implementation and generic implementation into different
    patches more reasonably.

v1->v2
 1. Fix some build WARNINGs complained by kernel test rebot
    Reported-by: kernel test robot <lkp@intel.com>
 2. Add iperf3 test data.

[1] https://lore.kernel.org/netdev/1671506505-104676-1-git-send-email-guwen@linux.alibaba.com/
[2] https://lore.kernel.org/netdev/20230123181752.1068-1-jaka@linux.ibm.com/
[3] https://lists.oasis-open.org/archives/virtio-comment/202302/msg00148.html
[4] https://github.com/goldsborough/ipc-bench
[5] https://lore.kernel.org/netdev/b9867c7d-bb2b-16fc-feda-b79579aa833d@linux.ibm.com/

Wen Gu (9):
  net/smc: Decouple ism_dev from SMC-D device dump
  net/smc: Decouple ism_dev from SMC-D DMB registration
  net/smc: Extract v2 check helper from SMC-D device registration
  net/smc: Introduce SMC-D loopback device
  net/smc: Introduce an interface for getting DMB attribute
  net/smc: Introudce interfaces for DMB attach and detach
  net/smc: Avoid data copy from sndbuf to peer RMB in SMC-D
  net/smc: Modify cursor update logic when using mappable DMB
  net/smc: Add interface implementation of loopback device

 drivers/s390/net/ism_drv.c |   5 +-
 include/net/smc.h          |  18 +-
 net/smc/Makefile           |   2 +-
 net/smc/af_smc.c           |  26 ++-
 net/smc/smc_cdc.c          |  59 ++++--
 net/smc/smc_cdc.h          |   1 +
 net/smc/smc_core.c         |  70 ++++++-
 net/smc/smc_core.h         |   1 +
 net/smc/smc_ism.c          |  79 ++++++--
 net/smc/smc_ism.h          |   4 +
 net/smc/smc_loopback.c     | 442 +++++++++++++++++++++++++++++++++++++++++++++
 net/smc/smc_loopback.h     |  55 ++++++
 12 files changed, 725 insertions(+), 37 deletions(-)
 create mode 100644 net/smc/smc_loopback.c
 create mode 100644 net/smc/smc_loopback.h

-- 
1.8.3.1

[RFC PATCH net-next v3 9/9] net/smc: Add interface implementation of loopback device

[Wen Gu]

This patch completes the specific implementation of loopback device
for the newly added SMC-D DMB-related interface.

The loopback device always provides mappable DMB because the device
users are in the same OS instance.

Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
---
 net/smc/smc_loopback.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++----
 net/smc/smc_loopback.h |  4 +++
 2 files changed, 90 insertions(+), 7 deletions(-)

diff --git a/net/smc/smc_loopback.c b/net/smc/smc_loopback.c
index 38bacd8..2fa2a73 100644
--- a/net/smc/smc_loopback.c
+++ b/net/smc/smc_loopback.c
@@ -77,6 +77,7 @@ static int smc_lo_register_dmb(struct smcd_dev *smcd, struct smcd_dmb *dmb,
 		goto err_node;
 	}
 	dmb_node->len = dmb->dmb_len;
+	refcount_set(&dmb_node->refcnt, 1);
 
 	/* TODO: token is random but not exclusive !
 	 * suppose to find token in dmb hask table, if has this token
@@ -87,6 +88,7 @@ static int smc_lo_register_dmb(struct smcd_dev *smcd, struct smcd_dmb *dmb,
 	write_lock(&ldev->dmb_ht_lock);
 	hash_add(ldev->dmb_ht, &dmb_node->list, dmb_node->token);
 	write_unlock(&ldev->dmb_ht_lock);
+	atomic_inc(&ldev->dmb_cnt);
 
 	dmb->sba_idx = dmb_node->sba_idx;
 	dmb->dmb_tok = dmb_node->token;
@@ -124,12 +126,76 @@ static int smc_lo_unregister_dmb(struct smcd_dev *smcd, struct smcd_dmb *dmb)
 	write_unlock(&ldev->dmb_ht_lock);
 
 	clear_bit(dmb_node->sba_idx, ldev->sba_idx_mask);
+
+	/* wait for dmb refcnt to be 0 */
+	if (!refcount_dec_and_test(&dmb_node->refcnt))
+		wait_event(ldev->dmbs_release, !refcount_read(&dmb_node->refcnt));
 	kfree(dmb_node->cpu_addr);
 	kfree(dmb_node);
 
+	if (atomic_dec_and_test(&ldev->dmb_cnt))
+		wake_up(&ldev->ldev_release);
+	return 0;
+}
+
+static int smc_lo_attach_dmb(struct smcd_dev *smcd, struct smcd_dmb *dmb)
+{
+	struct smc_lo_dmb_node *dmb_node = NULL, *tmp_node;
+	struct smc_lo_dev *ldev = smcd->priv;
+
+	/* find dmb_node according to dmb->dmb_tok */
+	read_lock(&ldev->dmb_ht_lock);
+	hash_for_each_possible(ldev->dmb_ht, tmp_node, list, dmb->dmb_tok) {
+		if (tmp_node->token == dmb->dmb_tok) {
+			dmb_node = tmp_node;
+			break;
+		}
+	}
+	if (!dmb_node) {
+		read_unlock(&ldev->dmb_ht_lock);
+		return -EINVAL;
+	}
+	read_unlock(&ldev->dmb_ht_lock);
+	refcount_inc(&dmb_node->refcnt);
+
+	/* provide dmb information */
+	dmb->sba_idx = dmb_node->sba_idx;
+	dmb->dmb_tok = dmb_node->token;
+	dmb->cpu_addr = dmb_node->cpu_addr;
+	dmb->dma_addr = dmb_node->dma_addr;
+	dmb->dmb_len = dmb_node->len;
+	return 0;
+}
+
+static int smc_lo_detach_dmb(struct smcd_dev *smcd, u64 token)
+{
+	struct smc_lo_dmb_node *dmb_node = NULL, *tmp_node;
+	struct smc_lo_dev *ldev = smcd->priv;
+
+	/* find dmb_node according to dmb->dmb_tok */
+	read_lock(&ldev->dmb_ht_lock);
+	hash_for_each_possible(ldev->dmb_ht, tmp_node, list, token) {
+		if (tmp_node->token == token) {
+			dmb_node = tmp_node;
+			break;
+		}
+	}
+	if (!dmb_node) {
+		read_unlock(&ldev->dmb_ht_lock);
+		return -EINVAL;
+	}
+	read_unlock(&ldev->dmb_ht_lock);
+
+	if (refcount_dec_and_test(&dmb_node->refcnt))
+		wake_up_all(&ldev->dmbs_release);
 	return 0;
 }
 
+static int smc_lo_get_dev_dmb_attr(struct smcd_dev *smcd)
+{
+	return (1 << ISM_DMB_MAPPABLE);
+}
+
 static int smc_lo_add_vlan_id(struct smcd_dev *smcd, u64 vlan_id)
 {
 	return -EOPNOTSUPP;
@@ -156,7 +222,15 @@ static int smc_lo_move_data(struct smcd_dev *smcd, u64 dmb_tok, unsigned int idx
 {
 	struct smc_lo_dmb_node *rmb_node = NULL, *tmp_node;
 	struct smc_lo_dev *ldev = smcd->priv;
-
+	struct smc_connection *conn;
+
+	if (!sf) {
+		/* local sndbuf shares the same physical memory with
+		 * peer RMB, so no need to copy data from local sndbuf
+		 * to peer RMB.
+		 */
+		return 0;
+	}
 	read_lock(&ldev->dmb_ht_lock);
 	hash_for_each_possible(ldev->dmb_ht, tmp_node, list, dmb_tok) {
 		if (tmp_node->token == dmb_tok) {
@@ -172,13 +246,10 @@ static int smc_lo_move_data(struct smcd_dev *smcd, u64 dmb_tok, unsigned int idx
 
 	memcpy((char *)rmb_node->cpu_addr + offset, data, size);
 
-	if (sf) {
-		struct smc_connection *conn =
-			smcd->conn[rmb_node->sba_idx];
+	conn = smcd->conn[rmb_node->sba_idx];
+	if (conn && !conn->killed)
+		smcd_cdc_rx_handler(conn);
 
-		if (conn && !conn->killed)
-			smcd_cdc_rx_handler(conn);
-	}
 	return 0;
 }
 
@@ -206,6 +277,8 @@ static struct device *smc_lo_get_dev(struct smcd_dev *smcd)
 	.query_remote_gid = smc_lo_query_rgid,
 	.register_dmb = smc_lo_register_dmb,
 	.unregister_dmb = smc_lo_unregister_dmb,
+	.attach_dmb = smc_lo_attach_dmb,
+	.detach_dmb = smc_lo_detach_dmb,
 	.add_vlan_id = smc_lo_add_vlan_id,
 	.del_vlan_id = smc_lo_del_vlan_id,
 	.set_vlan_required = smc_lo_set_vlan_required,
@@ -216,6 +289,7 @@ static struct device *smc_lo_get_dev(struct smcd_dev *smcd)
 	.get_local_gid = smc_lo_get_local_gid,
 	.get_chid = smc_lo_get_chid,
 	.get_dev = smc_lo_get_dev,
+	.get_dev_dmb_attr = smc_lo_get_dev_dmb_attr,
 };
 
 static struct smcd_dev *smcd_lo_alloc_dev(const struct smcd_ops *ops,
@@ -296,6 +370,9 @@ static int smc_lo_dev_init(struct smc_lo_dev *ldev)
 	smc_lo_gen_id(ldev);
 	rwlock_init(&ldev->dmb_ht_lock);
 	hash_init(ldev->dmb_ht);
+	atomic_set(&ldev->dmb_cnt, 0);
+	init_waitqueue_head(&ldev->dmbs_release);
+	init_waitqueue_head(&ldev->ldev_release);
 
 	return smcd_lo_register_dev(ldev);
 }
@@ -334,6 +411,8 @@ static int smc_lo_dev_probe(void)
 static void smc_lo_dev_exit(struct smc_lo_dev *ldev)
 {
 	smcd_lo_unregister_dev(ldev);
+	if (atomic_read(&ldev->dmb_cnt))
+		wait_event(ldev->ldev_release, !atomic_read(&ldev->dmb_cnt));
 }
 
 static void smc_lo_dev_remove(void)
diff --git a/net/smc/smc_loopback.h b/net/smc/smc_loopback.h
index 9d34aba..0a09330 100644
--- a/net/smc/smc_loopback.h
+++ b/net/smc/smc_loopback.h
@@ -33,6 +33,7 @@ struct smc_lo_dmb_node {
 	u32 sba_idx;
 	void *cpu_addr;
 	dma_addr_t dma_addr;
+	refcount_t refcnt;
 };
 
 struct smc_lo_dev {
@@ -43,6 +44,9 @@ struct smc_lo_dev {
 	DECLARE_BITMAP(sba_idx_mask, SMC_LODEV_MAX_DMBS);
 	rwlock_t dmb_ht_lock;
 	DECLARE_HASHTABLE(dmb_ht, SMC_LODEV_MAX_DMBS_BUCKETS);
+	atomic_t dmb_cnt;
+	wait_queue_head_t dmbs_release;
+	wait_queue_head_t ldev_release;
 };
 
 int smc_loopback_init(void);
-- 
1.8.3.1