[RFC net-next 0/2] Optimize the parallelism of SMC-R connections

[RFC net-next 0/2] Optimize the parallelism of SMC-R connections

[D. Wythe]

From: "D. Wythe" <alibuda@linux.alibaba.com>

This patchset attempts to optimize the parallelism of SMC-R connections
in quite a SIMPLE way, reduce unnecessary blocking on locks.

According to Off-CPU statistics, SMC worker's off-CPU statistics
as that: 

smc_listen_work 			(48.17%)
	__mutex_lock.isra.11 		(47.96%)

An ideal SMC-R connection process should only block on the IO events
of the network, but it's quite clear that the SMC-R connection now is
queued on the lock most of the time.

Before creating a connection, we always try to see if it can be
successfully created without allowing the creation of an lgr,
if so, it means it does not rely on new link group.
In other words, locking on xxx_lgr_pending is not necessary
any more.

Noted that removing this lock will not have an immediate effect
in the current version, as there are still some concurrency issues
in the SMC handshake phase. However, regardless, removing this lock
is a prerequisite for other optimizations.

If you have any questions or suggestions, please let me know.

D. Wythe (2):
  net/smc: refactoring lgr pending lock
  net/smc: remove locks smc_client_lgr_pending and
    smc_server_lgr_pending

 net/smc/af_smc.c   | 24 ++++++++++++------------
 net/smc/smc_clc.h  |  1 +
 net/smc/smc_core.c | 28 ++++++++++++++++++++++++++--
 net/smc/smc_core.h | 21 +++++++++++++++++++++
 4 files changed, 60 insertions(+), 14 deletions(-)

-- 
1.8.3.1

[RFC net-next 1/2] net/smc: refactoring lgr pending lock

[D. Wythe]

From: "D. Wythe" <alibuda@linux.alibaba.com>

