[RFC PATCH v2 net-next 0/6] net/smc: several features's implementation for smc v2.1

[RFC PATCH v2 net-next 0/6] net/smc: several features's implementation for smc v2.1

[Guangguan Wang]

This patch set implement several new features in SMC v2.1(https://
www.ibm.com/support/pages/node/7009315), including vendor unique
experimental options, max connections per lgr negotiation, max links
per lgr negotiation.

v1 - v2:
 - Remove ini pointer NULL check and fix code style in
   smc_clc_send_confirm_accept.
 - Optimize the max_conns check in smc_clc_xxx_v2x_features_validate.

Guangguan Wang (6):
  net/smc: support smc release version negotiation in clc handshake
  net/smc: add vendor unique experimental options area in clc handshake
  net/smc: support smc v2.x features validate
  net/smc: support max connections per lgr negotiation
  net/smc: support max links per lgr negotiation in clc handshake
  net/smc: Extend SMCR v2 linkgroup netlink attribute

 include/uapi/linux/smc.h |   2 +
 net/smc/af_smc.c         |  87 +++++++++++++++++------
 net/smc/smc.h            |   5 +-
 net/smc/smc_clc.c        | 149 ++++++++++++++++++++++++++++++++-------
 net/smc/smc_clc.h        |  53 ++++++++++++--
 net/smc/smc_core.c       |  13 +++-
 net/smc/smc_core.h       |  11 +++
 net/smc/smc_llc.c        |  21 ++++--
 8 files changed, 286 insertions(+), 55 deletions(-)

-- 
2.24.3 (Apple Git-128)

[RFC PATCH v2 net-next 1/6] net/smc: support smc release version negotiation in clc handshake

[Guangguan Wang]

Support smc release version negotiation in clc handshake. And set
the latest smc release version to 2.1.

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
---
 net/smc/af_smc.c   | 22 ++++++++++++++++++++--
 net/smc/smc.h      |  5 ++++-
 net/smc/smc_clc.c  | 14 +++++++-------
 net/smc/smc_clc.h  | 23 ++++++++++++++++++++++-
 net/smc/smc_core.h |  1 +
 5 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index a7f887d91d89..bac73eb0542d 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1187,6 +1187,11 @@ static int smc_connect_rdma_v2_prepare(struct smc_sock *smc,
 			return SMC_CLC_DECL_NOINDIRECT;
 		}
 	}
+
+	if (fce->release > SMC_RELEASE)
+		return SMC_CLC_DECL_VERSMISMAT;
+	ini->release_ver = fce->release;
+
 	return 0;
 }
 
@@ -1355,6 +1360,15 @@ static int smc_connect_ism(struct smc_sock *smc,
 		struct smc_clc_msg_accept_confirm_v2 *aclc_v2 =
 			(struct smc_clc_msg_accept_confirm_v2 *)aclc;
 
+		if (ini->first_contact_peer) {
+			struct smc_clc_first_contact_ext *fce =
+				smc_get_clc_first_contact_ext(aclc_v2, true);
+
+			if (fce->release > SMC_RELEASE)
+				return SMC_CLC_DECL_VERSMISMAT;
+			ini->release_ver = fce->release;
+		}
+
 		rc = smc_v2_determine_accepted_chid(aclc_v2, ini);
 		if (rc)
 			return rc;
@@ -1389,7 +1403,7 @@ static int smc_connect_ism(struct smc_sock *smc,
 	}
 
 	rc = smc_clc_send_confirm(smc, ini->first_contact_local,
-				  aclc->hdr.version, eid, NULL);
+				  aclc->hdr.version, eid, ini);
 	if (rc)
 		goto connect_abort;
 	mutex_unlock(&smc_server_lgr_pending);
@@ -1965,6 +1979,10 @@ static int smc_listen_v2_check(struct smc_sock *new_smc,
 		}
 	}
 
+	ini->release_ver = pclc_v2_ext->hdr.flag.release;
+	if (pclc_v2_ext->hdr.flag.release > SMC_RELEASE)
+		ini->release_ver = SMC_RELEASE;
+
 out:
 	if (!ini->smcd_version && !ini->smcr_version)
 		return rc;
@@ -2412,7 +2430,7 @@ static void smc_listen_work(struct work_struct *work)
 	/* send SMC Accept CLC message */
 	accept_version = ini->is_smcd ? ini->smcd_version : ini->smcr_version;
 	rc = smc_clc_send_accept(new_smc, ini->first_contact_local,
-				 accept_version, ini->negotiated_eid);
+				 accept_version, ini->negotiated_eid, ini);
 	if (rc)
 		goto out_unlock;
 
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 2eeea4cdc718..9cce1a41e011 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -21,7 +21,10 @@
 
 #define SMC_V1		1		/* SMC version V1 */
 #define SMC_V2		2		/* SMC version V2 */
-#define SMC_RELEASE	0
+
+#define SMC_RELEASE_0 0
+#define SMC_RELEASE_1 1
+#define SMC_RELEASE	SMC_RELEASE_1 /* the latest release version */
 
 #define SMCPROTO_SMC		0	/* SMC protocol, IPv4 */
 #define SMCPROTO_SMC6		1	/* SMC protocol, IPv6 */
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index b9b8b07aa702..4ae27bf65732 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -420,11 +420,11 @@ smc_clc_msg_decl_valid(struct smc_clc_msg_decline *dclc)
 	return true;
 }
 
-static void smc_clc_fill_fce(struct smc_clc_first_contact_ext *fce, int *len)
+static void smc_clc_fill_fce(struct smc_clc_first_contact_ext *fce, int *len, int release_ver)
 {
 	memset(fce, 0, sizeof(*fce));
 	fce->os_type = SMC_CLC_OS_LINUX;
-	fce->release = SMC_RELEASE;
+	fce->release = release_ver;
 	memcpy(fce->hostname, smc_hostname, sizeof(smc_hostname));
 	(*len) += sizeof(*fce);
 }
@@ -1019,7 +1019,7 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 				memcpy(clc_v2->d1.eid, eid, SMC_MAX_EID_LEN);
 			len = SMCD_CLC_ACCEPT_CONFIRM_LEN_V2;
 			if (first_contact)
-				smc_clc_fill_fce(&fce, &len);
+				smc_clc_fill_fce(&fce, &len, ini->release_ver);
 			clc_v2->hdr.length = htons(len);
 		}
 		memcpy(trl.eyecatcher, SMCD_EYECATCHER,
@@ -1063,10 +1063,10 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 				memcpy(clc_v2->r1.eid, eid, SMC_MAX_EID_LEN);
 			len = SMCR_CLC_ACCEPT_CONFIRM_LEN_V2;
 			if (first_contact) {
-				smc_clc_fill_fce(&fce, &len);
+				smc_clc_fill_fce(&fce, &len, ini->release_ver);
 				fce.v2_direct = !link->lgr->uses_gateway;
 				memset(&gle, 0, sizeof(gle));
-				if (ini && clc->hdr.type == SMC_CLC_CONFIRM) {
+				if (clc->hdr.type == SMC_CLC_CONFIRM) {
 					gle.gid_cnt = ini->smcrv2.gidlist.len;
 					len += sizeof(gle);
 					len += gle.gid_cnt * sizeof(gle.gid[0]);
@@ -1141,7 +1141,7 @@ int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact,
 
 /* send CLC ACCEPT message across internal TCP socket */
 int smc_clc_send_accept(struct smc_sock *new_smc, bool srv_first_contact,
-			u8 version, u8 *negotiated_eid)
+			u8 version, u8 *negotiated_eid, struct smc_init_info *ini)
 {
 	struct smc_clc_msg_accept_confirm_v2 aclc_v2;
 	int len;
@@ -1149,7 +1149,7 @@ int smc_clc_send_accept(struct smc_sock *new_smc, bool srv_first_contact,
 	memset(&aclc_v2, 0, sizeof(aclc_v2));
 	aclc_v2.hdr.type = SMC_CLC_ACCEPT;
 	len = smc_clc_send_confirm_accept(new_smc, &aclc_v2, srv_first_contact,
-					  version, negotiated_eid, NULL);
+					  version, negotiated_eid, ini);
 	if (len < ntohs(aclc_v2.hdr.length))
 		len = len >= 0 ? -EPROTO : -new_smc->clcsock->sk->sk_err;
 
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 5fee545c9a10..b923e89acafb 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -370,6 +370,27 @@ smc_get_clc_smcd_v2_ext(struct smc_clc_v2_extension *prop_v2ext)
 		 ntohs(prop_v2ext->hdr.smcd_v2_ext_offset));
 }
 
+static inline struct smc_clc_first_contact_ext *
+smc_get_clc_first_contact_ext(struct smc_clc_msg_accept_confirm_v2 *clc_v2,
+			      bool is_smcd)
+{
+	int clc_v2_len;
+
+	if (clc_v2->hdr.version == SMC_V1 ||
+	    !(clc_v2->hdr.typev2 & SMC_FIRST_CONTACT_MASK))
+		return NULL;
+
+	if (is_smcd)
+		clc_v2_len =
+			offsetofend(struct smc_clc_msg_accept_confirm_v2, d1);
+	else
+		clc_v2_len =
+			offsetofend(struct smc_clc_msg_accept_confirm_v2, r1);
+
+	return (struct smc_clc_first_contact_ext *)(((u8 *)clc_v2) +
+						    clc_v2_len);
+}
+
 struct smcd_dev;
 struct smc_init_info;
 
@@ -382,7 +403,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini);
 int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact,
 			 u8 version, u8 *eid, struct smc_init_info *ini);
 int smc_clc_send_accept(struct smc_sock *smc, bool srv_first_contact,
-			u8 version, u8 *negotiated_eid);
+			u8 version, u8 *negotiated_eid, struct smc_init_info *ini);
 void smc_clc_init(void) __init;
 void smc_clc_exit(void);
 void smc_clc_get_hostname(u8 **host);
diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
index 3c1b31bfa1cf..1a97fef39127 100644
--- a/net/smc/smc_core.h
+++ b/net/smc/smc_core.h
@@ -374,6 +374,7 @@ struct smc_init_info {
 	u8			is_smcd;
 	u8			smc_type_v1;
 	u8			smc_type_v2;
+	u8			release_ver;
 	u8			first_contact_peer;
 	u8			first_contact_local;
 	unsigned short		vlan_id;
-- 
2.24.3 (Apple Git-128)

Re: [RFC PATCH v2 net-next 1/6] net/smc: support smc release version negotiation in clc handshake

[Wenjia Zhang]

On 07.08.23 08:27, Guangguan Wang wrote:
> Support smc release version negotiation in clc handshake. And set
> the latest smc release version to 2.1.
> 

Could you elaborate the changes? Without reading code, it is really 
difficult to know what you did, and why you did it. Sure, one can read 
the code and the support document, but the commit message should always 
be the quick reference. The following information I missed especially:
- This implementation is based on SMCv2 where no negotiation process for 
different releases, but for different versions.
- The Server makes the decision for which release will be used.

> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
> Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
> ---
>   net/smc/af_smc.c   | 22 ++++++++++++++++++++--
>   net/smc/smc.h      |  5 ++++-
>   net/smc/smc_clc.c  | 14 +++++++-------
>   net/smc/smc_clc.h  | 23 ++++++++++++++++++++++-
>   net/smc/smc_core.h |  1 +
>   5 files changed, 54 insertions(+), 11 deletions(-)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index a7f887d91d89..bac73eb0542d 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1187,6 +1187,11 @@ static int smc_connect_rdma_v2_prepare(struct smc_sock *smc,
>   			return SMC_CLC_DECL_NOINDIRECT;
>   		}
>   	}
> +
> +	if (fce->release > SMC_RELEASE)
> +		return SMC_CLC_DECL_VERSMISMAT;
I'm wondering if this check is necessary, how it could happen?

