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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxxxx>
Signed-off-by: Wen Gu <guwen@xxxxxxxxxxxxxxxxx>
---
  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.

[...]




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux