diff mbox series

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

Message ID 20230816083328.95746-3-guangguan.wang@linux.alibaba.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net/smc: several features's implementation for smc v2.1 | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1330 this patch: 1330
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 1353 this patch: 1353
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1353 this patch: 1353
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Guangguan Wang Aug. 16, 2023, 8:33 a.m. UTC 
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(-)

Comments

Jan Karcher Aug. 16, 2023, 9:49 p.m. UTC | #1 
Hi Guangguan Wang,

thank you, some minor thoughts on this one.

On 16/08/2023 10:33, Guangguan Wang wrote:
> 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 97265691bc95..7b54c153bd0d 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 7c5627c6abcc..624dc970d187 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_nr)
> +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_nr;
> -	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_nr;

I don't like that this is called fce_v20.release which can be set to 
v2.1 here although the struct is named v20. Maybe let us call the struct 
something like fce_v2_base or fce_base_v2.

> +	memcpy(fce->fce_v20.hostname, smc_hostname, sizeof(smc_hostname));
> +	if (ini->is_smcd && ini->release_nr < 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_nr);
> +			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_nr);
> -				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];

Could we either make those variables a bit more self explaining via 
their name (e.g. vendor_organization_uid) or adding a comment /* vendor 
organizationally unique identifier */

>   };
>   
>   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;

as stated at the top where the release is assigned i'm not completly 
happy with the naming.

> +	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];
Guangguan Wang Aug. 17, 2023, 3:23 a.m. UTC | #2 
On 2023/8/17 05:49, Jan Karcher wrote:
> Hi Guangguan Wang,
> 
> thank you, some minor thoughts on this one.
> 
> On 16/08/2023 10:33, Guangguan Wang wrote:
...
>>   -static void smc_clc_fill_fce(struct smc_clc_first_contact_ext *fce, int *len, int release_nr)
>> +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_nr;
>> -    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_nr;
> 
> I don't like that this is called fce_v20.release which can be set to v2.1 here although the struct is named v20. Maybe let us call the struct something like fce_v2_base or fce_base_v2.
> 

fce_v2_base sounds better.


>> 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];
> 
> Could we either make those variables a bit more self explaining via their name (e.g. vendor_organization_uid) or adding a comment /* vendor organizationally unique identifier */
> 

I will fix it in the next version.

>>   };
>>     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;
> 
> as stated at the top where the release is assigned i'm not completly happy with the naming.
> 

Thanks,
Guangguan Wang
diff mbox series

Patch

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 97265691bc95..7b54c153bd0d 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 7c5627c6abcc..624dc970d187 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_nr)
+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_nr;
-	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_nr;
+	memcpy(fce->fce_v20.hostname, smc_hostname, sizeof(smc_hostname));
+	if (ini->is_smcd && ini->release_nr < 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_nr);
+			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_nr);
-				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];