> +	ini->release_ver = fce->release;
> +
>   	return 0;
>   }
>   
> @@ -1355,6 +1360,15 @@ static int smc_connect_ism(struct smc_sock *smc,
>   		struct smc_clc_msg_accept_confirm_v2 *aclc_v2 =
>   			(struct smc_clc_msg_accept_confirm_v2 *)aclc;
>   
> +		if (ini->first_contact_peer) {
> +			struct smc_clc_first_contact_ext *fce =
> +				smc_get_clc_first_contact_ext(aclc_v2, true);
> +
> +			if (fce->release > SMC_RELEASE)
> +				return SMC_CLC_DECL_VERSMISMAT;
> +			ini->release_ver = fce->release;
> +		}
> +
>   		rc = smc_v2_determine_accepted_chid(aclc_v2, ini);
>   		if (rc)
>   			return rc;
> @@ -1389,7 +1403,7 @@ static int smc_connect_ism(struct smc_sock *smc,
>   	}
>   
>   	rc = smc_clc_send_confirm(smc, ini->first_contact_local,
> -				  aclc->hdr.version, eid, NULL);
> +				  aclc->hdr.version, eid, ini);
>   	if (rc)
>   		goto connect_abort;
>   	mutex_unlock(&smc_server_lgr_pending);
> @@ -1965,6 +1979,10 @@ static int smc_listen_v2_check(struct smc_sock *new_smc,
>   		}
>   	}
>   
> +	ini->release_ver = pclc_v2_ext->hdr.flag.release;
> +	if (pclc_v2_ext->hdr.flag.release > SMC_RELEASE)
> +		ini->release_ver = SMC_RELEASE;
> +
>   out:
>   	if (!ini->smcd_version && !ini->smcr_version)
>   		return rc;
> @@ -2412,7 +2430,7 @@ static void smc_listen_work(struct work_struct *work)
>   	/* send SMC Accept CLC message */
>   	accept_version = ini->is_smcd ? ini->smcd_version : ini->smcr_version;
>   	rc = smc_clc_send_accept(new_smc, ini->first_contact_local,
> -				 accept_version, ini->negotiated_eid);
> +				 accept_version, ini->negotiated_eid, ini);
>   	if (rc)
>   		goto out_unlock;
>   
> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 2eeea4cdc718..9cce1a41e011 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -21,7 +21,10 @@
>   
>   #define SMC_V1		1		/* SMC version V1 */
>   #define SMC_V2		2		/* SMC version V2 */
> -#define SMC_RELEASE	0
> +
> +#define SMC_RELEASE_0 0
> +#define SMC_RELEASE_1 1
> +#define SMC_RELEASE	SMC_RELEASE_1 /* the latest release version */
>   
>   #define SMCPROTO_SMC		0	/* SMC protocol, IPv4 */
>   #define SMCPROTO_SMC6		1	/* SMC protocol, IPv6 */
> diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
> index b9b8b07aa702..4ae27bf65732 100644
> --- a/net/smc/smc_clc.c
> +++ b/net/smc/smc_clc.c
> @@ -420,11 +420,11 @@ smc_clc_msg_decl_valid(struct smc_clc_msg_decline *dclc)
>   	return true;
>   }
>   
> -static void smc_clc_fill_fce(struct smc_clc_first_contact_ext *fce, int *len)
> +static void smc_clc_fill_fce(struct smc_clc_first_contact_ext *fce, int *len, int release_ver)
>   {
>   	memset(fce, 0, sizeof(*fce));
>   	fce->os_type = SMC_CLC_OS_LINUX;
> -	fce->release = SMC_RELEASE;
> +	fce->release = release_ver;
>   	memcpy(fce->hostname, smc_hostname, sizeof(smc_hostname));
>   	(*len) += sizeof(*fce);
>   }

Personally I'd like release_nr instead of release_ver.

> @@ -1019,7 +1019,7 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
>   				memcpy(clc_v2->d1.eid, eid, SMC_MAX_EID_LEN);
>   			len = SMCD_CLC_ACCEPT_CONFIRM_LEN_V2;
>   			if (first_contact)
> -				smc_clc_fill_fce(&fce, &len);
> +				smc_clc_fill_fce(&fce, &len, ini->release_ver);
>   			clc_v2->hdr.length = htons(len);
>   		}
>   		memcpy(trl.eyecatcher, SMCD_EYECATCHER,
> @@ -1063,10 +1063,10 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
>   				memcpy(clc_v2->r1.eid, eid, SMC_MAX_EID_LEN);
>   			len = SMCR_CLC_ACCEPT_CONFIRM_LEN_V2;
>   			if (first_contact) {
> -				smc_clc_fill_fce(&fce, &len);
> +				smc_clc_fill_fce(&fce, &len, ini->release_ver);
>   				fce.v2_direct = !link->lgr->uses_gateway;
>   				memset(&gle, 0, sizeof(gle));
> -				if (ini && clc->hdr.type == SMC_CLC_CONFIRM) {
> +				if (clc->hdr.type == SMC_CLC_CONFIRM) {
>   					gle.gid_cnt = ini->smcrv2.gidlist.len;
>   					len += sizeof(gle);
>   					len += gle.gid_cnt * sizeof(gle.gid[0]);
> @@ -1141,7 +1141,7 @@ int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact,
>   
>   /* send CLC ACCEPT message across internal TCP socket */
>   int smc_clc_send_accept(struct smc_sock *new_smc, bool srv_first_contact,
> -			u8 version, u8 *negotiated_eid)
> +			u8 version, u8 *negotiated_eid, struct smc_init_info *ini)
>   {
>   	struct smc_clc_msg_accept_confirm_v2 aclc_v2;
>   	int len;
> @@ -1149,7 +1149,7 @@ int smc_clc_send_accept(struct smc_sock *new_smc, bool srv_first_contact,
>   	memset(&aclc_v2, 0, sizeof(aclc_v2));
>   	aclc_v2.hdr.type = SMC_CLC_ACCEPT;
>   	len = smc_clc_send_confirm_accept(new_smc, &aclc_v2, srv_first_contact,
> -					  version, negotiated_eid, NULL);
> +					  version, negotiated_eid, ini);
>   	if (len < ntohs(aclc_v2.hdr.length))
>   		len = len >= 0 ? -EPROTO : -new_smc->clcsock->sk->sk_err;
>   
> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
> index 5fee545c9a10..b923e89acafb 100644
> --- a/net/smc/smc_clc.h
> +++ b/net/smc/smc_clc.h
> @@ -370,6 +370,27 @@ smc_get_clc_smcd_v2_ext(struct smc_clc_v2_extension *prop_v2ext)
>   		 ntohs(prop_v2ext->hdr.smcd_v2_ext_offset));
>   }
>   
> +static inline struct smc_clc_first_contact_ext *
> +smc_get_clc_first_contact_ext(struct smc_clc_msg_accept_confirm_v2 *clc_v2,
> +			      bool is_smcd)
> +{
> +	int clc_v2_len;
> +
> +	if (clc_v2->hdr.version == SMC_V1 ||
> +	    !(clc_v2->hdr.typev2 & SMC_FIRST_CONTACT_MASK))
> +		return NULL;
> +
> +	if (is_smcd)
> +		clc_v2_len =
> +			offsetofend(struct smc_clc_msg_accept_confirm_v2, d1);
> +	else
> +		clc_v2_len =
> +			offsetofend(struct smc_clc_msg_accept_confirm_v2, r1);
> +
> +	return (struct smc_clc_first_contact_ext *)(((u8 *)clc_v2) +
> +						    clc_v2_len);
> +}
> +
>   struct smcd_dev;
>   struct smc_init_info;
>   
> @@ -382,7 +403,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini);
>   int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact,
>   			 u8 version, u8 *eid, struct smc_init_info *ini);
>   int smc_clc_send_accept(struct smc_sock *smc, bool srv_first_contact,
> -			u8 version, u8 *negotiated_eid);
> +			u8 version, u8 *negotiated_eid, struct smc_init_info *ini);
>   void smc_clc_init(void) __init;
>   void smc_clc_exit(void);
>   void smc_clc_get_hostname(u8 **host);
> diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
> index 3c1b31bfa1cf..1a97fef39127 100644
> --- a/net/smc/smc_core.h
> +++ b/net/smc/smc_core.h
> @@ -374,6 +374,7 @@ struct smc_init_info {
>   	u8			is_smcd;
>   	u8			smc_type_v1;
>   	u8			smc_type_v2;
> +	u8			release_ver;

Also here, I'd like release_nr more.

>   	u8			first_contact_peer;
>   	u8			first_contact_local;
>   	unsigned short		vlan_id;

Re: [RFC PATCH v2 net-next 1/6] net/smc: support smc release version negotiation in clc handshake

[Guangguan Wang]

On 2023/8/10 00:03, Wenjia Zhang wrote:
> 
> 
> On 07.08.23 08:27, Guangguan Wang wrote:
>> Support smc release version negotiation in clc handshake. And set
>> the latest smc release version to 2.1.
>>
> 
> Could you elaborate the changes? Without reading code, it is really difficult to know what you did, and why you did it. Sure, one can read the code and the support document, but the commit message should always be the quick reference. The following information I missed especially:
> - This implementation is based on SMCv2 where no negotiation process for different releases, but for different versions.
> - The Server makes the decision for which release will be used.

Sorry for the lack of descriptions, more descriptions will be added in the next version.

>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index a7f887d91d89..bac73eb0542d 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -1187,6 +1187,11 @@ static int smc_connect_rdma_v2_prepare(struct smc_sock *smc,
>>               return SMC_CLC_DECL_NOINDIRECT;
>>           }
>>       }
>> +
>> +    if (fce->release > SMC_RELEASE)
>> +        return SMC_CLC_DECL_VERSMISMAT;
> I'm wondering if this check is necessary, how it could happen?

You are right, I will remove the check.

>>   -static void smc_clc_fill_fce(struct smc_clc_first_contact_ext *fce, int *len)
>> +static void smc_clc_fill_fce(struct smc_clc_first_contact_ext *fce, int *len, int release_ver)
>>   {
>>       memset(fce, 0, sizeof(*fce));
>>       fce->os_type = SMC_CLC_OS_LINUX;
>> -    fce->release = SMC_RELEASE;
>> +    fce->release = release_ver;
>>       memcpy(fce->hostname, smc_hostname, sizeof(smc_hostname));
>>       (*len) += sizeof(*fce);
>>   }
> 
> Personally I'd like release_nr instead of release_ver.


>>   @@ -382,7 +403,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini);
>>   int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact,
>>                u8 version, u8 *eid, struct smc_init_info *ini);
>>   int smc_clc_send_accept(struct smc_sock *smc, bool srv_first_contact,
>> -            u8 version, u8 *negotiated_eid);
>> +            u8 version, u8 *negotiated_eid, struct smc_init_info *ini);
>>   void smc_clc_init(void) __init;
>>   void smc_clc_exit(void);
>>   void smc_clc_get_hostname(u8 **host);
>> diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
>> index 3c1b31bfa1cf..1a97fef39127 100644
>> --- a/net/smc/smc_core.h
>> +++ b/net/smc/smc_core.h
>> @@ -374,6 +374,7 @@ struct smc_init_info {
>>       u8            is_smcd;
>>       u8            smc_type_v1;
>>       u8            smc_type_v2;
>> +    u8            release_ver;
> 
> Also here, I'd like release_nr more.

OK, I will modify it in the next version.

[RFC PATCH v2 net-next 2/6] net/smc: add vendor unique experimental options area in clc handshake

[Guangguan Wang]

Add vendor unique experimental options area in clc handshake. In clc
accept and confirm msg, vendor unique experimental options use the
16-Bytes reserved field, which defined in struct smc_clc_fce_gid_ext
in previous version. Because of the struct smc_clc_first_contact_ext
is widely used and limit the scope of modification, this patch moves
the 16-Bytes reserved field out of struct smc_clc_fce_gid_ext, and
followed with the struct smc_clc_first_contact_ext in a new struct
names struct smc_clc_first_contact_ext_v2x.

For SMC-R first connection, in previous version, the struct smc_clc_
first_contact_ext and the 16-Bytes reserved field has already been
included in clc accept and confirm msg. Thus, this patch use struct
smc_clc_first_contact_ext_v2x instead of the struct smc_clc_first_
contact_ext and the 16-Bytes reserved field in SMC-R clc accept and
confirm msg is compatible with previous version.

For SMC-D first connection, in previous version, only the struct smc_
clc_first_contact_ext is included in clc accept and confirm msg, and
the 16-Bytes reserved field is not included. Thus, when the negotiated
smc release version is the version before v2.1, we still use struct
smc_clc_first_contact_ext for compatible consideration. If the negotiated
smc release version is v2.1 or later, use struct smc_clc_first_contact_
ext_v2x instead.

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
---
 net/smc/af_smc.c  |  2 +-
 net/smc/smc_clc.c | 44 +++++++++++++++++++++++---------------------
 net/smc/smc_clc.h | 15 +++++++++++++--
 3 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index bac73eb0542d..52279bdc100a 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1113,7 +1113,7 @@ static int smc_connect_ism_vlan_cleanup(struct smc_sock *smc,
 
 #define SMC_CLC_MAX_ACCEPT_LEN \
 	(sizeof(struct smc_clc_msg_accept_confirm_v2) + \
-	 sizeof(struct smc_clc_first_contact_ext) + \
+	 sizeof(struct smc_clc_first_contact_ext_v2x) + \
 	 sizeof(struct smc_clc_msg_trail))
 
 /* CLC handshake during connect */
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index 4ae27bf65732..ae80c191a834 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -391,9 +391,7 @@ smc_clc_msg_acc_conf_valid(struct smc_clc_msg_accept_confirm_v2 *clc_v2)
 			return false;
 	} else {
 		if (hdr->typev1 == SMC_TYPE_D &&
-		    ntohs(hdr->length) != SMCD_CLC_ACCEPT_CONFIRM_LEN_V2 &&
-		    (ntohs(hdr->length) != SMCD_CLC_ACCEPT_CONFIRM_LEN_V2 +
-				sizeof(struct smc_clc_first_contact_ext)))
+		    ntohs(hdr->length) < SMCD_CLC_ACCEPT_CONFIRM_LEN_V2)
 			return false;
 		if (hdr->typev1 == SMC_TYPE_R &&
 		    ntohs(hdr->length) < SMCR_CLC_ACCEPT_CONFIRM_LEN_V2)
@@ -420,13 +418,19 @@ smc_clc_msg_decl_valid(struct smc_clc_msg_decline *dclc)
 	return true;
 }
 
