Re: [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2

[Wen Gu <guwen@linux.alibaba.com>]

On 2023/12/18 16:39, Alexandra Winter wrote:
> 
> 
> On 12.12.23 09:52, Wen Gu wrote:
>> The structs of CLC accept and confirm messages for SMCv1 and SMCv2 are
>> separately defined and often casted to each other in the code, which may
>> increase the risk of errors caused by future divergence of them. So
>> unify them into one struct for better maintainability.
>>
>> Suggested-by: Alexandra Winter <wintera@linux.ibm.com>
>> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
>> ---
>>   net/smc/af_smc.c  | 50 +++++++++++++++---------------------------
>>   net/smc/smc_clc.c | 65 ++++++++++++++++++++++++-------------------------------
>>   net/smc/smc_clc.h | 32 ++++++++++-----------------
>>   3 files changed, 57 insertions(+), 90 deletions(-)
>>
> 
> [...]
> Thank you very much, Wen Gu. I think this makes it much easier to spot the
> places in the accept/confirm code code where v1 vs v2 really make a difference.
> I understand that this is not really related to v2.1, but I feel it is worth
> simplifying the already complex strucutres before adding even more complexity.
> 
> 
> 
>> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
>> index 1697b84..614fa2f 100644
>> --- a/net/smc/smc_clc.h
>> +++ b/net/smc/smc_clc.h
>> @@ -259,29 +259,22 @@ struct smc_clc_fce_gid_ext {
>>   struct smc_clc_msg_accept_confirm {	/* clc accept / confirm message */
>>   	struct smc_clc_msg_hdr hdr;
>>   	union {
>> -		struct smcr_clc_msg_accept_confirm r0; /* SMC-R */
>> -		struct { /* SMC-D */
>> -			struct smcd_clc_msg_accept_confirm_common d0;
>> -			u32 reserved5[3];
>> -		};
>> -	};
>> -} __packed;			/* format defined in RFC7609 */
>> -
>> -struct smc_clc_msg_accept_confirm_v2 {	/* clc accept / confirm message */
>> -	struct smc_clc_msg_hdr hdr;
>> -	union {
>>   		struct { /* SMC-R */
>> -			struct smcr_clc_msg_accept_confirm r0;
>> +			struct smcr_clc_msg_accept_confirm _r0;
>> +			/* v2 only, reserved and ignored in v1: */
>>   			u8 eid[SMC_MAX_EID_LEN];
>>   			u8 reserved6[8];
>>   		} r1;
>>   		struct { /* SMC-D */
>> -			struct smcd_clc_msg_accept_confirm_common d0;
>> +			struct smcd_clc_msg_accept_confirm_common _d0;
>> +			/* v2 only, reserved and ignored in v1: */
>>   			__be16 chid;
>>   			u8 eid[SMC_MAX_EID_LEN];
>>   			u8 reserved5[8];
>>   		} d1;
>>   	};
>> +#define r0	r1._r0
>> +#define d0	d1._d0
> 
> This adds complexity.
> If you add the v2-only fields to struct smcr_clc_msg_accept_confirm and
> struct smcd_clc_msg_accept_confirm_common respectively, you can avoid the
> #define and the extra layer in the struct.
> Actually there are already v2-only fields in smcd_clc_msg_accept_confirm_common
> and smcd_clc_msg_accept_confirm_common (gid and others). So you could add the
> correct informative comments there.

Thank you very much for the suggestions, Sandy.

I checked the history commits:
c758dfddc1b5 ("net/smc: add SMC-D support in CLC messages")
3d9725a6a133 ("net/smc: common routine for CLC accept and confirm")
a7c9c5f4af7f ("net/smc: CLC accept / confirm V2")
e5c4744cfb59 ("net/smc: add SMC-Rv2 connection establishment")

The fields in smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common
seem to have not changed since SMCDv1. So I guess there is no v2-only fields
in this two struct. I tried to confirm this in some documents but didn't find
the message format for v1.

If the smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common
is inherited from v1, should we still put the fields of v2 into these two structures?

If still, I will change these structures as

diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 614fa2f298f5..18157aeb14ec 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -201,9 +201,12 @@ struct smcr_clc_msg_accept_confirm {       /* SMCR accept/confirm */
         __be64 rmb_dma_addr;    /* RMB virtual address */
         u8 reserved2;
         u8 psn[3];              /* packet sequence number */
+       /* v2 only, reserved and ignored in v1: */
+       u8 eid[SMC_MAX_EID_LEN];
+       u8 reserved6[8];
  } __packed;

-struct smcd_clc_msg_accept_confirm_common {    /* SMCD accept/confirm */
+struct smcd_clc_msg_accept_confirm {   /* SMCD accept/confirm */
         __be64 gid;             /* Sender GID */
         __be64 token;           /* DMB token */
         u8 dmbe_idx;            /* DMBE index */
@@ -216,6 +219,10 @@ struct smcd_clc_msg_accept_confirm_common {        /* SMCD accept/confirm */
  #endif
         u16 reserved4;
         __be32 linkid;          /* Link identifier */
+       /* v2 only, reserved and ignored in v1: */
+       __be16 chid;
+       u8 eid[SMC_MAX_EID_LEN];
+       u8 reserved5[8];
  } __packed;

  #define SMC_CLC_OS_ZOS         1
@@ -259,22 +266,9 @@ struct smc_clc_fce_gid_ext {
  struct smc_clc_msg_accept_confirm {    /* clc accept / confirm message */
         struct smc_clc_msg_hdr hdr;
         union {
-               struct { /* SMC-R */
-                       struct smcr_clc_msg_accept_confirm _r0;
-                       /* v2 only, reserved and ignored in v1: */
-                       u8 eid[SMC_MAX_EID_LEN];
-                       u8 reserved6[8];
-               } r1;
-               struct { /* SMC-D */
-                       struct smcd_clc_msg_accept_confirm_common _d0;
-                       /* v2 only, reserved and ignored in v1: */
-                       __be16 chid;
-                       u8 eid[SMC_MAX_EID_LEN];
-                       u8 reserved5[8];
-               } d1;
+               struct smcr_clc_msg_accept_confirm r0; /* SMC-R */
+               struct smcd_clc_msg_accept_confirm d0; /* SMC-D */
         };
-#define r0     r1._r0
-#define d0     d1._d0
  };

> 
>>   };
> 
> You have removed the __packed attribute.
> patch 07/10 adds it back for the SMC-D case, but the SMC-R case needs it as well.
> 

r1 and d1 in smc_clc_msg_accept_confirm_v2 (smc_clc_msg_accept_confirm now in
this patch) is aligned well. In patch 07/10 I replaced reserved5[8] with u64 gid_ext,
thus making a hole before gid_ext, so I added __packed attribute to SMC-D.

If it is to avoid potential mistakes in future expansion, I can also add __packed to SMC-R.

Thanks.
> 
> [...]