| Message ID | 1702371151-125258-4-git-send-email-guwen@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/smc: implement SMCv2.1 virtual ISM device support | expand |
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. > }; 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. [...]
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. > > [...]
On 18.12.23 13:21, Wen Gu wrote: > 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. V1 is documented in https://datatracker.ietf.org/doc/html/draft-fox-tcpm-shared-memory-rdma-03 > > 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? You are right, they do not contain v2 fields, I guess I was confused. I still think, it would be better for readability and maintainability to avoid +#define r0 r1._r0 +#define d0 d1._d0 I guess you and previous editors wanted to avoid changing all the instances that use r0 and d0. But then.. it is a rather simple search/replace.. > > 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: */ ^^ Actually these commetns are not fully correct. The fields are not reserved in V1. (my bad) The message length is shorter in V1. So /* v2 only: */ would be more correct. > - 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: */ same here: /* v2 only: */ > - __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 > }; > >> >>> }; Yes, I like that solution better. But I have no strong feelings. At least the duplicate declarations are gone. So, if you prefer the #defines , it's ok with me. >> >> 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. > Yes, __packed is not only about preventing misalignement today. IMU, without __packed, there is no guarantee that a future compile run will not insert unused bytes. (highly unlikely, I admit). But __packed makes it visible that this needs to go to hardware in exactly this layout. > Thanks.
On 2023/12/19 01:40, Alexandra Winter wrote: > > > On 18.12.23 13:21, Wen Gu wrote: >> 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. > > V1 is documented in > https://datatracker.ietf.org/doc/html/draft-fox-tcpm-shared-memory-rdma-03 > Thank you, Sandy. It clearly shows the SMC-Rv1 message format. I guess SMC-Dv1 message format is not publicly documented? >> >> 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? > > You are right, they do not contain v2 fields, I guess I was confused. > > I still think, it would be better for readability and maintainability to avoid > +#define r0 r1._r0 > +#define d0 d1._d0 > I agree. Macros may cause some unexpected substitutions. I will remove them. > I guess you and previous editors wanted to avoid changing all the instances that use r0 and d0. > But then.. it is a rather simple search/replace.. > Yes, but not exactly. clc->r1.r0.xxx is somewhat strange for me, compared to clc->r0.xxx. So I try to avoid it. >> >> 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: */ > > ^^ Actually these commetns are not fully correct. The fields are not reserved in V1. > (my bad) The message length is shorter in V1. > So /* v2 only: */ would be more correct. > >> - 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: */ > > same here: /* v2 only: */ > >> - __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 >> }; >> >>> >>>> }; > > Yes, I like that solution better. > But I have no strong feelings. At least the duplicate declarations are gone. > So, if you prefer the #defines , it's ok with me. > After wrestling with several options, I decided to go with this definition. 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; - u8 eid[SMC_MAX_EID_LEN]; - u8 reserved6[8]; - } r1; + struct { /* v2 only */ + u8 eid[SMC_MAX_EID_LEN]; + u8 reserved6[8]; + } __packed r1; + }; struct { /* SMC-D */ struct smcd_clc_msg_accept_confirm_common d0; - __be16 chid; - u8 eid[SMC_MAX_EID_LEN]; - u8 reserved5[8]; - } d1; + struct { /* v2 only, but 12 bytes reserved in v1 */ + __be16 chid; + u8 eid[SMC_MAX_EID_LEN]; + u8 reserved5[8]; + } __packed d1; + }; }; }; Based on these considerations: - smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common are inherited from v1 or common with v1, I think it's better to reflect this in the definition. So I didn't put the v2 fields into them. - d1 and r1 is used as name of following v2 fields, so that no need to change the instances that use r0/d0/r1/d1, and no need to use macro. - __packed is added in d1 and r1 since smc_clc_msg_hdr, smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common is packed, and the subsequent modifications will most likely occur on d1 and r1 (e.g. using the reserved fields). - Add the comment 'but 12 bytes reserved in v1' since I guess people may wonder why SMCD_CLC_ACCEPT_CONFIRM_LEN is defined as 48. The comments could be a explanation. (I have also considered using a union to show the 12 bytes, but this would make the structure appear complicated. Given that the length of the SMCDv2 field has already exceeded 12 bytes and cannot be shortened, I think it is fine as it is.) > > >>> >>> 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. >> > > Yes, __packed is not only about preventing misalignement today. > IMU, without __packed, there is no guarantee that a future compile run will not insert unused bytes. > (highly unlikely, I admit). But __packed makes it visible that this needs to go to hardware in exactly > this layout. > Agree. I will add them to r1 and d1 definition. Thank you. Wen Gu > >> Thanks.
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c index 7fc2f3c..30dfc4cf 100644 --- a/net/smc/af_smc.c +++ b/net/smc/af_smc.c @@ -677,8 +677,6 @@ static bool smc_isascii(char *hostname) static void smc_conn_save_peer_info_fce(struct smc_sock *smc, struct smc_clc_msg_accept_confirm *clc) { - struct smc_clc_msg_accept_confirm_v2 *clc_v2 = - (struct smc_clc_msg_accept_confirm_v2 *)clc; struct smc_clc_first_contact_ext *fce; int clc_v2_len; @@ -687,17 +685,17 @@ static void smc_conn_save_peer_info_fce(struct smc_sock *smc, return; if (smc->conn.lgr->is_smcd) { - memcpy(smc->conn.lgr->negotiated_eid, clc_v2->d1.eid, + memcpy(smc->conn.lgr->negotiated_eid, clc->d1.eid, SMC_MAX_EID_LEN); - clc_v2_len = offsetofend(struct smc_clc_msg_accept_confirm_v2, + clc_v2_len = offsetofend(struct smc_clc_msg_accept_confirm, d1); } else { - memcpy(smc->conn.lgr->negotiated_eid, clc_v2->r1.eid, + memcpy(smc->conn.lgr->negotiated_eid, clc->r1.eid, SMC_MAX_EID_LEN); - clc_v2_len = offsetofend(struct smc_clc_msg_accept_confirm_v2, + clc_v2_len = offsetofend(struct smc_clc_msg_accept_confirm, r1); } - fce = (struct smc_clc_first_contact_ext *)(((u8 *)clc_v2) + clc_v2_len); + fce = (struct smc_clc_first_contact_ext *)(((u8 *)clc) + clc_v2_len); smc->conn.lgr->peer_os = fce->os_type; smc->conn.lgr->peer_smc_release = fce->release; if (smc_isascii(fce->hostname)) @@ -1149,13 +1147,13 @@ 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_msg_accept_confirm) + \ sizeof(struct smc_clc_first_contact_ext_v2x) + \ sizeof(struct smc_clc_msg_trail)) /* CLC handshake during connect */ static int smc_connect_clc(struct smc_sock *smc, - struct smc_clc_msg_accept_confirm_v2 *aclc2, + struct smc_clc_msg_accept_confirm *aclc, struct smc_init_info *ini) { int rc = 0; @@ -1165,7 +1163,7 @@ static int smc_connect_clc(struct smc_sock *smc, if (rc) return rc; /* receive SMC Accept CLC message */ - return smc_clc_wait_msg(smc, aclc2, SMC_CLC_MAX_ACCEPT_LEN, + return smc_clc_wait_msg(smc, aclc, SMC_CLC_MAX_ACCEPT_LEN, SMC_CLC_ACCEPT, CLC_WAIT_TIME); } @@ -1201,10 +1199,8 @@ static int smc_connect_rdma_v2_prepare(struct smc_sock *smc, struct smc_clc_msg_accept_confirm *aclc, struct smc_init_info *ini) { - struct smc_clc_msg_accept_confirm_v2 *clc_v2 = - (struct smc_clc_msg_accept_confirm_v2 *)aclc; struct smc_clc_first_contact_ext *fce = - smc_get_clc_first_contact_ext(clc_v2, false); + smc_get_clc_first_contact_ext(aclc, false); struct net *net = sock_net(&smc->sk); int rc; @@ -1327,10 +1323,7 @@ static int smc_connect_rdma(struct smc_sock *smc, } if (aclc->hdr.version > SMC_V1) { - struct smc_clc_msg_accept_confirm_v2 *clc_v2 = - (struct smc_clc_msg_accept_confirm_v2 *)aclc; - - eid = clc_v2->r1.eid; + eid = aclc->r1.eid; if (ini->first_contact_local) smc_fill_gid_list(link->lgr, &ini->smcrv2.gidlist, link->smcibdev, link->gid); @@ -1371,7 +1364,7 @@ static int smc_connect_rdma(struct smc_sock *smc, * Determine from the CHID of the received CLC ACCEPT the ISM device chosen. */ static int -smc_v2_determine_accepted_chid(struct smc_clc_msg_accept_confirm_v2 *aclc, +smc_v2_determine_accepted_chid(struct smc_clc_msg_accept_confirm *aclc, struct smc_init_info *ini) { int i; @@ -1398,12 +1391,9 @@ static int smc_connect_ism(struct smc_sock *smc, ini->first_contact_peer = aclc->hdr.typev2 & SMC_FIRST_CONTACT_MASK; if (aclc->hdr.version == SMC_V2) { - 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); + smc_get_clc_first_contact_ext(aclc, true); ini->release_nr = fce->release; rc = smc_clc_clnt_v2x_features_validate(fce, ini); @@ -1411,7 +1401,7 @@ static int smc_connect_ism(struct smc_sock *smc, return rc; } - rc = smc_v2_determine_accepted_chid(aclc_v2, ini); + rc = smc_v2_determine_accepted_chid(aclc, ini); if (rc) return rc; } @@ -1437,12 +1427,8 @@ static int smc_connect_ism(struct smc_sock *smc, smc_rx_init(smc); smc_tx_init(smc); - if (aclc->hdr.version > SMC_V1) { - struct smc_clc_msg_accept_confirm_v2 *clc_v2 = - (struct smc_clc_msg_accept_confirm_v2 *)aclc; - - eid = clc_v2->d1.eid; - } + if (aclc->hdr.version > SMC_V1) + eid = aclc->d1.eid; rc = smc_clc_send_confirm(smc, ini->first_contact_local, aclc->hdr.version, eid, ini); @@ -1493,7 +1479,6 @@ static int smc_connect_check_aclc(struct smc_init_info *ini, static int __smc_connect(struct smc_sock *smc) { u8 version = smc_ism_is_v2_capable() ? SMC_V2 : SMC_V1; - struct smc_clc_msg_accept_confirm_v2 *aclc2; struct smc_clc_msg_accept_confirm *aclc; struct smc_init_info *ini = NULL; u8 *buf = NULL; @@ -1541,11 +1526,10 @@ static int __smc_connect(struct smc_sock *smc) rc = SMC_CLC_DECL_MEM; goto fallback; } - aclc2 = (struct smc_clc_msg_accept_confirm_v2 *)buf; - aclc = (struct smc_clc_msg_accept_confirm *)aclc2; + aclc = (struct smc_clc_msg_accept_confirm *)buf; /* perform CLC handshake */ - rc = smc_connect_clc(smc, aclc2, ini); + rc = smc_connect_clc(smc, aclc, ini); if (rc) { /* -EAGAIN on timeout, see tcp_recvmsg() */ if (rc == -EAGAIN) { diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c index 1a230d8..6473333 100644 --- a/net/smc/smc_clc.c +++ b/net/smc/smc_clc.c @@ -377,9 +377,9 @@ static bool smc_clc_msg_prop_valid(struct smc_clc_msg_proposal *pclc) /* check arriving CLC accept or confirm */ static bool -smc_clc_msg_acc_conf_valid(struct smc_clc_msg_accept_confirm_v2 *clc_v2) +smc_clc_msg_acc_conf_valid(struct smc_clc_msg_accept_confirm *clc) { - struct smc_clc_msg_hdr *hdr = &clc_v2->hdr; + struct smc_clc_msg_hdr *hdr = &clc->hdr; if (hdr->typev1 != SMC_TYPE_R && hdr->typev1 != SMC_TYPE_D) return false; @@ -449,7 +449,7 @@ static int smc_clc_fill_fce_v2x(struct smc_clc_first_contact_ext_v2x *fce_v2x, */ static bool smc_clc_msg_hdr_valid(struct smc_clc_msg_hdr *clcm, bool check_trl) { - struct smc_clc_msg_accept_confirm_v2 *clc_v2; + struct smc_clc_msg_accept_confirm *clc; struct smc_clc_msg_proposal *pclc; struct smc_clc_msg_decline *dclc; struct smc_clc_msg_trail *trl; @@ -467,12 +467,11 @@ static bool smc_clc_msg_hdr_valid(struct smc_clc_msg_hdr *clcm, bool check_trl) break; case SMC_CLC_ACCEPT: case SMC_CLC_CONFIRM: - clc_v2 = (struct smc_clc_msg_accept_confirm_v2 *)clcm; - if (!smc_clc_msg_acc_conf_valid(clc_v2)) + clc = (struct smc_clc_msg_accept_confirm *)clcm; + if (!smc_clc_msg_acc_conf_valid(clc)) return false; trl = (struct smc_clc_msg_trail *) - ((u8 *)clc_v2 + ntohs(clc_v2->hdr.length) - - sizeof(*trl)); + ((u8 *)clc + ntohs(clc->hdr.length) - sizeof(*trl)); break; case SMC_CLC_DECLINE: dclc = (struct smc_clc_msg_decline *)clcm; @@ -1000,7 +999,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) static void smcd_clc_prep_confirm_accept(struct smc_connection *conn, - struct smc_clc_msg_accept_confirm_v2 *clc_v2, + struct smc_clc_msg_accept_confirm *clc, int first_contact, u8 version, u8 *eid, struct smc_init_info *ini, int *fce_len, @@ -1008,11 +1007,9 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) struct smc_clc_msg_trail *trl) { struct smcd_dev *smcd = conn->lgr->smcd; - struct smc_clc_msg_accept_confirm *clc; int len; /* SMC-D specific settings */ - clc = (struct smc_clc_msg_accept_confirm *)clc_v2; memcpy(clc->hdr.eyecatcher, SMCD_EYECATCHER, sizeof(SMCD_EYECATCHER)); clc->hdr.typev1 = SMC_TYPE_D; @@ -1024,15 +1021,15 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) if (version == SMC_V1) { clc->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN); } else { - clc_v2->d1.chid = htons(smc_ism_get_chid(smcd)); + clc->d1.chid = htons(smc_ism_get_chid(smcd)); if (eid && eid[0]) - memcpy(clc_v2->d1.eid, eid, SMC_MAX_EID_LEN); + memcpy(clc->d1.eid, eid, SMC_MAX_EID_LEN); len = SMCD_CLC_ACCEPT_CONFIRM_LEN_V2; if (first_contact) { *fce_len = smc_clc_fill_fce_v2x(fce_v2x, ini); len += *fce_len; } - clc_v2->hdr.length = htons(len); + clc->hdr.length = htons(len); } memcpy(trl->eyecatcher, SMCD_EYECATCHER, sizeof(SMCD_EYECATCHER)); @@ -1040,7 +1037,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) static void smcr_clc_prep_confirm_accept(struct smc_connection *conn, - struct smc_clc_msg_accept_confirm_v2 *clc_v2, + struct smc_clc_msg_accept_confirm *clc, int first_contact, u8 version, u8 *eid, struct smc_init_info *ini, int *fce_len, @@ -1048,12 +1045,10 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) struct smc_clc_fce_gid_ext *gle, struct smc_clc_msg_trail *trl) { - struct smc_clc_msg_accept_confirm *clc; struct smc_link *link = conn->lnk; int len; /* SMC-R specific settings */ - clc = (struct smc_clc_msg_accept_confirm *)clc_v2; memcpy(clc->hdr.eyecatcher, SMC_EYECATCHER, sizeof(SMC_EYECATCHER)); clc->hdr.typev1 = SMC_TYPE_R; @@ -1085,7 +1080,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN); } else { if (eid && eid[0]) - memcpy(clc_v2->r1.eid, eid, SMC_MAX_EID_LEN); + memcpy(clc->r1.eid, eid, SMC_MAX_EID_LEN); len = SMCR_CLC_ACCEPT_CONFIRM_LEN_V2; if (first_contact) { *fce_len = smc_clc_fill_fce_v2x(fce_v2x, ini); @@ -1099,20 +1094,19 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) len += gle->gid_cnt * sizeof(gle->gid[0]); } } - clc_v2->hdr.length = htons(len); + clc->hdr.length = htons(len); } memcpy(trl->eyecatcher, SMC_EYECATCHER, sizeof(SMC_EYECATCHER)); } /* build and send CLC CONFIRM / ACCEPT message */ static int smc_clc_send_confirm_accept(struct smc_sock *smc, - struct smc_clc_msg_accept_confirm_v2 *clc_v2, + struct smc_clc_msg_accept_confirm *clc, int first_contact, u8 version, u8 *eid, struct smc_init_info *ini) { struct smc_clc_first_contact_ext_v2x fce_v2x; struct smc_connection *conn = &smc->conn; - struct smc_clc_msg_accept_confirm *clc; struct smc_clc_fce_gid_ext gle; struct smc_clc_msg_trail trl; int i, fce_len; @@ -1120,21 +1114,20 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc, struct msghdr msg; /* send SMC Confirm CLC msg */ - clc = (struct smc_clc_msg_accept_confirm *)clc_v2; clc->hdr.version = version; /* SMC version */ if (first_contact) clc->hdr.typev2 |= SMC_FIRST_CONTACT_MASK; if (conn->lgr->is_smcd) - smcd_clc_prep_confirm_accept(conn, clc_v2, first_contact, + smcd_clc_prep_confirm_accept(conn, clc, first_contact, version, eid, ini, &fce_len, &fce_v2x, &trl); else - smcr_clc_prep_confirm_accept(conn, clc_v2, first_contact, + smcr_clc_prep_confirm_accept(conn, clc, first_contact, version, eid, ini, &fce_len, &fce_v2x, &gle, &trl); memset(&msg, 0, sizeof(msg)); i = 0; - vec[i].iov_base = clc_v2; + vec[i].iov_base = clc; if (version > SMC_V1) vec[i++].iov_len = (clc->hdr.typev1 == SMC_TYPE_D ? SMCD_CLC_ACCEPT_CONFIRM_LEN_V2 : @@ -1168,16 +1161,16 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc, int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact, u8 version, u8 *eid, struct smc_init_info *ini) { - struct smc_clc_msg_accept_confirm_v2 cclc_v2; + struct smc_clc_msg_accept_confirm cclc; int reason_code = 0; int len; /* send SMC Confirm CLC msg */ - memset(&cclc_v2, 0, sizeof(cclc_v2)); - cclc_v2.hdr.type = SMC_CLC_CONFIRM; - len = smc_clc_send_confirm_accept(smc, &cclc_v2, clnt_first_contact, + memset(&cclc, 0, sizeof(cclc)); + cclc.hdr.type = SMC_CLC_CONFIRM; + len = smc_clc_send_confirm_accept(smc, &cclc, clnt_first_contact, version, eid, ini); - if (len < ntohs(cclc_v2.hdr.length)) { + if (len < ntohs(cclc.hdr.length)) { if (len >= 0) { reason_code = -ENETUNREACH; smc->sk.sk_err = -reason_code; @@ -1193,14 +1186,14 @@ int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact, int smc_clc_send_accept(struct smc_sock *new_smc, bool srv_first_contact, u8 version, u8 *negotiated_eid, struct smc_init_info *ini) { - struct smc_clc_msg_accept_confirm_v2 aclc_v2; + struct smc_clc_msg_accept_confirm aclc; int len; - 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, + memset(&aclc, 0, sizeof(aclc)); + aclc.hdr.type = SMC_CLC_ACCEPT; + len = smc_clc_send_confirm_accept(new_smc, &aclc, srv_first_contact, version, negotiated_eid, ini); - if (len < ntohs(aclc_v2.hdr.length)) + if (len < ntohs(aclc.hdr.length)) len = len >= 0 ? -EPROTO : -new_smc->clcsock->sk->sk_err; return len > 0 ? 0 : len; @@ -1265,10 +1258,8 @@ int smc_clc_clnt_v2x_features_validate(struct smc_clc_first_contact_ext *fce, 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); + smc_get_clc_first_contact_ext(cclc, ini->is_smcd); struct smc_clc_first_contact_ext_v2x *fce_v2x = (struct smc_clc_first_contact_ext_v2x *)fce; 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 }; struct smc_clc_msg_decline { /* clc decline message */ @@ -389,24 +382,23 @@ static inline u8 smc_indicated_type(int is_smcd, int is_smcr) } static inline struct smc_clc_first_contact_ext * -smc_get_clc_first_contact_ext(struct smc_clc_msg_accept_confirm_v2 *clc_v2, +smc_get_clc_first_contact_ext(struct smc_clc_msg_accept_confirm *clc, bool is_smcd) { int clc_v2_len; - if (clc_v2->hdr.version == SMC_V1 || - !(clc_v2->hdr.typev2 & SMC_FIRST_CONTACT_MASK)) + if (clc->hdr.version == SMC_V1 || + !(clc->hdr.typev2 & SMC_FIRST_CONTACT_MASK)) return NULL; if (is_smcd) clc_v2_len = - offsetofend(struct smc_clc_msg_accept_confirm_v2, d1); + offsetofend(struct smc_clc_msg_accept_confirm, d1); else clc_v2_len = - offsetofend(struct smc_clc_msg_accept_confirm_v2, r1); + offsetofend(struct smc_clc_msg_accept_confirm, r1); - return (struct smc_clc_first_contact_ext *)(((u8 *)clc_v2) + - clc_v2_len); + return (struct smc_clc_first_contact_ext *)(((u8 *)clc) + clc_v2_len); } struct smcd_dev;
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(-)