-static void smc_clc_fill_fce(struct smc_clc_first_contact_ext *fce, int *len, int release_ver)
+static int smc_clc_fill_fce(struct smc_clc_first_contact_ext_v2x *fce,
+			    struct smc_init_info *ini)
 {
+	int ret = sizeof(*fce);
+
 	memset(fce, 0, sizeof(*fce));
-	fce->os_type = SMC_CLC_OS_LINUX;
-	fce->release = release_ver;
-	memcpy(fce->hostname, smc_hostname, sizeof(smc_hostname));
-	(*len) += sizeof(*fce);
+	fce->fce_v20.os_type = SMC_CLC_OS_LINUX;
+	fce->fce_v20.release = ini->release_ver;
+	memcpy(fce->fce_v20.hostname, smc_hostname, sizeof(smc_hostname));
+	if (ini->is_smcd && ini->release_ver < SMC_RELEASE_1)
+		ret = sizeof(struct smc_clc_first_contact_ext);
+
+	return ret;
 }
 
 /* check if received message has a correct header length and contains valid
@@ -986,13 +990,13 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 				       u8 *eid, struct smc_init_info *ini)
 {
 	struct smc_connection *conn = &smc->conn;
+	struct smc_clc_first_contact_ext_v2x fce;
 	struct smc_clc_msg_accept_confirm *clc;
-	struct smc_clc_first_contact_ext fce;
 	struct smc_clc_fce_gid_ext gle;
 	struct smc_clc_msg_trail trl;
+	int i, len, fce_len;
 	struct kvec vec[5];
 	struct msghdr msg;
-	int i, len;
 
 	/* send SMC Confirm CLC msg */
 	clc = (struct smc_clc_msg_accept_confirm *)clc_v2;
@@ -1018,8 +1022,10 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 			if (eid && eid[0])
 				memcpy(clc_v2->d1.eid, eid, SMC_MAX_EID_LEN);
 			len = SMCD_CLC_ACCEPT_CONFIRM_LEN_V2;
-			if (first_contact)
-				smc_clc_fill_fce(&fce, &len, ini->release_ver);
+			if (first_contact) {
+				fce_len = smc_clc_fill_fce(&fce, ini);
+				len += fce_len;
+			}
 			clc_v2->hdr.length = htons(len);
 		}
 		memcpy(trl.eyecatcher, SMCD_EYECATCHER,
@@ -1063,15 +1069,14 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 				memcpy(clc_v2->r1.eid, eid, SMC_MAX_EID_LEN);
 			len = SMCR_CLC_ACCEPT_CONFIRM_LEN_V2;
 			if (first_contact) {
-				smc_clc_fill_fce(&fce, &len, ini->release_ver);
-				fce.v2_direct = !link->lgr->uses_gateway;
-				memset(&gle, 0, sizeof(gle));
+				fce_len = smc_clc_fill_fce(&fce, ini);
+				len += fce_len;
+				fce.fce_v20.v2_direct = !link->lgr->uses_gateway;
 				if (clc->hdr.type == SMC_CLC_CONFIRM) {
+					memset(&gle, 0, sizeof(gle));
 					gle.gid_cnt = ini->smcrv2.gidlist.len;
 					len += sizeof(gle);
 					len += gle.gid_cnt * sizeof(gle.gid[0]);
-				} else {
-					len += sizeof(gle.reserved);
 				}
 			}
 			clc_v2->hdr.length = htons(len);
@@ -1094,7 +1099,7 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 				   sizeof(trl);
 	if (version > SMC_V1 && first_contact) {
 		vec[i].iov_base = &fce;
-		vec[i++].iov_len = sizeof(fce);
+		vec[i++].iov_len = fce_len;
 		if (!conn->lgr->is_smcd) {
 			if (clc->hdr.type == SMC_CLC_CONFIRM) {
 				vec[i].iov_base = &gle;
@@ -1102,9 +1107,6 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 				vec[i].iov_base = &ini->smcrv2.gidlist.list;
 				vec[i++].iov_len = gle.gid_cnt *
 						   sizeof(gle.gid[0]);
-			} else {
-				vec[i].iov_base = &gle.reserved;
-				vec[i++].iov_len = sizeof(gle.reserved);
 			}
 		}
 	}
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index b923e89acafb..6133276a8839 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -147,7 +147,9 @@ struct smc_clc_msg_proposal_prefix {	/* prefix part of clc proposal message*/
 struct smc_clc_msg_smcd {	/* SMC-D GID information */
 	struct smc_clc_smcd_gid_chid ism; /* ISM native GID+CHID of requestor */
 	__be16 v2_ext_offset;	/* SMC Version 2 Extension Offset */
-	u8 reserved[28];
+	u8 vendor_oui[3];
+	u8 vendor_exp_options[5];
+	u8 reserved[20];
 };
 
 struct smc_clc_smcd_v2_extension {
@@ -231,8 +233,17 @@ struct smc_clc_first_contact_ext {
 	u8 hostname[SMC_MAX_HOSTNAME_LEN];
 };
 
+struct smc_clc_first_contact_ext_v2x {
+	struct smc_clc_first_contact_ext fce_v20;
+	u8 reserved3[4];
+	__be32 vendor_exp_options;
+	u8 reserved4[8];
+} __packed;		/* format defined in
+			 * IBM Shared Memory Communications Version 2 (Third Edition)
+			 * (https://www.ibm.com/support/pages/node/7009315)
+			 */
+
 struct smc_clc_fce_gid_ext {
-	u8 reserved[16];
 	u8 gid_cnt;
 	u8 reserved2[3];
 	u8 gid[][SMC_GID_SIZE];
-- 
2.24.3 (Apple Git-128)

[RFC PATCH v2 net-next 3/6] net/smc: support smc v2.x features validate

[Guangguan Wang]

Support smc v2.x features validate for smc v2.1.

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
---
 net/smc/af_smc.c  | 18 ++++++++++++++++++
 net/smc/smc_clc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 net/smc/smc_clc.h |  7 +++++++
 3 files changed, 71 insertions(+)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 52279bdc100a..fd58e25beddf 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1169,6 +1169,7 @@ static int smc_connect_rdma_v2_prepare(struct smc_sock *smc,
 	struct smc_clc_first_contact_ext *fce =
 		(struct smc_clc_first_contact_ext *)
 			(((u8 *)clc_v2) + sizeof(*clc_v2));
+	int rc;
 
 	if (!ini->first_contact_peer || aclc->hdr.version == SMC_V1)
 		return 0;
@@ -1191,6 +1192,9 @@ static int smc_connect_rdma_v2_prepare(struct smc_sock *smc,
 	if (fce->release > SMC_RELEASE)
 		return SMC_CLC_DECL_VERSMISMAT;
 	ini->release_ver = fce->release;
+	rc = smc_clc_cli_v2x_features_validate(fce, ini);
+	if (rc)
+		return rc;
 
 	return 0;
 }
@@ -1367,6 +1371,9 @@ static int smc_connect_ism(struct smc_sock *smc,
 			if (fce->release > SMC_RELEASE)
 				return SMC_CLC_DECL_VERSMISMAT;
 			ini->release_ver = fce->release;
+			rc = smc_clc_cli_v2x_features_validate(fce, ini);
+			if (rc)
+				return rc;
 		}
 
 		rc = smc_v2_determine_accepted_chid(aclc_v2, ini);
@@ -2417,6 +2424,10 @@ static void smc_listen_work(struct work_struct *work)
 	if (rc)
 		goto out_decl;
 
+	rc = smc_clc_srv_v2x_features_validate(pclc, ini);
+	if (rc)
+		goto out_decl;
+
 	mutex_lock(&smc_server_lgr_pending);
 	smc_close_init(new_smc);
 	smc_rx_init(new_smc);
@@ -2449,6 +2460,13 @@ static void smc_listen_work(struct work_struct *work)
 		goto out_decl;
 	}
 
+	rc = smc_clc_v2x_features_confirm_check(cclc, ini);
+	if (rc) {
+		if (!ini->is_smcd)
+			goto out_unlock;
+		goto out_decl;
+	}
+
 	/* finish worker */
 	if (!ini->is_smcd) {
 		rc = smc_listen_rdma_finish(new_smc, cclc,
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index ae80c191a834..4f6b69af2b80 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -1158,6 +1158,52 @@ int smc_clc_send_accept(struct smc_sock *new_smc, bool srv_first_contact,
 	return len > 0 ? 0 : len;
 }
 
+int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc,
+				      struct smc_init_info *ini)
+{
+	struct smc_clc_v2_extension *pclc_v2_ext;
+
+	if ((!(ini->smcd_version & SMC_V2) && !(ini->smcr_version & SMC_V2)) ||
+	    ini->release_ver < SMC_RELEASE_1)
+		return 0;
+
+	pclc_v2_ext = smc_get_clc_v2_ext(pclc);
+	if (!pclc_v2_ext)
+		return SMC_CLC_DECL_NOV2EXT;
+
+	return 0;
+}
+
+int smc_clc_cli_v2x_features_validate(struct smc_clc_first_contact_ext *fce,
+				      struct smc_init_info *ini)
+{
+	if (ini->release_ver < SMC_RELEASE_1)
+		return 0;
+
+	return 0;
+}
+
+int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc,
+				       struct smc_init_info *ini)
+{
+	struct smc_clc_msg_accept_confirm_v2 *clc_v2 =
+		(struct smc_clc_msg_accept_confirm_v2 *)cclc;
+	struct smc_clc_first_contact_ext *fce =
+		smc_get_clc_first_contact_ext(clc_v2, ini->is_smcd);
+
+	if (cclc->hdr.version == SMC_V1 ||
+	    !(cclc->hdr.typev2 & SMC_FIRST_CONTACT_MASK))
+		return 0;
+
+	if (ini->release_ver != fce->release)
+		return SMC_CLC_DECL_RELEASEERR;
+
+	if (fce->release < SMC_RELEASE_1)
+		return 0;
+
+	return 0;
+}
+
 void smc_clc_get_hostname(u8 **host)
 {
 	*host = &smc_hostname[0];
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 6133276a8839..66932bfdc6d0 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -45,6 +45,7 @@
 #define SMC_CLC_DECL_NOSEID	0x03030006  /* peer sent no SEID	      */
 #define SMC_CLC_DECL_NOSMCD2DEV	0x03030007  /* no SMC-Dv2 device found	      */
 #define SMC_CLC_DECL_NOUEID	0x03030008  /* peer sent no UEID	      */
+#define SMC_CLC_DECL_RELEASEERR	0x03030009  /* release version negotiate failed */
 #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 */
@@ -415,6 +416,12 @@ int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact,
 			 u8 version, u8 *eid, struct smc_init_info *ini);
 int smc_clc_send_accept(struct smc_sock *smc, bool srv_first_contact,
 			u8 version, u8 *negotiated_eid, struct smc_init_info *ini);
+int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc,
+				      struct smc_init_info *ini);
+int smc_clc_cli_v2x_features_validate(struct smc_clc_first_contact_ext *fce,
+				      struct smc_init_info *ini);
+int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc,
+				       struct smc_init_info *ini);
 void smc_clc_init(void) __init;
 void smc_clc_exit(void);
 void smc_clc_get_hostname(u8 **host);
-- 
2.24.3 (Apple Git-128)

Re: [RFC PATCH v2 net-next 3/6] net/smc: support smc v2.x features validate

[Wenjia Zhang]

On 07.08.23 08:27, Guangguan Wang wrote:
> Support smc v2.x features validate for smc v2.1.
> 
a bit more description?

> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
> Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
> ---
>   net/smc/af_smc.c  | 18 ++++++++++++++++++
>   net/smc/smc_clc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>   net/smc/smc_clc.h |  7 +++++++
>   3 files changed, 71 insertions(+)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 52279bdc100a..fd58e25beddf 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1169,6 +1169,7 @@ static int smc_connect_rdma_v2_prepare(struct smc_sock *smc,
>   	struct smc_clc_first_contact_ext *fce =
>   		(struct smc_clc_first_contact_ext *)
>   			(((u8 *)clc_v2) + sizeof(*clc_v2));
> +	int rc;
>   
>   	if (!ini->first_contact_peer || aclc->hdr.version == SMC_V1)
>   		return 0;
> @@ -1191,6 +1192,9 @@ static int smc_connect_rdma_v2_prepare(struct smc_sock *smc,
>   	if (fce->release > SMC_RELEASE)
>   		return SMC_CLC_DECL_VERSMISMAT;
>   	ini->release_ver = fce->release;
> +	rc = smc_clc_cli_v2x_features_validate(fce, ini);
> +	if (rc)
> +		return rc;
>   
>   	return 0;
>   }
> @@ -1367,6 +1371,9 @@ static int smc_connect_ism(struct smc_sock *smc,
>   			if (fce->release > SMC_RELEASE)
>   				return SMC_CLC_DECL_VERSMISMAT;
>   			ini->release_ver = fce->release;
> +			rc = smc_clc_cli_v2x_features_validate(fce, ini);
> +			if (rc)
> +				return rc;
>   		}
>   
>   		rc = smc_v2_determine_accepted_chid(aclc_v2, ini);
> @@ -2417,6 +2424,10 @@ static void smc_listen_work(struct work_struct *work)
>   	if (rc)
>   		goto out_decl;
>   
> +	rc = smc_clc_srv_v2x_features_validate(pclc, ini);
> +	if (rc)
> +		goto out_decl;
> +
>   	mutex_lock(&smc_server_lgr_pending);
>   	smc_close_init(new_smc);
>   	smc_rx_init(new_smc);
> @@ -2449,6 +2460,13 @@ static void smc_listen_work(struct work_struct *work)
>   		goto out_decl;
>   	}
>   
> +	rc = smc_clc_v2x_features_confirm_check(cclc, ini);
> +	if (rc) {
> +		if (!ini->is_smcd)
> +			goto out_unlock;
> +		goto out_decl;
> +	}
> +
>   	/* finish worker */
>   	if (!ini->is_smcd) {
>   		rc = smc_listen_rdma_finish(new_smc, cclc,
> diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
> index ae80c191a834..4f6b69af2b80 100644
> --- a/net/smc/smc_clc.c
> +++ b/net/smc/smc_clc.c
> @@ -1158,6 +1158,52 @@ int smc_clc_send_accept(struct smc_sock *new_smc, bool srv_first_contact,
>   	return len > 0 ? 0 : len;
>   }
>   
> +int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc,
> +				      struct smc_init_info *ini)
> +{
> +	struct smc_clc_v2_extension *pclc_v2_ext;
> +
> +	if ((!(ini->smcd_version & SMC_V2) && !(ini->smcr_version & SMC_V2)) ||
> +	    ini->release_ver < SMC_RELEASE_1)
> +		return 0;
> +
> +	pclc_v2_ext = smc_get_clc_v2_ext(pclc);
> +	if (!pclc_v2_ext)
> +		return SMC_CLC_DECL_NOV2EXT;
> +
> +	return 0;
> +}
> +
> +int smc_clc_cli_v2x_features_validate(struct smc_clc_first_contact_ext *fce,
> +				      struct smc_init_info *ini)
> +{
> +	if (ini->release_ver < SMC_RELEASE_1)
> +		return 0;
> +
> +	return 0;
> +}

Why need the function? Since it returns 0 anyway.
> +
> +int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc,
> +				       struct smc_init_info *ini)
> +{
> +	struct smc_clc_msg_accept_confirm_v2 *clc_v2 =
> +		(struct smc_clc_msg_accept_confirm_v2 *)cclc;
> +	struct smc_clc_first_contact_ext *fce =
> +		smc_get_clc_first_contact_ext(clc_v2, ini->is_smcd);
> +
> +	if (cclc->hdr.version == SMC_V1 ||
> +	    !(cclc->hdr.typev2 & SMC_FIRST_CONTACT_MASK))
> +		return 0;
> +
> +	if (ini->release_ver != fce->release)
> +		return SMC_CLC_DECL_RELEASEERR;
> +
> +	if (fce->release < SMC_RELEASE_1)
> +		return 0;
> +
> +	return 0;
> +}
> +
>   void smc_clc_get_hostname(u8 **host)
>   {
>   	*host = &smc_hostname[0];
> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
> index 6133276a8839..66932bfdc6d0 100644
> --- a/net/smc/smc_clc.h
> +++ b/net/smc/smc_clc.h
> @@ -45,6 +45,7 @@
>   #define SMC_CLC_DECL_NOSEID	0x03030006  /* peer sent no SEID	      */
>   #define SMC_CLC_DECL_NOSMCD2DEV	0x03030007  /* no SMC-Dv2 device found	      */
>   #define SMC_CLC_DECL_NOUEID	0x03030008  /* peer sent no UEID	      */
> +#define SMC_CLC_DECL_RELEASEERR	0x03030009  /* release version negotiate failed */
>   #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 */
> @@ -415,6 +416,12 @@ int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact,
>   			 u8 version, u8 *eid, struct smc_init_info *ini);
>   int smc_clc_send_accept(struct smc_sock *smc, bool srv_first_contact,
>   			u8 version, u8 *negotiated_eid, struct smc_init_info *ini);
> +int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc,
> +				      struct smc_init_info *ini);
> +int smc_clc_cli_v2x_features_validate(struct smc_clc_first_contact_ext *fce,
> +				      struct smc_init_info *ini);
> +int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc,
> +				       struct smc_init_info *ini);
>   void smc_clc_init(void) __init;
>   void smc_clc_exit(void);
>   void smc_clc_get_hostname(u8 **host);

