Messages in this thread Patch in this message | | | Date | Mon, 18 Dec 2023 20:21:28 +0800 | Subject | Re: [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2 | From | Wen Gu <> |
| |
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. > > [...]
| |