This patch replaces the locking and unlocking of lgr pending with
a unified inline function, and since the granularity of lgr pending
lock is within the lifecycle of init_info, which make it possible
to record the lock state on init_info. So that other routines can
unlock lgr pending lock safely.

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
 net/smc/af_smc.c   | 24 ++++++++++++------------
 net/smc/smc_core.h | 21 +++++++++++++++++++++
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index bacdd97..52a987b 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1251,10 +1251,10 @@ static int smc_connect_rdma(struct smc_sock *smc,
 	if (reason_code)
 		return reason_code;
 
-	mutex_lock(&smc_client_lgr_pending);
+	smc_lgr_pending_lock(ini, &smc_client_lgr_pending);
 	reason_code = smc_conn_create(smc, ini);
 	if (reason_code) {
-		mutex_unlock(&smc_client_lgr_pending);
+		smc_lgr_pending_unlock(ini, &smc_client_lgr_pending);
 		return reason_code;
 	}
 
@@ -1346,7 +1346,7 @@ static int smc_connect_rdma(struct smc_sock *smc,
 		if (reason_code)
 			goto connect_abort;
 	}
-	mutex_unlock(&smc_client_lgr_pending);
+	smc_lgr_pending_unlock(ini, &smc_client_lgr_pending);
 
 	smc_copy_sock_settings_to_clc(smc);
 	smc->connect_nonblock = 0;
@@ -1356,7 +1356,7 @@ static int smc_connect_rdma(struct smc_sock *smc,
 	return 0;
 connect_abort:
 	smc_conn_abort(smc, ini->first_contact_local);
-	mutex_unlock(&smc_client_lgr_pending);
+	smc_lgr_pending_unlock(ini, &smc_client_lgr_pending);
 	smc->connect_nonblock = 0;
 
 	return reason_code;
@@ -1413,10 +1413,10 @@ static int smc_connect_ism(struct smc_sock *smc,
 	ini->ism_peer_gid[ini->ism_selected] = aclc->d0.gid;
 
 	/* there is only one lgr role for SMC-D; use server lock */
-	mutex_lock(&smc_server_lgr_pending);
+	smc_lgr_pending_lock(ini, &smc_server_lgr_pending);
 	rc = smc_conn_create(smc, ini);
 	if (rc) {
-		mutex_unlock(&smc_server_lgr_pending);
+		smc_lgr_pending_unlock(ini, &smc_server_lgr_pending);
 		return rc;
 	}
 
@@ -1443,7 +1443,7 @@ static int smc_connect_ism(struct smc_sock *smc,
 				  aclc->hdr.version, eid, ini);
 	if (rc)
 		goto connect_abort;
-	mutex_unlock(&smc_server_lgr_pending);
+	smc_lgr_pending_unlock(ini, &smc_server_lgr_pending);
 
 	smc_copy_sock_settings_to_clc(smc);
 	smc->connect_nonblock = 0;
@@ -1453,7 +1453,7 @@ static int smc_connect_ism(struct smc_sock *smc,
 	return 0;
 connect_abort:
 	smc_conn_abort(smc, ini->first_contact_local);
-	mutex_unlock(&smc_server_lgr_pending);
+	smc_lgr_pending_unlock(ini, &smc_server_lgr_pending);
 	smc->connect_nonblock = 0;
 
 	return rc;
@@ -2460,7 +2460,7 @@ static void smc_listen_work(struct work_struct *work)
 	if (rc)
 		goto out_decl;
 
-	mutex_lock(&smc_server_lgr_pending);
+	smc_lgr_pending_lock(ini, &smc_server_lgr_pending);
 	smc_close_init(new_smc);
 	smc_rx_init(new_smc);
 	smc_tx_init(new_smc);
@@ -2479,7 +2479,7 @@ static void smc_listen_work(struct work_struct *work)
 
 	/* SMC-D does not need this lock any more */
 	if (ini->is_smcd)
-		mutex_unlock(&smc_server_lgr_pending);
+		smc_lgr_pending_unlock(ini, &smc_server_lgr_pending);
 
 	/* receive SMC Confirm CLC message */
 	memset(buf, 0, sizeof(*buf));
@@ -2510,7 +2510,7 @@ static void smc_listen_work(struct work_struct *work)
 					    ini->first_contact_local, ini);
 		if (rc)
 			goto out_unlock;
-		mutex_unlock(&smc_server_lgr_pending);
+		smc_lgr_pending_unlock(ini, &smc_server_lgr_pending);
 	}
 	smc_conn_save_peer_info(new_smc, cclc);
 	smc_listen_out_connected(new_smc);
@@ -2518,7 +2518,7 @@ static void smc_listen_work(struct work_struct *work)
 	goto out_free;
 
 out_unlock:
-	mutex_unlock(&smc_server_lgr_pending);
+	smc_lgr_pending_unlock(ini, &smc_server_lgr_pending);
 out_decl:
 	smc_listen_decline(new_smc, rc, ini ? ini->first_contact_local : 0,
 			   proposal_version);
diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
index 120027d..6f309a3 100644
--- a/net/smc/smc_core.h
+++ b/net/smc/smc_core.h
@@ -422,6 +422,8 @@ struct smc_init_info {
 	u8			ism_offered_cnt; /* # of ISM devices offered */
 	u8			ism_selected;    /* index of selected ISM dev*/
 	u8			smcd_version;
+	/* mutex holding for conn create */
+	struct mutex *mutex;
 };
 
 /* Find the connection associated with the given alert token in the link group.
@@ -589,6 +591,25 @@ struct smc_link *smc_switch_conns(struct smc_link_group *lgr,
 int smcr_nl_get_link(struct sk_buff *skb, struct netlink_callback *cb);
 int smcd_nl_get_lgr(struct sk_buff *skb, struct netlink_callback *cb);
 
+static inline void smc_lgr_pending_lock(struct smc_init_info *ini, struct mutex *lock)
+{
+	if (unlikely(ini->mutex))
+		pr_warn_once("smc: lgr pending deadlock dected.");
+
+	mutex_lock(lock);
+	ini->mutex = lock;
+}
+
+static inline void smc_lgr_pending_unlock(struct smc_init_info *ini, struct mutex *lock)
+{
+	/* already unlock it */
+	if (!ini->mutex)
+		return;
+
+	ini->mutex = NULL;
+	mutex_unlock(lock);
+}
+
 static inline struct smc_link_group *smc_get_lgr(struct smc_link *link)
 {
 	return link->lgr;
-- 
1.8.3.1

[RFC net-next 2/2] net/smc: remove locks smc_client_lgr_pending and smc_server_lgr_pending

[D. Wythe]

From: "D. Wythe" <alibuda@linux.alibaba.com>

This patch attempts to remove locks named smc_client_lgr_pending and
smc_server_lgr_pending, which aim to serialize the creation of link
group. However, once link group existed already, those locks are
meaningless, worse still, they make incoming connections have to be
queued one after the other.

Before attempting to locking at xxx_lgr_pending, trying to invoke
smc_conn_create() firstly but does not allow it to create link group.
Once we found we MUST create link group, then we can make lock on it.
In that way, we can skip meaningless lock.

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
 net/smc/smc_clc.h  |  1 +
 net/smc/smc_core.c | 28 ++++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index c5c8e7d..050484a 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -48,6 +48,7 @@
 #define SMC_CLC_DECL_RELEASEERR	0x03030009  /* release version negotiate failed */
 #define SMC_CLC_DECL_MAXCONNERR	0x0303000a  /* max connections negotiate failed */
 #define SMC_CLC_DECL_MAXLINKERR	0x0303000b  /* max links negotiate failed */
+#define SMC_CLC_DECL_REQLGR	0x0303000c  /* required create link grou */
 #define SMC_CLC_DECL_MODEUNSUPP	0x03040000  /* smc modes do not match (R or D)*/
 #define SMC_CLC_DECL_RMBE_EC	0x03050000  /* peer has eyecatcher in RMBE    */
 #define SMC_CLC_DECL_OPTUNSUPP	0x03060000  /* fastopen sockopt not supported */
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index bd01dd3..76c82ae 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1863,8 +1863,7 @@ static bool smcd_lgr_match(struct smc_link_group *lgr,
 	return lgr->peer_gid == peer_gid && lgr->smcd == smcismdev;
 }
 
-/* create a new SMC connection (and a new link group if necessary) */
-int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
+static int __smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini, bool create_lgr)
 {
 	struct smc_connection *conn = &smc->conn;
 	struct net *net = sock_net(&smc->sk);
@@ -1927,6 +1926,8 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
 
 create:
 	if (ini->first_contact_local) {
+		if (!create_lgr)
+			return SMC_CLC_DECL_REQLGR;
 		rc = smc_lgr_create(smc, ini);
 		if (rc)
 			goto out;
@@ -1962,6 +1963,29 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
 	return rc;
 }
 
+/* create a new SMC connection (and a new link group if necessary) */
+int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
+{
+	int rc;
+
+	/* make no impact on SMCD */
+	if (ini->is_smcd)
+		goto locked;
+
+	/* try create conn without create lgr first */
+	rc = __smc_conn_create(smc, ini, /* disallow create lgr */ false);
+	if (!rc) {
+		/* not rely on new lgr, unlock lgr pending lock in advance. */
+		smc_lgr_pending_unlock(ini, ini->mutex);
+		return 0;
+	} else if (rc != SMC_CLC_DECL_REQLGR) {
+		/* that's unexcepted error */
+		return rc;
+	}
+locked:
+	return __smc_conn_create(smc, ini, /* create lgr if needed */ true);
+}
+
 #define SMCD_DMBE_SIZES		6 /* 0 -> 16KB, 1 -> 32KB, .. 6 -> 1MB */
 #define SMCR_RMBE_SIZES		5 /* 0 -> 16KB, 1 -> 32KB, .. 5 -> 512KB */
 
-- 
1.8.3.1

Re: [RFC net-next 0/2] Optimize the parallelism of SMC-R connections

[Alexandra Winter]

On 06.09.23 15:55, D. Wythe wrote:
> From: "D. Wythe" <alibuda@linux.alibaba.com>
> 
> This patchset attempts to optimize the parallelism of SMC-R connections
> in quite a SIMPLE way, reduce unnecessary blocking on locks.
> 
> According to Off-CPU statistics, SMC worker's off-CPU statistics
> as that: 
> 
> smc_listen_work 			(48.17%)
> 	__mutex_lock.isra.11 		(47.96%)
> 
> An ideal SMC-R connection process should only block on the IO events
> of the network, but it's quite clear that the SMC-R connection now is
> queued on the lock most of the time.
> 
> Before creating a connection, we always try to see if it can be
> successfully created without allowing the creation of an lgr,
> if so, it means it does not rely on new link group.
> In other words, locking on xxx_lgr_pending is not necessary
> any more.
> 
> Noted that removing this lock will not have an immediate effect
> in the current version, as there are still some concurrency issues
> in the SMC handshake phase. However, regardless, removing this lock
> is a prerequisite for other optimizations.
> 
> If you have any questions or suggestions, please let me know.
> 
> D. Wythe (2):
>   net/smc: refactoring lgr pending lock
>   net/smc: remove locks smc_client_lgr_pending and
>     smc_server_lgr_pending
> 
>  net/smc/af_smc.c   | 24 ++++++++++++------------
>  net/smc/smc_clc.h  |  1 +
>  net/smc/smc_core.c | 28 ++++++++++++++++++++++++++--
>  net/smc/smc_core.h | 21 +++++++++++++++++++++
>  4 files changed, 60 insertions(+), 14 deletions(-)
> 


I have to admit that locking in SMC is quite confusing to me, so this is just my thougths.

Your proposal seems to make things even more complex.

I understand the goal to optimize parallelism.
Today we have the global smc_server/client_lgr_pending AND smc_lgr_list.lock (and more).
There seems to be some overlpa in scope..
Maybe there is some way to reduce the length of the locked paths?
Or use other mechanisms than the big fat smc_server/client_lgr_pending mutex?
e.g.
If you think you can unlock after __smc_conn_create in the re-use-existing_LGR case,
why is the lock needed until after smc_clc_send_confirm in the new-LGR case??


Your use of storing the global lock per ini and then double-freeing it sometimes,
seems a bit homebrewed, though.
E.g. I'm afraid the existing lock checking algorithms could not verify this pattern.

Re: [RFC net-next 0/2] Optimize the parallelism of SMC-R connections

[Alexandra Winter]

On 18.09.23 05:58, D. Wythe wrote:
> Hi Alexandra,
> 
> Sorry for the late reply. I have been thinking about the question you mentioned for a while, and this is a great opportunity to discuss this issue.
> My point is that the purpose of the locks is to minimize the expansion of the number of link groups as much as possible.
> 
> As we all know, the SMC-R protocol has the following specifications:
> 
>  * A SMC-R connection MUST be mapped into one link group.
>  * A link group is usually created by a connection, which is also known
>    as "First Contact."
> 
> If we start from scratch, we can design the connection process as follows:
> 
> 1. Check if there are any available link groups. If so, map the
>    connection into it and go to step 3.
> 2. Mark this connection as "First Contact," create a link group, and
>    mark the new link group as unavailable.
> 3. Finish connection establishment.
> 4. If the connection is "First Contact," mark the new link group as
>    available and map the connection into it.
> 
> I think there is no logical problem with this process, but there is a practical issue where burst traffic can result in burst link groups.
> 
> For example, if there are 10,000 incoming connections, based on the above logic, the most extreme scenario would be to create 10,000 link groups.
> This can cause significant memory pressure and even be used for security attacks.
> 
> To address this goal, the simplest way is to make each connection process mutually exclusive, having the following process:
> 
> 1. Block other incoming connections.
> 2. Check if there are any available link groups. If so, map the
>    connection into it and go to step 4.
> 3. Mark this connection as "First Contact," create a link group, and
>    mark it as unavailable.
> 4. Finish connection establishment.
> 5. If the connection is "First Contact," mark the new link group as
>    available and map the connection into it.
> 6. Allow other connections to come in.
> 
> And this is our current process now!
> 
> Regarding the purpose of the locks, to minimize the expansion of the number of link groups. If we agree with this point, we can observe that
> in phase 2 going to phase 4, this process will never create a new link group. Obviously, the lock is not needed here.

Well, you still have issue of a link group going away. Thread 1 is deleting the last connection from a link group and shutting it down. Thread 2 is adding a 'second' connection (from its poitn ov view) to the linkgroup.

> 
> Then the last question: why is the lock needed until after smc_clc_send_confirm in the new-LGR case? We can try to move phase 6 ahead as follows:
> 
> 1. Block other incoming connections.
> 2. Check if there are any available link groups. If so, map the
>    connection into it and go to step 4.
> 3. Mark this connection as "First Contact," create a link group, and
>    mark it as unavailable.
> 4. Allow other connections to come in.
> 5. Finish connection establishment.
> 6. If the connection is "First Contact," mark the new link group as
>    available and map the connection into it.
> 
> There is also no problem with this process! However, note that this logic does not address burst issues.
> Burst traffic will still result in burst link groups because a new link group can only be marked as available when the "First Contact" is completed,
> which is after sending the CLC Confirm.
> 
> Hope my point is helpful to you. If you have any questions, please let me know. Thanks.
> 
> Best wishes,
> D. Wythe

You are asking exactly the right questions here. Creation of new connections is on the critical path,
and if the design can be optimized for parallelism that will increase perfromance, while insufficient
locking will create nasty bugs.
Many programmers have dealt with these issues before us. I would recommend to consult existing proven
patterns; e.g. the ones listed in Paul McKenney's book 
(https://mirrors.edge.kernel.org/pub/linux/kernel/people/paulmck/perfbook/) 
e.g. 'Chapter 10.3 Read-Mostly Data Structures' and of course the kernel documentation folder.
Improving an existing codebase like smc without breaking is not trivial. Obviuosly a step-by-step approach,
works best. So if you can identify actions that can be be done under a smaller (as in more granular) lock
instead of under a global lock. OR change a mutex into R/W or RCU.
Smaller changes are easier to review (and bisect in case of regressions).

Re: [RFC net-next 0/2] Optimize the parallelism of SMC-R connections

[D. Wythe]

On 9/21/23 8:36 PM, Alexandra Winter wrote:
> On 18.09.23 05:58, D. Wythe wrote:
>> Hi Alexandra,
>>
>> Sorry for the late reply. I have been thinking about the question you mentioned for a while, and this is a great opportunity to discuss this issue.
>> My point is that the purpose of the locks is to minimize the expansion of the number of link groups as much as possible.
>>
>> As we all know, the SMC-R protocol has the following specifications:
>>
>>   * A SMC-R connection MUST be mapped into one link group.
>>   * A link group is usually created by a connection, which is also known
>>     as "First Contact."
>>
>> If we start from scratch, we can design the connection process as follows:
>>
>> 1. Check if there are any available link groups. If so, map the
>>     connection into it and go to step 3.
>> 2. Mark this connection as "First Contact," create a link group, and
>>     mark the new link group as unavailable.
>> 3. Finish connection establishment.
>> 4. If the connection is "First Contact," mark the new link group as
>>     available and map the connection into it.
>>
>> I think there is no logical problem with this process, but there is a practical issue where burst traffic can result in burst link groups.
>>
>> For example, if there are 10,000 incoming connections, based on the above logic, the most extreme scenario would be to create 10,000 link groups.
>> This can cause significant memory pressure and even be used for security attacks.
>>
>> To address this goal, the simplest way is to make each connection process mutually exclusive, having the following process:
>>
>> 1. Block other incoming connections.
>> 2. Check if there are any available link groups. If so, map the
>>     connection into it and go to step 4.
>> 3. Mark this connection as "First Contact," create a link group, and
>>     mark it as unavailable.
>> 4. Finish connection establishment.
>> 5. If the connection is "First Contact," mark the new link group as
>>     available and map the connection into it.
>> 6. Allow other connections to come in.
>>
>> And this is our current process now!
>>
>> Regarding the purpose of the locks, to minimize the expansion of the number of link groups. If we agree with this point, we can observe that
>> in phase 2 going to phase 4, this process will never create a new link group. Obviously, the lock is not needed here.
> Well, you still have issue of a link group going away. Thread 1 is deleting the last connection from a link group and shutting it down. Thread 2 is adding a 'second' connection (from its poitn ov view) to the linkgroup.

Hi Alexandra,

That's right.  But even if we do nothing, the current implements still 
has this problem.
And this problem can be solved by the spinlock inside smc_conn_create, 
rather than the
pending lock.

And also deleting the last connection from a link group will not 
shutting the down right now,
usually waiting for 10 minutes of idle time.

>> Then the last question: why is the lock needed until after smc_clc_send_confirm in the new-LGR case? We can try to move phase 6 ahead as follows:
>>
>> 1. Block other incoming connections.
>> 2. Check if there are any available link groups. If so, map the
>>     connection into it and go to step 4.
>> 3. Mark this connection as "First Contact," create a link group, and
>>     mark it as unavailable.
>> 4. Allow other connections to come in.
>> 5. Finish connection establishment.
>> 6. If the connection is "First Contact," mark the new link group as
>>     available and map the connection into it.
>>
>> There is also no problem with this process! However, note that this logic does not address burst issues.
>> Burst traffic will still result in burst link groups because a new link group can only be marked as available when the "First Contact" is completed,
>> which is after sending the CLC Confirm.
>>
>> Hope my point is helpful to you. If you have any questions, please let me know. Thanks.
>>
>> Best wishes,
>> D. Wythe
> You are asking exactly the right questions here. Creation of new connections is on the critical path,
> and if the design can be optimized for parallelism that will increase perfromance, while insufficient
> locking will create nasty bugs.
> Many programmers have dealt with these issues before us. I would recommend to consult existing proven
> patterns; e.g. the ones listed in Paul McKenney's book
> (https://mirrors.edge.kernel.org/pub/linux/kernel/people/paulmck/perfbook/)
> e.g. 'Chapter 10.3 Read-Mostly Data Structures' and of course the kernel documentation folder.
> Improving an existing codebase like smc without breaking is not trivial. Obviuosly a step-by-step approach,
> works best. So if you can identify actions that can be be done under a smaller (as in more granular) lock
> instead of under a global lock. OR change a mutex into R/W or RCU.
> Smaller changes are easier to review (and bisect in case of regressions).

I have to say it's quite hard to make the lock smaller, we have indeed 
considered the impact of the complexity of the patch on review,
and this might be the simplest solution we can think of. If this 
solution is not okay for you, perhaps we can discuss
whether there is a better solution ?

Best wishes,
D. Wythe

Re: [RFC net-next 0/2] Optimize the parallelism of SMC-R connections

[Alexandra Winter]

On 25.09.23 12:10, D. Wythe wrote:
> That's right.  But even if we do nothing, the current implements still has this problem.
> And this problem can be solved by the spinlock inside smc_conn_create, rather than the
> pending lock.
> 

May I kindly propose to fix this problem first and then do performance improvements after that?


> And also deleting the last connection from a link group will not shutting the down right now,
> usually waiting for 10 minutes of idle time.

Still the new connection could come in just the moment when the 10 minutes are over.