Re: [RFC PATCH v2 net-next 3/6] net/smc: support smc v2.x features validate

[Guangguan Wang]

On 2023/8/10 00:03, Wenjia Zhang wrote:
> 
> 
> On 07.08.23 08:27, Guangguan Wang wrote:
>> Support smc v2.x features validate for smc v2.1.
>>
> a bit more description?
> 

OK, more descriptions will be added in the next version.

>> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
>> Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>

[RFC PATCH v2 net-next 4/6] net/smc: support max connections per lgr negotiation

[Guangguan Wang]

Support max connections per lgr negotiation for SMCR v2.1,
which is one of smc v2.1 features.

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
---
 net/smc/af_smc.c   |  1 +
 net/smc/smc_clc.c  | 41 +++++++++++++++++++++++++++++++++++++++--
 net/smc/smc_clc.h  |  7 +++++--
 net/smc/smc_core.c |  4 +++-
 net/smc/smc_core.h |  5 +++++
 net/smc/smc_llc.c  |  6 +++++-
 6 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index fd58e25beddf..b95d3fd48c28 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1214,6 +1214,7 @@ static int smc_connect_rdma(struct smc_sock *smc,
 	memcpy(ini->peer_systemid, aclc->r0.lcl.id_for_peer, SMC_SYSTEMID_LEN);
 	memcpy(ini->peer_gid, aclc->r0.lcl.gid, SMC_GID_SIZE);
 	memcpy(ini->peer_mac, aclc->r0.lcl.mac, ETH_ALEN);
+	ini->max_conns = SMC_RMBS_PER_LGR_MAX;
 
 	reason_code = smc_connect_rdma_v2_prepare(smc, aclc, ini);
 	if (reason_code)
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index 4f6b69af2b80..e2b224063dcc 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -427,9 +427,17 @@ static int smc_clc_fill_fce(struct smc_clc_first_contact_ext_v2x *fce,
 	fce->fce_v20.os_type = SMC_CLC_OS_LINUX;
 	fce->fce_v20.release = ini->release_ver;
 	memcpy(fce->fce_v20.hostname, smc_hostname, sizeof(smc_hostname));
-	if (ini->is_smcd && ini->release_ver < SMC_RELEASE_1)
+	if (ini->is_smcd && ini->release_ver < SMC_RELEASE_1) {
 		ret = sizeof(struct smc_clc_first_contact_ext);
+		goto out;
+	}
+
+	if (ini->release_ver >= SMC_RELEASE_1) {
+		if (!ini->is_smcd)
+			fce->max_conns = ini->max_conns;
+	}
 
+out:
 	return ret;
 }
 
@@ -931,8 +939,10 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 				sizeof(struct smc_clc_smcd_gid_chid);
 		}
 	}
-	if (smcr_indicated(ini->smc_type_v2))
+	if (smcr_indicated(ini->smc_type_v2)) {
 		memcpy(v2_ext->roce, ini->smcrv2.ib_gid_v2, SMC_GID_SIZE);
+		v2_ext->max_conns = SMC_CONN_PER_LGR_MAX;
+	}
 
 	pclc_base->hdr.length = htons(plen);
 	memcpy(trl->eyecatcher, SMC_EYECATCHER, sizeof(SMC_EYECATCHER));
@@ -1163,6 +1173,11 @@ int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc,
 {
 	struct smc_clc_v2_extension *pclc_v2_ext;
 
+	/* default max conn is SMC_RMBS_PER_LGR_MAX(255),
+	 * which is the default value in smc v1 and v2.0.
+	 */
+	ini->max_conns = SMC_RMBS_PER_LGR_MAX;
+
 	if ((!(ini->smcd_version & SMC_V2) && !(ini->smcr_version & SMC_V2)) ||
 	    ini->release_ver < SMC_RELEASE_1)
 		return 0;
@@ -1171,15 +1186,30 @@ int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc,
 	if (!pclc_v2_ext)
 		return SMC_CLC_DECL_NOV2EXT;
 
+	if (ini->smcr_version & SMC_V2) {
+		ini->max_conns = min_t(u8, pclc_v2_ext->max_conns, SMC_CONN_PER_LGR_MAX);
+		if (ini->max_conns < SMC_CONN_PER_LGR_MIN)
+			return SMC_CLC_DECL_MAXCONNERR;
+	}
+
 	return 0;
 }
 
 int smc_clc_cli_v2x_features_validate(struct smc_clc_first_contact_ext *fce,
 				      struct smc_init_info *ini)
 {
+	struct smc_clc_first_contact_ext_v2x *fce_v2x =
+		(struct smc_clc_first_contact_ext_v2x *)fce;
+
 	if (ini->release_ver < SMC_RELEASE_1)
 		return 0;
 
+	if (!ini->is_smcd) {
+		if (fce_v2x->max_conns < SMC_CONN_PER_LGR_MIN)
+			return SMC_CLC_DECL_MAXCONNERR;
+		ini->max_conns = fce_v2x->max_conns;
+	}
+
 	return 0;
 }
 
@@ -1190,6 +1220,8 @@ int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc,
 		(struct smc_clc_msg_accept_confirm_v2 *)cclc;
 	struct smc_clc_first_contact_ext *fce =
 		smc_get_clc_first_contact_ext(clc_v2, ini->is_smcd);
+	struct smc_clc_first_contact_ext_v2x *fce_v2x =
+		(struct smc_clc_first_contact_ext_v2x *)fce;
 
 	if (cclc->hdr.version == SMC_V1 ||
 	    !(cclc->hdr.typev2 & SMC_FIRST_CONTACT_MASK))
@@ -1201,6 +1233,11 @@ int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc,
 	if (fce->release < SMC_RELEASE_1)
 		return 0;
 
+	if (!ini->is_smcd) {
+		if (fce_v2x->max_conns != ini->max_conns)
+			return SMC_CLC_DECL_MAXCONNERR;
+	}
+
 	return 0;
 }
 
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 66932bfdc6d0..54077e50c368 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -46,6 +46,7 @@
 #define SMC_CLC_DECL_NOSMCD2DEV	0x03030007  /* no SMC-Dv2 device found	      */
 #define SMC_CLC_DECL_NOUEID	0x03030008  /* peer sent no UEID	      */
 #define SMC_CLC_DECL_RELEASEERR	0x03030009  /* release version negotiate failed */
+#define SMC_CLC_DECL_MAXCONNERR	0x0303000a  /* max connections negotiate failed */
 #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 */
@@ -134,7 +135,8 @@ struct smc_clc_smcd_gid_chid {
 struct smc_clc_v2_extension {
 	struct smc_clnt_opts_area_hdr hdr;
 	u8 roce[16];		/* RoCEv2 GID */
-	u8 reserved[16];
+	u8 max_conns;
+	u8 reserved[15];
 	u8 user_eids[][SMC_MAX_EID_LEN];
 };
 
@@ -236,7 +238,8 @@ struct smc_clc_first_contact_ext {
 
 struct smc_clc_first_contact_ext_v2x {
 	struct smc_clc_first_contact_ext fce_v20;
-	u8 reserved3[4];
+	u8 max_conns; /* for SMC-R only */
+	u8 reserved3[3];
 	__be32 vendor_exp_options;
 	u8 reserved4[8];
 } __packed;		/* format defined in
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 6aa3db47a956..5de1fbaa6e28 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -895,9 +895,11 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini)
 			lgr->uses_gateway = ini->smcrv2.uses_gateway;
 			memcpy(lgr->nexthop_mac, ini->smcrv2.nexthop_mac,
 			       ETH_ALEN);
+			lgr->max_conns = ini->max_conns;
 		} else {
 			ibdev = ini->ib_dev;
 			ibport = ini->ib_port;
+			lgr->max_conns = SMC_RMBS_PER_LGR_MAX;
 		}
 		memcpy(lgr->pnet_id, ibdev->pnetid[ibport - 1],
 		       SMC_MAX_PNETID_LEN);
@@ -1890,7 +1892,7 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
 		    (ini->smcd_version == SMC_V2 ||
 		     lgr->vlan_id == ini->vlan_id) &&
 		    (role == SMC_CLNT || ini->is_smcd ||
-		    (lgr->conns_num < SMC_RMBS_PER_LGR_MAX &&
+		    (lgr->conns_num < lgr->max_conns &&
 		      !bitmap_full(lgr->rtokens_used_mask, SMC_RMBS_PER_LGR_MAX)))) {
 			/* link group found */
 			ini->first_contact_local = 0;
diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
index 1a97fef39127..f4f7299c810a 100644
--- a/net/smc/smc_core.h
+++ b/net/smc/smc_core.h
@@ -22,6 +22,8 @@
 #include "smc_ib.h"
 
 #define SMC_RMBS_PER_LGR_MAX	255	/* max. # of RMBs per link group */
+#define SMC_CONN_PER_LGR_MAX	255	/* max. # of connections per link group */
+#define SMC_CONN_PER_LGR_MIN	16	/* min. # of connections per link group */
 
 struct smc_lgr_list {			/* list of link group definition */
 	struct list_head	list;
@@ -331,6 +333,8 @@ struct smc_link_group {
 			__be32			saddr;
 						/* net namespace */
 			struct net		*net;
+			u8			max_conns;
+						/* max conn can be assigned to lgr */
 		};
 		struct { /* SMC-D */
 			u64			peer_gid;
@@ -375,6 +379,7 @@ struct smc_init_info {
 	u8			smc_type_v1;
 	u8			smc_type_v2;
 	u8			release_ver;
+	u8			max_conns;
 	u8			first_contact_peer;
 	u8			first_contact_local;
 	unsigned short		vlan_id;
diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
index 90f0b60b196a..5347b62f1518 100644
--- a/net/smc/smc_llc.c
+++ b/net/smc/smc_llc.c
@@ -52,7 +52,8 @@ struct smc_llc_msg_confirm_link {	/* type 0x01 */
 	u8 link_num;
 	u8 link_uid[SMC_LGR_ID_SIZE];
 	u8 max_links;
-	u8 reserved[9];
+	u8 max_conns;
+	u8 reserved[8];
 };
 
 #define SMC_LLC_FLAG_ADD_LNK_REJ	0x40
@@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link,
 	confllc->link_num = link->link_id;
 	memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE);
 	confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS;
+	if (link->lgr->smc_version == SMC_V2 &&
+	    link->lgr->peer_smc_release >= SMC_RELEASE_1)
+		confllc->max_conns = link->lgr->max_conns;
 	/* send llc message */
 	rc = smc_wr_tx_send(link, pend);
 put_out:
-- 
2.24.3 (Apple Git-128)

Re: [RFC PATCH v2 net-next 4/6] net/smc: support max connections per lgr negotiation

[Wenjia Zhang]

On 07.08.23 08:27, Guangguan Wang wrote:
> Support max connections per lgr negotiation for SMCR v2.1,
> which is one of smc v2.1 features.
> 
> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
> Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
> ---
>   net/smc/af_smc.c   |  1 +
>   net/smc/smc_clc.c  | 41 +++++++++++++++++++++++++++++++++++++++--
>   net/smc/smc_clc.h  |  7 +++++--
>   net/smc/smc_core.c |  4 +++-
>   net/smc/smc_core.h |  5 +++++
>   net/smc/smc_llc.c  |  6 +++++-
>   6 files changed, 58 insertions(+), 6 deletions(-)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index fd58e25beddf..b95d3fd48c28 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1214,6 +1214,7 @@ static int smc_connect_rdma(struct smc_sock *smc,
>   	memcpy(ini->peer_systemid, aclc->r0.lcl.id_for_peer, SMC_SYSTEMID_LEN);
>   	memcpy(ini->peer_gid, aclc->r0.lcl.gid, SMC_GID_SIZE);
>   	memcpy(ini->peer_mac, aclc->r0.lcl.mac, ETH_ALEN);
> +	ini->max_conns = SMC_RMBS_PER_LGR_MAX;
>   
>   	reason_code = smc_connect_rdma_v2_prepare(smc, aclc, ini);
>   	if (reason_code)
> diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
> index 4f6b69af2b80..e2b224063dcc 100644
> --- a/net/smc/smc_clc.c
> +++ b/net/smc/smc_clc.c
> @@ -427,9 +427,17 @@ static int smc_clc_fill_fce(struct smc_clc_first_contact_ext_v2x *fce,
>   	fce->fce_v20.os_type = SMC_CLC_OS_LINUX;
>   	fce->fce_v20.release = ini->release_ver;
>   	memcpy(fce->fce_v20.hostname, smc_hostname, sizeof(smc_hostname));
> -	if (ini->is_smcd && ini->release_ver < SMC_RELEASE_1)
> +	if (ini->is_smcd && ini->release_ver < SMC_RELEASE_1) {
>   		ret = sizeof(struct smc_clc_first_contact_ext);
> +		goto out;
> +	}
> +
> +	if (ini->release_ver >= SMC_RELEASE_1) {
> +		if (!ini->is_smcd)
> +			fce->max_conns = ini->max_conns;
> +	}
>   
> +out:
>   	return ret;
>   }
>   
> @@ -931,8 +939,10 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
>   				sizeof(struct smc_clc_smcd_gid_chid);
>   		}
>   	}
> -	if (smcr_indicated(ini->smc_type_v2))
> +	if (smcr_indicated(ini->smc_type_v2)) {
>   		memcpy(v2_ext->roce, ini->smcrv2.ib_gid_v2, SMC_GID_SIZE);
> +		v2_ext->max_conns = SMC_CONN_PER_LGR_MAX;
> +	}
>   
>   	pclc_base->hdr.length = htons(plen);
>   	memcpy(trl->eyecatcher, SMC_EYECATCHER, sizeof(SMC_EYECATCHER));
> @@ -1163,6 +1173,11 @@ int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc,
>   {
>   	struct smc_clc_v2_extension *pclc_v2_ext;
>   
> +	/* default max conn is SMC_RMBS_PER_LGR_MAX(255),
> +	 * which is the default value in smc v1 and v2.0.
> +	 */
> +	ini->max_conns = SMC_RMBS_PER_LGR_MAX;
> +
>   	if ((!(ini->smcd_version & SMC_V2) && !(ini->smcr_version & SMC_V2)) ||
>   	    ini->release_ver < SMC_RELEASE_1)
>   		return 0;
> @@ -1171,15 +1186,30 @@ int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc,
>   	if (!pclc_v2_ext)
>   		return SMC_CLC_DECL_NOV2EXT;
>   
> +	if (ini->smcr_version & SMC_V2) {
> +		ini->max_conns = min_t(u8, pclc_v2_ext->max_conns, SMC_CONN_PER_LGR_MAX);
> +		if (ini->max_conns < SMC_CONN_PER_LGR_MIN)
> +			return SMC_CLC_DECL_MAXCONNERR;
> +	}
> +
>   	return 0;
>   }
>   
>   int smc_clc_cli_v2x_features_validate(struct smc_clc_first_contact_ext *fce,
>   				      struct smc_init_info *ini)
>   {
> +	struct smc_clc_first_contact_ext_v2x *fce_v2x =
> +		(struct smc_clc_first_contact_ext_v2x *)fce;
> +
>   	if (ini->release_ver < SMC_RELEASE_1)
>   		return 0;
>   
> +	if (!ini->is_smcd) {
> +		if (fce_v2x->max_conns < SMC_CONN_PER_LGR_MIN)
> +			return SMC_CLC_DECL_MAXCONNERR;
> +		ini->max_conns = fce_v2x->max_conns;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -1190,6 +1220,8 @@ int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc,
>   		(struct smc_clc_msg_accept_confirm_v2 *)cclc;
>   	struct smc_clc_first_contact_ext *fce =
>   		smc_get_clc_first_contact_ext(clc_v2, ini->is_smcd);
> +	struct smc_clc_first_contact_ext_v2x *fce_v2x =
> +		(struct smc_clc_first_contact_ext_v2x *)fce;
>   
>   	if (cclc->hdr.version == SMC_V1 ||
>   	    !(cclc->hdr.typev2 & SMC_FIRST_CONTACT_MASK))
> @@ -1201,6 +1233,11 @@ int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc,
>   	if (fce->release < SMC_RELEASE_1)
>   		return 0;
>   
> +	if (!ini->is_smcd) {
> +		if (fce_v2x->max_conns != ini->max_conns)
> +			return SMC_CLC_DECL_MAXCONNERR;
> +	}
> +
>   	return 0;
>   }
>   
ok, now I have the answer from the last patch.

> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
> index 66932bfdc6d0..54077e50c368 100644
> --- a/net/smc/smc_clc.h
> +++ b/net/smc/smc_clc.h
> @@ -46,6 +46,7 @@
>   #define SMC_CLC_DECL_NOSMCD2DEV	0x03030007  /* no SMC-Dv2 device found	      */
>   #define SMC_CLC_DECL_NOUEID	0x03030008  /* peer sent no UEID	      */
>   #define SMC_CLC_DECL_RELEASEERR	0x03030009  /* release version negotiate failed */
> +#define SMC_CLC_DECL_MAXCONNERR	0x0303000a  /* max connections negotiate failed */
>   #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 */
> @@ -134,7 +135,8 @@ struct smc_clc_smcd_gid_chid {
>   struct smc_clc_v2_extension {
>   	struct smc_clnt_opts_area_hdr hdr;
>   	u8 roce[16];		/* RoCEv2 GID */
> -	u8 reserved[16];
> +	u8 max_conns;
> +	u8 reserved[15];
>   	u8 user_eids[][SMC_MAX_EID_LEN];
>   };
>   
> @@ -236,7 +238,8 @@ struct smc_clc_first_contact_ext {
>   
>   struct smc_clc_first_contact_ext_v2x {
>   	struct smc_clc_first_contact_ext fce_v20;
> -	u8 reserved3[4];
> +	u8 max_conns; /* for SMC-R only */
> +	u8 reserved3[3];
>   	__be32 vendor_exp_options;
>   	u8 reserved4[8];
>   } __packed;		/* format defined in
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 6aa3db47a956..5de1fbaa6e28 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -895,9 +895,11 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini)
>   			lgr->uses_gateway = ini->smcrv2.uses_gateway;
>   			memcpy(lgr->nexthop_mac, ini->smcrv2.nexthop_mac,
>   			       ETH_ALEN);
> +			lgr->max_conns = ini->max_conns;
>   		} else {
>   			ibdev = ini->ib_dev;
>   			ibport = ini->ib_port;
> +			lgr->max_conns = SMC_RMBS_PER_LGR_MAX;


It is kind of confused sometimes SMC_RMBS_PER_LGR_MAX is used and 
sometimes SMC_CONN_PER_LGR_MAX. IMO, you can use SMC_CONN_PER_LGR_MAX in 
the patches series for the new feature, because they are the same value 
and the name is more suiable.

>   		}
>   		memcpy(lgr->pnet_id, ibdev->pnetid[ibport - 1],
>   		       SMC_MAX_PNETID_LEN);
> @@ -1890,7 +1892,7 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
>   		    (ini->smcd_version == SMC_V2 ||
>   		     lgr->vlan_id == ini->vlan_id) &&
>   		    (role == SMC_CLNT || ini->is_smcd ||
> -		    (lgr->conns_num < SMC_RMBS_PER_LGR_MAX &&
> +		    (lgr->conns_num < lgr->max_conns &&
>   		      !bitmap_full(lgr->rtokens_used_mask, SMC_RMBS_PER_LGR_MAX)))) {
>   			/* link group found */
>   			ini->first_contact_local = 0;
> diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
> index 1a97fef39127..f4f7299c810a 100644
> --- a/net/smc/smc_core.h
> +++ b/net/smc/smc_core.h
> @@ -22,6 +22,8 @@
>   #include "smc_ib.h"
>   
>   #define SMC_RMBS_PER_LGR_MAX	255	/* max. # of RMBs per link group */
> +#define SMC_CONN_PER_LGR_MAX	255	/* max. # of connections per link group */
> +#define SMC_CONN_PER_LGR_MIN	16	/* min. # of connections per link group */
>   
>   struct smc_lgr_list {			/* list of link group definition */
>   	struct list_head	list;
> @@ -331,6 +333,8 @@ struct smc_link_group {
>   			__be32			saddr;
>   						/* net namespace */
>   			struct net		*net;
> +			u8			max_conns;
> +						/* max conn can be assigned to lgr */
>   		};
>   		struct { /* SMC-D */
>   			u64			peer_gid;
> @@ -375,6 +379,7 @@ struct smc_init_info {
>   	u8			smc_type_v1;
>   	u8			smc_type_v2;
>   	u8			release_ver;
> +	u8			max_conns;
>   	u8			first_contact_peer;
>   	u8			first_contact_local;
>   	unsigned short		vlan_id;
> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index 90f0b60b196a..5347b62f1518 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
> @@ -52,7 +52,8 @@ struct smc_llc_msg_confirm_link {	/* type 0x01 */
>   	u8 link_num;
>   	u8 link_uid[SMC_LGR_ID_SIZE];
>   	u8 max_links;
> -	u8 reserved[9];
> +	u8 max_conns;
> +	u8 reserved[8];
>   };
>   
>   #define SMC_LLC_FLAG_ADD_LNK_REJ	0x40
> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link,
>   	confllc->link_num = link->link_id;
>   	memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE);
>   	confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS;
> +	if (link->lgr->smc_version == SMC_V2 &&
> +	    link->lgr->peer_smc_release >= SMC_RELEASE_1)
> +		confllc->max_conns = link->lgr->max_conns;
>   	/* send llc message */
>   	rc = smc_wr_tx_send(link, pend);
>   put_out:

Did I miss the negotiation process somewhere for the following scenario?
(Example 4 in the document)
Client 				Server
	Proposal(max conns(16))
	----------------------->

	Accept(max conns(32))
	<-----------------------

	Confirm(max conns(32))
	----------------------->

Re: [RFC PATCH v2 net-next 4/6] net/smc: support max connections per lgr negotiation

[Guangguan Wang]

On 2023/8/10 00:04, Wenjia Zhang wrote:
> 
> 
> On 07.08.23 08:27, Guangguan Wang wrote:
>> Support max connections per lgr negotiation for SMCR v2.1,
>> which is one of smc v2.1 features.
...
>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>> index 6aa3db47a956..5de1fbaa6e28 100644
>> --- a/net/smc/smc_core.c
>> +++ b/net/smc/smc_core.c
>> @@ -895,9 +895,11 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini)
>>               lgr->uses_gateway = ini->smcrv2.uses_gateway;
>>               memcpy(lgr->nexthop_mac, ini->smcrv2.nexthop_mac,
>>                      ETH_ALEN);
>> +            lgr->max_conns = ini->max_conns;
>>           } else {
>>               ibdev = ini->ib_dev;
>>               ibport = ini->ib_port;
>> +            lgr->max_conns = SMC_RMBS_PER_LGR_MAX;
> 
> 
> It is kind of confused sometimes SMC_RMBS_PER_LGR_MAX is used and sometimes SMC_CONN_PER_LGR_MAX. IMO, you can use SMC_CONN_PER_LGR_MAX in the patches series for the new feature, because they are the same value and the name is more suiable.

OK, I will re-define the macros like this:
#define SMC_CONN_PER_LGR_MAX 255
#define SMC_CONN_PER_LGR_MIN 16
#define SMC_CONN_PER_LGR_PREFER 255 //vendors or distrubutions can modify this to a value between 16-255 as needed. 

...
>> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link,
>>       confllc->link_num = link->link_id;
>>       memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE);
>>       confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS;
>> +    if (link->lgr->smc_version == SMC_V2 &&
>> +        link->lgr->peer_smc_release >= SMC_RELEASE_1)
>> +        confllc->max_conns = link->lgr->max_conns;
>>       /* send llc message */
>>       rc = smc_wr_tx_send(link, pend);
>>   put_out:
> 
> Did I miss the negotiation process somewhere for the following scenario?
> (Example 4 in the document)
> Client                 Server
>     Proposal(max conns(16))
>     ----------------------->
> 
>     Accept(max conns(32))
>     <-----------------------
> 
>     Confirm(max conns(32))
>     ----------------------->

Did you mean the accepted max conns is different(not 32) from the Example 4 when the proposal max conns is 16?

As described in (https://www.ibm.com/support/pages/node/7009315) page 41:
...
2. Max conns and max links values sent in the CLC Proposal are the client preferred values.
3. The v2.1 values sent in the Accept message are the final values. The client must accept the values or
DECLINE the connection.
4. Max conns and links values sent in the CLC Accept are the final values (server dictates). The server can
either honor the client’s preferred values or return different (negotiated but final) values. 
...

If I understand correctly, the server dictates the final value of max conns, but how the server dictates the final
value of max conns is not defined in SMC v2.1. In this patch, the server use the minimum value of client preferred
value and server preferred value as the final value of max conns. The max links is negotiated with the same logic.

Client                 Server
     Proposal(max conns(client preferred))
     ----------------------->
 
     Accept(max conns(accepted value)) accepted value=min(client preferred, server preferred)
     <-----------------------
 
     Confirm(max conns(accepted value))
     ----------------------->

I also will add this description into commit message for better understanding.

Thanks,
Guangguan Wang

Re: [RFC PATCH v2 net-next 4/6] net/smc: support max connections per lgr negotiation

[Wenjia Zhang]

On 15.08.23 08:31, Guangguan Wang wrote:
> 
> 
> On 2023/8/10 00:04, Wenjia Zhang wrote:
>>
>>
>> On 07.08.23 08:27, Guangguan Wang wrote:
>>> Support max connections per lgr negotiation for SMCR v2.1,
>>> which is one of smc v2.1 features.
> ...
>>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>>> index 6aa3db47a956..5de1fbaa6e28 100644
>>> --- a/net/smc/smc_core.c
>>> +++ b/net/smc/smc_core.c
>>> @@ -895,9 +895,11 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini)
>>>                lgr->uses_gateway = ini->smcrv2.uses_gateway;
>>>                memcpy(lgr->nexthop_mac, ini->smcrv2.nexthop_mac,
>>>                       ETH_ALEN);
>>> +            lgr->max_conns = ini->max_conns;
>>>            } else {
>>>                ibdev = ini->ib_dev;
>>>                ibport = ini->ib_port;
>>> +            lgr->max_conns = SMC_RMBS_PER_LGR_MAX;
>>
>>
>> It is kind of confused sometimes SMC_RMBS_PER_LGR_MAX is used and sometimes SMC_CONN_PER_LGR_MAX. IMO, you can use SMC_CONN_PER_LGR_MAX in the patches series for the new feature, because they are the same value and the name is more suiable.
> 
> OK, I will re-define the macros like this:
> #define SMC_CONN_PER_LGR_MAX 255
> #define SMC_CONN_PER_LGR_MIN 16
> #define SMC_CONN_PER_LGR_PREFER 255 //vendors or distrubutions can modify this to a value between 16-255 as needed.
> 
> ...
>>> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link,
>>>        confllc->link_num = link->link_id;
>>>        memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE);
>>>        confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS;
>>> +    if (link->lgr->smc_version == SMC_V2 &&
>>> +        link->lgr->peer_smc_release >= SMC_RELEASE_1)
>>> +        confllc->max_conns = link->lgr->max_conns;
>>>        /* send llc message */
>>>        rc = smc_wr_tx_send(link, pend);
>>>    put_out:
>>
>> Did I miss the negotiation process somewhere for the following scenario?
>> (Example 4 in the document)
>> Client                 Server
>>      Proposal(max conns(16))
>>      ----------------------->
>>
>>      Accept(max conns(32))
>>      <-----------------------
>>
>>      Confirm(max conns(32))
>>      ----------------------->
> 
> Did you mean the accepted max conns is different(not 32) from the Example 4 when the proposal max conns is 16?
> 
> As described in (https://www.ibm.com/support/pages/node/7009315) page 41:
> ...
> 2. Max conns and max links values sent in the CLC Proposal are the client preferred values.
> 3. The v2.1 values sent in the Accept message are the final values. The client must accept the values or
> DECLINE the connection.
> 4. Max conns and links values sent in the CLC Accept are the final values (server dictates). The server can
> either honor the client’s preferred values or return different (negotiated but final) values.
> ...
> 
> If I understand correctly, the server dictates the final value of max conns, but how the server dictates the final
> value of max conns is not defined in SMC v2.1. In this patch, the server use the minimum value of client preferred
> value and server preferred value as the final value of max conns. The max links is negotiated with the same logic.
> 
> Client                 Server
>       Proposal(max conns(client preferred))
>       ----------------------->
>   
>       Accept(max conns(accepted value)) accepted value=min(client preferred, server preferred)
>       <-----------------------
>   
>       Confirm(max conns(accepted value))
>       ----------------------->
> 
> I also will add this description into commit message for better understanding.
> 
> Thanks,
> Guangguan Wang
> 
> 
> 

Sorry for the late answer, I'm just back from vacation.

That's true that the protocol does not define how the server decides the 
final value(s). I'm wondering if there is some reason for you to use the 
minimum value instead of maximum (corresponding to the examples in the 
document). If the both prefered values (client's and server's) are in 
the range of the acceptable value, why not the maximum? Is there any 
consideration on that?

Best,
Wenjia

Re: [RFC PATCH v2 net-next 4/6] net/smc: support max connections per lgr negotiation

[Guangguan Wang]

On 2023/8/28 20:54, Wenjia Zhang wrote:
> 
> 
> On 15.08.23 08:31, Guangguan Wang wrote:
>>
>>
>> On 2023/8/10 00:04, Wenjia Zhang wrote:
>>>
>>>
>>> On 07.08.23 08:27, Guangguan Wang wrote:
>>>> Support max connections per lgr negotiation for SMCR v2.1,
>>>> which is one of smc v2.1 features.
>> ...
>>>> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link,
>>>>        confllc->link_num = link->link_id;
>>>>        memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE);
>>>>        confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS;
>>>> +    if (link->lgr->smc_version == SMC_V2 &&
>>>> +        link->lgr->peer_smc_release >= SMC_RELEASE_1)
>>>> +        confllc->max_conns = link->lgr->max_conns;
>>>>        /* send llc message */
>>>>        rc = smc_wr_tx_send(link, pend);
>>>>    put_out:
>>>
>>> Did I miss the negotiation process somewhere for the following scenario?
>>> (Example 4 in the document)
>>> Client                 Server
>>>      Proposal(max conns(16))
>>>      ----------------------->
>>>
>>>      Accept(max conns(32))
>>>      <-----------------------
>>>
>>>      Confirm(max conns(32))
>>>      ----------------------->
>>
>> Did you mean the accepted max conns is different(not 32) from the Example 4 when the proposal max conns is 16?
>>
>> As described in (https://www.ibm.com/support/pages/node/7009315) page 41:
>> ...
>> 2. Max conns and max links values sent in the CLC Proposal are the client preferred values.
>> 3. The v2.1 values sent in the Accept message are the final values. The client must accept the values or
>> DECLINE the connection.
>> 4. Max conns and links values sent in the CLC Accept are the final values (server dictates). The server can
>> either honor the client’s preferred values or return different (negotiated but final) values.
>> ...
>>
>> If I understand correctly, the server dictates the final value of max conns, but how the server dictates the final
>> value of max conns is not defined in SMC v2.1. In this patch, the server use the minimum value of client preferred
>> value and server preferred value as the final value of max conns. The max links is negotiated with the same logic.
>>
>> Client                 Server
>>       Proposal(max conns(client preferred))
>>       ----------------------->
>>         Accept(max conns(accepted value)) accepted value=min(client preferred, server preferred)
>>       <-----------------------
>>         Confirm(max conns(accepted value))
>>       ----------------------->
>>
>> I also will add this description into commit message for better understanding.
>>
>> Thanks,
>> Guangguan Wang
>>
>>
>>
> 
> Sorry for the late answer, I'm just back from vacation.
> 
> That's true that the protocol does not define how the server decides the final value(s). I'm wondering if there is some reason for you to use the minimum value instead of maximum (corresponding to the examples in the document). If the both prefered values (client's and server's) are in the range of the acceptable value, why not the maximum? Is there any consideration on that?
> 
> Best,
> Wenjia

Since the value of the default preferred max conns is already the maximum value of the range(16-255), I am wondering
whether it makes any sense to use the maximum for decision, where the negotiated result of max conns is always 255.
So does the max links. 

Thanks,
Guangguan

Re: [RFC PATCH v2 net-next 4/6] net/smc: support max connections per lgr negotiation

[Wenjia Zhang]

On 29.08.23 04:31, Guangguan Wang wrote:
> 
> 
> On 2023/8/28 20:54, Wenjia Zhang wrote:
>>
>>
>> On 15.08.23 08:31, Guangguan Wang wrote:
>>>
>>>
>>> On 2023/8/10 00:04, Wenjia Zhang wrote:
>>>>
>>>>
>>>> On 07.08.23 08:27, Guangguan Wang wrote:
>>>>> Support max connections per lgr negotiation for SMCR v2.1,
>>>>> which is one of smc v2.1 features.
>>> ...
>>>>> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link,
>>>>>         confllc->link_num = link->link_id;
>>>>>         memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE);
>>>>>         confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS;
>>>>> +    if (link->lgr->smc_version == SMC_V2 &&
>>>>> +        link->lgr->peer_smc_release >= SMC_RELEASE_1)
>>>>> +        confllc->max_conns = link->lgr->max_conns;
>>>>>         /* send llc message */
>>>>>         rc = smc_wr_tx_send(link, pend);
>>>>>     put_out:
>>>>
>>>> Did I miss the negotiation process somewhere for the following scenario?
>>>> (Example 4 in the document)
>>>> Client                 Server
>>>>       Proposal(max conns(16))
>>>>       ----------------------->
>>>>
>>>>       Accept(max conns(32))
>>>>       <-----------------------
>>>>
>>>>       Confirm(max conns(32))
>>>>       ----------------------->
>>>
>>> Did you mean the accepted max conns is different(not 32) from the Example 4 when the proposal max conns is 16?
>>>
>>> As described in (https://www.ibm.com/support/pages/node/7009315) page 41:
>>> ...
>>> 2. Max conns and max links values sent in the CLC Proposal are the client preferred values.
>>> 3. The v2.1 values sent in the Accept message are the final values. The client must accept the values or
>>> DECLINE the connection.
>>> 4. Max conns and links values sent in the CLC Accept are the final values (server dictates). The server can
>>> either honor the client’s preferred values or return different (negotiated but final) values.
>>> ...
>>>
>>> If I understand correctly, the server dictates the final value of max conns, but how the server dictates the final
>>> value of max conns is not defined in SMC v2.1. In this patch, the server use the minimum value of client preferred
>>> value and server preferred value as the final value of max conns. The max links is negotiated with the same logic.
>>>
>>> Client                 Server
>>>        Proposal(max conns(client preferred))
>>>        ----------------------->
>>>          Accept(max conns(accepted value)) accepted value=min(client preferred, server preferred)
>>>        <-----------------------
>>>          Confirm(max conns(accepted value))
>>>        ----------------------->
>>>
>>> I also will add this description into commit message for better understanding.
>>>
>>> Thanks,
>>> Guangguan Wang
>>>
>>>
>>>
>>
>> Sorry for the late answer, I'm just back from vacation.
>>
>> That's true that the protocol does not define how the server decides the final value(s). I'm wondering if there is some reason for you to use the minimum value instead of maximum (corresponding to the examples in the document). If the both prefered values (client's and server's) are in the range of the acceptable value, why not the maximum? Is there any consideration on that?
>>
>> Best,
>> Wenjia
> 
> Since the value of the default preferred max conns is already the maximum value of the range(16-255), I am wondering
> whether it makes any sense to use the maximum for decision, where the negotiated result of max conns is always 255.
> So does the max links.
> 
> Thanks,
> Guangguan

I don't think the server's default maxconns must be the maximum value, 
i.e 255. Since the patches series are already applied, we say the 
previous implementation uses maximus value because the maxconns is not 
tunable, so that we choose an appropriate value as the default value.
Now the value is negotiable, the default value could be also the 
server's prefer value.
But regarding maxlinks, I'm fine with the minimus, and actually it 
should be, because it should not be possible to try to add another link 
if one of the peers can and want to support only one link, i.e. down-level.
Any opinion?

Best,
Wenjia

Re: [RFC PATCH v2 net-next 4/6] net/smc: support max connections per lgr negotiation

[Guangguan Wang]

On 2023/8/29 21:18, Wenjia Zhang wrote:
> 
> 
> On 29.08.23 04:31, Guangguan Wang wrote:
>>
>>
>> On 2023/8/28 20:54, Wenjia Zhang wrote:
>>>
>>>
>>> On 15.08.23 08:31, Guangguan Wang wrote:
>>>>
>>>>
>>>> On 2023/8/10 00:04, Wenjia Zhang wrote:
>>>>>
>>>>>
>>>>> On 07.08.23 08:27, Guangguan Wang wrote:
>>>>>> Support max connections per lgr negotiation for SMCR v2.1,
>>>>>> which is one of smc v2.1 features.
>>>> ...
>>>>>> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link,
>>>>>>         confllc->link_num = link->link_id;
>>>>>>         memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE);
>>>>>>         confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS;
>>>>>> +    if (link->lgr->smc_version == SMC_V2 &&
>>>>>> +        link->lgr->peer_smc_release >= SMC_RELEASE_1)
>>>>>> +        confllc->max_conns = link->lgr->max_conns;
>>>>>>         /* send llc message */
>>>>>>         rc = smc_wr_tx_send(link, pend);
>>>>>>     put_out:
>>>>>
>>>>> Did I miss the negotiation process somewhere for the following scenario?
>>>>> (Example 4 in the document)
>>>>> Client                 Server
>>>>>       Proposal(max conns(16))
>>>>>       ----------------------->
>>>>>
>>>>>       Accept(max conns(32))
>>>>>       <-----------------------
>>>>>
>>>>>       Confirm(max conns(32))
>>>>>       ----------------------->
>>>>
>>>> Did you mean the accepted max conns is different(not 32) from the Example 4 when the proposal max conns is 16?
>>>>
>>>> As described in (https://www.ibm.com/support/pages/node/7009315) page 41:
>>>> ...
>>>> 2. Max conns and max links values sent in the CLC Proposal are the client preferred values.
>>>> 3. The v2.1 values sent in the Accept message are the final values. The client must accept the values or
>>>> DECLINE the connection.
>>>> 4. Max conns and links values sent in the CLC Accept are the final values (server dictates). The server can
>>>> either honor the client’s preferred values or return different (negotiated but final) values.
>>>> ...
>>>>
>>>> If I understand correctly, the server dictates the final value of max conns, but how the server dictates the final
>>>> value of max conns is not defined in SMC v2.1. In this patch, the server use the minimum value of client preferred
>>>> value and server preferred value as the final value of max conns. The max links is negotiated with the same logic.
>>>>
>>>> Client                 Server
>>>>        Proposal(max conns(client preferred))
>>>>        ----------------------->
>>>>          Accept(max conns(accepted value)) accepted value=min(client preferred, server preferred)
>>>>        <-----------------------
>>>>          Confirm(max conns(accepted value))
>>>>        ----------------------->
>>>>
>>>> I also will add this description into commit message for better understanding.
>>>>
>>>> Thanks,
>>>> Guangguan Wang
>>>>
>>>>
>>>>
>>>
>>> Sorry for the late answer, I'm just back from vacation.
>>>
>>> That's true that the protocol does not define how the server decides the final value(s). I'm wondering if there is some reason for you to use the minimum value instead of maximum (corresponding to the examples in the document). If the both prefered values (client's and server's) are in the range of the acceptable value, why not the maximum? Is there any consideration on that?
>>>
>>> Best,
>>> Wenjia
>>
>> Since the value of the default preferred max conns is already the maximum value of the range(16-255), I am wondering
>> whether it makes any sense to use the maximum for decision, where the negotiated result of max conns is always 255.
>> So does the max links.
>>
>> Thanks,
>> Guangguan
> 
> I don't think the server's default maxconns must be the maximum value, i.e 255. Since the patches series are already applied, we say the previous implementation uses maximus value because the maxconns is not tunable, so that we choose an appropriate value as the default value.
> Now the value is negotiable, the default value could be also the server's prefer value.
If the server's default maxconns could be other value rather than maximum value, it's OK to use other decision algorithm(minimum, maximum or others).
But it is still a question that how to tune the default maxconns, maybe it is different from different linux distributions and different vendors of rdma nic.

> But regarding maxlinks, I'm fine with the minimus, and actually it should be, because it should not be possible to try to add another link if one of the peers can and want to support only one link, i.e. down-level.
Agree with you.

> Any opinion?
> 
> Best,
> Wenjia

Thanks,
Guangguan Wang

Re: [RFC PATCH v2 net-next 4/6] net/smc: support max connections per lgr negotiation

[Wenjia Zhang]

On 30.08.23 05:17, Guangguan Wang wrote:
> 
> 
> On 2023/8/29 21:18, Wenjia Zhang wrote:
>>
>>
>> On 29.08.23 04:31, Guangguan Wang wrote:
>>>
>>>
>>> On 2023/8/28 20:54, Wenjia Zhang wrote:
>>>>
>>>>
>>>> On 15.08.23 08:31, Guangguan Wang wrote:
>>>>>
>>>>>
>>>>> On 2023/8/10 00:04, Wenjia Zhang wrote:
>>>>>>
>>>>>>
>>>>>> On 07.08.23 08:27, Guangguan Wang wrote:
>>>>>>> Support max connections per lgr negotiation for SMCR v2.1,
>>>>>>> which is one of smc v2.1 features.
>>>>> ...
>>>>>>> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link,
>>>>>>>          confllc->link_num = link->link_id;
>>>>>>>          memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE);
>>>>>>>          confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS;
>>>>>>> +    if (link->lgr->smc_version == SMC_V2 &&
>>>>>>> +        link->lgr->peer_smc_release >= SMC_RELEASE_1)
>>>>>>> +        confllc->max_conns = link->lgr->max_conns;
>>>>>>>          /* send llc message */
>>>>>>>          rc = smc_wr_tx_send(link, pend);
>>>>>>>      put_out:
>>>>>>
>>>>>> Did I miss the negotiation process somewhere for the following scenario?
>>>>>> (Example 4 in the document)
>>>>>> Client                 Server
>>>>>>        Proposal(max conns(16))
>>>>>>        ----------------------->
>>>>>>
>>>>>>        Accept(max conns(32))
>>>>>>        <-----------------------
>>>>>>
>>>>>>        Confirm(max conns(32))
>>>>>>        ----------------------->
>>>>>
>>>>> Did you mean the accepted max conns is different(not 32) from the Example 4 when the proposal max conns is 16?
>>>>>
>>>>> As described in (https://www.ibm.com/support/pages/node/7009315) page 41:
>>>>> ...
>>>>> 2. Max conns and max links values sent in the CLC Proposal are the client preferred values.
>>>>> 3. The v2.1 values sent in the Accept message are the final values. The client must accept the values or
>>>>> DECLINE the connection.
>>>>> 4. Max conns and links values sent in the CLC Accept are the final values (server dictates). The server can
>>>>> either honor the client’s preferred values or return different (negotiated but final) values.
>>>>> ...
>>>>>
>>>>> If I understand correctly, the server dictates the final value of max conns, but how the server dictates the final
>>>>> value of max conns is not defined in SMC v2.1. In this patch, the server use the minimum value of client preferred
>>>>> value and server preferred value as the final value of max conns. The max links is negotiated with the same logic.
>>>>>
>>>>> Client                 Server
>>>>>         Proposal(max conns(client preferred))
>>>>>         ----------------------->
>>>>>           Accept(max conns(accepted value)) accepted value=min(client preferred, server preferred)
>>>>>         <-----------------------
>>>>>           Confirm(max conns(accepted value))
>>>>>         ----------------------->
>>>>>
>>>>> I also will add this description into commit message for better understanding.
>>>>>
>>>>> Thanks,
>>>>> Guangguan Wang
>>>>>
>>>>>
>>>>>
>>>>
>>>> Sorry for the late answer, I'm just back from vacation.
>>>>
>>>> That's true that the protocol does not define how the server decides the final value(s). I'm wondering if there is some reason for you to use the minimum value instead of maximum (corresponding to the examples in the document). If the both prefered values (client's and server's) are in the range of the acceptable value, why not the maximum? Is there any consideration on that?
>>>>
>>>> Best,
>>>> Wenjia
>>>
>>> Since the value of the default preferred max conns is already the maximum value of the range(16-255), I am wondering
>>> whether it makes any sense to use the maximum for decision, where the negotiated result of max conns is always 255.
>>> So does the max links.
>>>
>>> Thanks,
>>> Guangguan
>>
>> I don't think the server's default maxconns must be the maximum value, i.e 255. Since the patches series are already applied, we say the previous implementation uses maximus value because the maxconns is not tunable, so that we choose an appropriate value as the default value.
>> Now the value is negotiable, the default value could be also the server's prefer value.
> If the server's default maxconns could be other value rather than maximum value, it's OK to use other decision algorithm(minimum, maximum or others).
> But it is still a question that how to tune the default maxconns, maybe it is different from different linux distributions and different vendors of rdma nic.
> 
That's true. I think more discussion is needed. Let's talk about it 
offline first, since these patches are already applied.

BTW, thank you for the efforts!

Best,
Wenjia

>> But regarding maxlinks, I'm fine with the minimus, and actually it should be, because it should not be possible to try to add another link if one of the peers can and want to support only one link, i.e. down-level.
> Agree with you.
> 
>> Any opinion?
>>
>> Best,
>> Wenjia
> 
> Thanks,
> Guangguan Wang

[RFC PATCH v2 net-next 5/6] net/smc: support max links per lgr negotiation in clc handshake

[Guangguan Wang]

Support max links per lgr negotiation in clc handshake for SMCR v2.1,
which is one of smc v2.1 features.

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
---
 net/smc/af_smc.c   | 44 +++++++++++++++++++++++++++-----------------
 net/smc/smc_clc.c  | 16 +++++++++++++++-
 net/smc/smc_clc.h  |  7 +++++--
 net/smc/smc_core.c |  5 +++++
 net/smc/smc_core.h |  5 +++++
 net/smc/smc_llc.c  | 17 +++++++++++++----
 6 files changed, 70 insertions(+), 24 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index b95d3fd48c28..23384d08d3f2 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -610,20 +610,22 @@ static int smcr_clnt_conf_first_link(struct smc_sock *smc)
 	smc_llc_link_active(link);
 	smcr_lgr_set_type(link->lgr, SMC_LGR_SINGLE);
 
-	/* optional 2nd link, receive ADD LINK request from server */
-	qentry = smc_llc_wait(link->lgr, NULL, SMC_LLC_WAIT_TIME,
-			      SMC_LLC_ADD_LINK);
-	if (!qentry) {
-		struct smc_clc_msg_decline dclc;
-
-		rc = smc_clc_wait_msg(smc, &dclc, sizeof(dclc),
-				      SMC_CLC_DECLINE, CLC_WAIT_TIME_SHORT);
-		if (rc == -EAGAIN)
-			rc = 0; /* no DECLINE received, go with one link */
-		return rc;
+	if (link->lgr->max_links > 1) {
+		/* optional 2nd link, receive ADD LINK request from server */
+		qentry = smc_llc_wait(link->lgr, NULL, SMC_LLC_WAIT_TIME,
+				      SMC_LLC_ADD_LINK);
+		if (!qentry) {
+			struct smc_clc_msg_decline dclc;
+
+			rc = smc_clc_wait_msg(smc, &dclc, sizeof(dclc),
+					      SMC_CLC_DECLINE, CLC_WAIT_TIME_SHORT);
+			if (rc == -EAGAIN)
+				rc = 0; /* no DECLINE received, go with one link */
+			return rc;
+		}
+		smc_llc_flow_qentry_clr(&link->lgr->llc_flow_lcl);
+		smc_llc_cli_add_link(link, qentry);
 	}
-	smc_llc_flow_qentry_clr(&link->lgr->llc_flow_lcl);
-	smc_llc_cli_add_link(link, qentry);
 	return 0;
 }
 
@@ -1215,6 +1217,7 @@ static int smc_connect_rdma(struct smc_sock *smc,
 	memcpy(ini->peer_gid, aclc->r0.lcl.gid, SMC_GID_SIZE);
 	memcpy(ini->peer_mac, aclc->r0.lcl.mac, ETH_ALEN);
 	ini->max_conns = SMC_RMBS_PER_LGR_MAX;
+	ini->max_links = SMC_LINKS_ADD_LNK_MAX;
 
 	reason_code = smc_connect_rdma_v2_prepare(smc, aclc, ini);
 	if (reason_code)
@@ -1861,10 +1864,12 @@ static int smcr_serv_conf_first_link(struct smc_sock *smc)
 	smc_llc_link_active(link);
 	smcr_lgr_set_type(link->lgr, SMC_LGR_SINGLE);
 
-	down_write(&link->lgr->llc_conf_mutex);
-	/* initial contact - try to establish second link */
-	smc_llc_srv_add_link(link, NULL);
-	up_write(&link->lgr->llc_conf_mutex);
+	if (link->lgr->max_links > 1) {
+		down_write(&link->lgr->llc_conf_mutex);
+		/* initial contact - try to establish second link */
+		smc_llc_srv_add_link(link, NULL);
+		up_write(&link->lgr->llc_conf_mutex);
+	}
 	return 0;
 }
 
@@ -2468,6 +2473,11 @@ static void smc_listen_work(struct work_struct *work)
 		goto out_decl;
 	}
 
+	/* fce smc release version is needed in smc_listen_rdma_finish,
+	 * so save fce info here.
+	 */
+	smc_conn_save_peer_info_fce(new_smc, cclc);
+
 	/* finish worker */
 	if (!ini->is_smcd) {
 		rc = smc_listen_rdma_finish(new_smc, cclc,
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index e2b224063dcc..84c47cb1e779 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -433,8 +433,10 @@ static int smc_clc_fill_fce(struct smc_clc_first_contact_ext_v2x *fce,
 	}
 
 	if (ini->release_ver >= SMC_RELEASE_1) {
-		if (!ini->is_smcd)
+		if (!ini->is_smcd) {
 			fce->max_conns = ini->max_conns;
+			fce->max_links = ini->max_links;
+		}
 	}
 
 out:
@@ -942,6 +944,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 	if (smcr_indicated(ini->smc_type_v2)) {
 		memcpy(v2_ext->roce, ini->smcrv2.ib_gid_v2, SMC_GID_SIZE);
 		v2_ext->max_conns = SMC_CONN_PER_LGR_MAX;
+		v2_ext->max_links = SMC_LINKS_PER_LGR_MAX_PREFER;
 	}
 
 	pclc_base->hdr.length = htons(plen);
@@ -1177,6 +1180,7 @@ int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc,
 	 * which is the default value in smc v1 and v2.0.
 	 */
 	ini->max_conns = SMC_RMBS_PER_LGR_MAX;
+	ini->max_links = SMC_LINKS_ADD_LNK_MAX;
 
 	if ((!(ini->smcd_version & SMC_V2) && !(ini->smcr_version & SMC_V2)) ||
 	    ini->release_ver < SMC_RELEASE_1)
@@ -1190,6 +1194,10 @@ int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc,
 		ini->max_conns = min_t(u8, pclc_v2_ext->max_conns, SMC_CONN_PER_LGR_MAX);
 		if (ini->max_conns < SMC_CONN_PER_LGR_MIN)
 			return SMC_CLC_DECL_MAXCONNERR;
+
+		ini->max_links = min_t(u8, pclc_v2_ext->max_links, SMC_LINKS_PER_LGR_MAX_PREFER);
+		if (!ini->max_links)
+			return SMC_CLC_DECL_MAXLINKERR;
 	}
 
 	return 0;
@@ -1208,6 +1216,10 @@ int smc_clc_cli_v2x_features_validate(struct smc_clc_first_contact_ext *fce,
 		if (fce_v2x->max_conns < SMC_CONN_PER_LGR_MIN)
 			return SMC_CLC_DECL_MAXCONNERR;
 		ini->max_conns = fce_v2x->max_conns;
+
+		if (fce_v2x->max_links > SMC_LINKS_ADD_LNK_MAX)
+			return SMC_CLC_DECL_MAXLINKERR;
+		ini->max_links = fce_v2x->max_links;
 	}
 
 	return 0;
@@ -1236,6 +1248,8 @@ int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc,
 	if (!ini->is_smcd) {
 		if (fce_v2x->max_conns != ini->max_conns)
 			return SMC_CLC_DECL_MAXCONNERR;
+		if (fce_v2x->max_links != ini->max_links)
+			return SMC_CLC_DECL_MAXLINKERR;
 	}
 
 	return 0;
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 54077e50c368..53fb20d7c14d 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -47,6 +47,7 @@
 #define SMC_CLC_DECL_NOUEID	0x03030008  /* peer sent no UEID	      */
 #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_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 */
@@ -136,7 +137,8 @@ struct smc_clc_v2_extension {
 	struct smc_clnt_opts_area_hdr hdr;
 	u8 roce[16];		/* RoCEv2 GID */
 	u8 max_conns;
-	u8 reserved[15];
+	u8 max_links;
+	u8 reserved[14];
 	u8 user_eids[][SMC_MAX_EID_LEN];
 };
 
@@ -239,7 +241,8 @@ struct smc_clc_first_contact_ext {
 struct smc_clc_first_contact_ext_v2x {
 	struct smc_clc_first_contact_ext fce_v20;
 	u8 max_conns; /* for SMC-R only */
-	u8 reserved3[3];
+	u8 max_links; /* for SMC-R only */
+	u8 reserved3[2];
 	__be32 vendor_exp_options;
 	u8 reserved4[8];
 } __packed;		/* format defined in
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 5de1fbaa6e28..d5967826bcdf 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -896,10 +896,12 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini)
 			memcpy(lgr->nexthop_mac, ini->smcrv2.nexthop_mac,
 			       ETH_ALEN);
 			lgr->max_conns = ini->max_conns;
+			lgr->max_links = ini->max_links;
 		} else {
 			ibdev = ini->ib_dev;
 			ibport = ini->ib_port;
 			lgr->max_conns = SMC_RMBS_PER_LGR_MAX;
+			lgr->max_links = SMC_LINKS_ADD_LNK_MAX;
 		}
 		memcpy(lgr->pnet_id, ibdev->pnetid[ibport - 1],
 		       SMC_MAX_PNETID_LEN);
@@ -1667,6 +1669,9 @@ void smcr_port_add(struct smc_ib_device *smcibdev, u8 ibport)
 		    !rdma_dev_access_netns(smcibdev->ibdev, lgr->net))
 			continue;
 
+		if (lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
+			continue;
+
 		/* trigger local add link processing */
 		link = smc_llc_usable_link(lgr);
 		if (link)
diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
index f4f7299c810a..b327ef01c838 100644
--- a/net/smc/smc_core.h
+++ b/net/smc/smc_core.h
@@ -166,6 +166,8 @@ struct smc_link {
  */
 #define SMC_LINKS_PER_LGR_MAX	3
 #define SMC_SINGLE_LINK		0
+#define SMC_LINKS_PER_LGR_MAX_PREFER	2	/* prefer 2 links max per lgr */
+#define SMC_LINKS_ADD_LNK_MAX	2
 
 /* tx/rx buffer list element for sndbufs list and rmbs list of a lgr */
 struct smc_buf_desc {
@@ -335,6 +337,8 @@ struct smc_link_group {
 			struct net		*net;
 			u8			max_conns;
 						/* max conn can be assigned to lgr */
+			u8			max_links;
+						/* max links can be added in lgr */
 		};
 		struct { /* SMC-D */
 			u64			peer_gid;
@@ -380,6 +384,7 @@ struct smc_init_info {
 	u8			smc_type_v2;
 	u8			release_ver;
 	u8			max_conns;
+	u8			max_links;
 	u8			first_contact_peer;
 	u8			first_contact_local;
 	unsigned short		vlan_id;
diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
index 5347b62f1518..41e5e149edf3 100644
--- a/net/smc/smc_llc.c
+++ b/net/smc/smc_llc.c
@@ -59,8 +59,6 @@ struct smc_llc_msg_confirm_link {	/* type 0x01 */
 #define SMC_LLC_FLAG_ADD_LNK_REJ	0x40
 #define SMC_LLC_REJ_RSN_NO_ALT_PATH	1
 
-#define SMC_LLC_ADD_LNK_MAX_LINKS	2
-
 struct smc_llc_msg_add_link {		/* type 0x02 */
 	struct smc_llc_hdr hd;
 	u8 sender_mac[ETH_ALEN];
@@ -472,10 +470,12 @@ int smc_llc_send_confirm_link(struct smc_link *link,
 	hton24(confllc->sender_qp_num, link->roce_qp->qp_num);
 	confllc->link_num = link->link_id;
 	memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE);
-	confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS;
+	confllc->max_links = SMC_LINKS_ADD_LNK_MAX;
 	if (link->lgr->smc_version == SMC_V2 &&
-	    link->lgr->peer_smc_release >= SMC_RELEASE_1)
+	    link->lgr->peer_smc_release >= SMC_RELEASE_1) {
 		confllc->max_conns = link->lgr->max_conns;
+		confllc->max_links = link->lgr->max_links;
+	}
 	/* send llc message */
 	rc = smc_wr_tx_send(link, pend);
 put_out:
@@ -1045,6 +1045,9 @@ int smc_llc_cli_add_link(struct smc_link *link, struct smc_llc_qentry *qentry)
 		goto out_reject;
 	}
 
+	if (lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
+		goto out_reject;
+
 	ini->vlan_id = lgr->vlan_id;
 	if (lgr->smc_version == SMC_V2) {
 		ini->check_smcrv2 = true;
@@ -1169,6 +1172,9 @@ static void smc_llc_cli_add_link_invite(struct smc_link *link,
 	    lgr->type == SMC_LGR_ASYMMETRIC_PEER)
 		goto out;
 
+	if (lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
+		goto out;
+
 	ini = kzalloc(sizeof(*ini), GFP_KERNEL);
 	if (!ini)
 		goto out;
@@ -1414,6 +1420,9 @@ int smc_llc_srv_add_link(struct smc_link *link,
 		goto out;
 	}
 
+	if (lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)
+		goto out;
+
 	/* ignore client add link recommendation, start new flow */
 	ini->vlan_id = lgr->vlan_id;
 	if (lgr->smc_version == SMC_V2) {
-- 
2.24.3 (Apple Git-128)

Re: [RFC PATCH v2 net-next 5/6] net/smc: support max links per lgr negotiation in clc handshake

[Simon Horman]

On Mon, Aug 07, 2023 at 02:27:19PM +0800, Guangguan Wang wrote:

...

> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c

...

> @@ -1414,6 +1420,9 @@ int smc_llc_srv_add_link(struct smc_link *link,
>  		goto out;
>  	}
>  
> +	if (lgr->type == SMC_LGR_SINGLE && lgr->max_links <= 1)

Hi Guangguan Wang,

The function will return rc.
Should it be set to an error value here?

Flagged by Smatch.

> +		goto out;
> +
>  	/* ignore client add link recommendation, start new flow */
>  	ini->vlan_id = lgr->vlan_id;
>  	if (lgr->smc_version == SMC_V2) {
> -- 
> 2.24.3 (Apple Git-128)
> 

[RFC PATCH v2 net-next 6/6] net/smc: Extend SMCR v2 linkgroup netlink attribute

[Guangguan Wang]

Add SMC_NLA_LGR_R_V2_MAX_CONNS and SMC_NLA_LGR_R_V2_MAX_LINKS
to SMCR v2 linkgroup netlink attribute SMC_NLA_LGR_R_V2 for
linkgroup's detail info showing.

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
---
 include/uapi/linux/smc.h | 2 ++
 net/smc/smc_core.c       | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/include/uapi/linux/smc.h b/include/uapi/linux/smc.h
index bb4dacca31e7..837fcd4b0abc 100644
--- a/include/uapi/linux/smc.h
+++ b/include/uapi/linux/smc.h
@@ -107,6 +107,8 @@ enum {
 enum {
 	SMC_NLA_LGR_R_V2_UNSPEC,
 	SMC_NLA_LGR_R_V2_DIRECT,	/* u8 */
+	SMC_NLA_LGR_R_V2_MAX_CONNS,	/* u8 */
+	SMC_NLA_LGR_R_V2_MAX_LINKS,	/* u8 */
 	__SMC_NLA_LGR_R_V2_MAX,
 	SMC_NLA_LGR_R_V2_MAX = __SMC_NLA_LGR_R_V2_MAX - 1
 };
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index d5967826bcdf..182ac69d25c8 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -319,6 +319,10 @@ static int smc_nl_fill_smcr_lgr_v2(struct smc_link_group *lgr,
 		goto errattr;
 	if (nla_put_u8(skb, SMC_NLA_LGR_R_V2_DIRECT, !lgr->uses_gateway))
 		goto errv2attr;
+	if (nla_put_u8(skb, SMC_NLA_LGR_R_V2_MAX_CONNS, lgr->max_conns))
+		goto errv2attr;
+	if (nla_put_u8(skb, SMC_NLA_LGR_R_V2_MAX_LINKS, lgr->max_links))
+		goto errv2attr;
 
 	nla_nest_end(skb, v2_attrs);
 	return 0;
-- 
2.24.3 (Apple Git-128)