| Message ID | 1702021259-41504-3-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 2023/12/8 15:40, Wen Gu wrote: > There is a large if-else block in smc_clc_send_confirm_accept() and it > is better to split it into two sub-functions. > > Suggested-by: Alexandra Winter <wintera@linux.ibm.com> > Signed-off-by: Wen Gu <guwen@linux.alibaba.com> > --- > net/smc/smc_clc.c | 196 +++++++++++++++++++++++++++++++----------------------- > 1 file changed, 114 insertions(+), 82 deletions(-) > > diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c > index 0fcb035..52b4ea9 100644 > --- a/net/smc/smc_clc.c > +++ b/net/smc/smc_clc.c > @@ -998,6 +998,111 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) > return reason_code; > } > > +static void smcd_clc_prep_confirm_accept(struct smc_connection *conn, > + struct smc_clc_msg_accept_confirm_v2 *clc_v2, checkpatch will complain 'Alignment should match open parenthesis' here. But in order to make the length less than 80 columns, there seems to be no other good way. > + int first_contact, u8 version, > + u8 *eid, struct smc_init_info *ini, > + int *fce_len, > + struct smc_clc_first_contact_ext_v2x *fce_v2x, > + struct smc_clc_msg_trail *trl) > +{ <...> > + > +static void smcr_clc_prep_confirm_accept(struct smc_connection *conn, > + struct smc_clc_msg_accept_confirm_v2 *clc_v2, And here. > + int first_contact, u8 version, > + u8 *eid, struct smc_init_info *ini, > + int *fce_len, > + struct smc_clc_first_contact_ext_v2x *fce_v2x, > + struct smc_clc_fce_gid_ext *gle, > + struct smc_clc_msg_trail *trl) > +{ <...>
On 09.12.23 03:50, Wen Gu wrote: > > > On 2023/12/8 15:40, Wen Gu wrote: > >> There is a large if-else block in smc_clc_send_confirm_accept() and it >> is better to split it into two sub-functions. >> >> Suggested-by: Alexandra Winter <wintera@linux.ibm.com> >> Signed-off-by: Wen Gu <guwen@linux.alibaba.com> >> --- >> net/smc/smc_clc.c | 196 +++++++++++++++++++++++++++++++----------------------- >> 1 file changed, 114 insertions(+), 82 deletions(-) >> >> diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c >> index 0fcb035..52b4ea9 100644 >> --- a/net/smc/smc_clc.c >> +++ b/net/smc/smc_clc.c >> @@ -998,6 +998,111 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) >> return reason_code; >> } >> +static void smcd_clc_prep_confirm_accept(struct smc_connection *conn, >> + struct smc_clc_msg_accept_confirm_v2 *clc_v2, > > checkpatch will complain 'Alignment should match open parenthesis' here. > But in order to make the length less than 80 columns, there seems to be > no other good way. > >> + int first_contact, u8 version, >> + u8 *eid, struct smc_init_info *ini, >> + int *fce_len, >> + struct smc_clc_first_contact_ext_v2x *fce_v2x, >> + struct smc_clc_msg_trail *trl) >> +{ > <...> > >> + >> +static void smcr_clc_prep_confirm_accept(struct smc_connection *conn, >> + struct smc_clc_msg_accept_confirm_v2 *clc_v2, > > And here. > >> + int first_contact, u8 version, >> + u8 *eid, struct smc_init_info *ini, >> + int *fce_len, >> + struct smc_clc_first_contact_ext_v2x *fce_v2x, >> + struct smc_clc_fce_gid_ext *gle, >> + struct smc_clc_msg_trail *trl) >> +{ > <...> > You could shorten the names of the functions
On 08.12.23 08:40, Wen Gu wrote: > There is a large if-else block in smc_clc_send_confirm_accept() and it > is better to split it into two sub-functions. > > Suggested-by: Alexandra Winter <wintera@linux.ibm.com> > Signed-off-by: Wen Gu <guwen@linux.alibaba.com> > --- Thank you very much Wen Gu for improving the codebase. > net/smc/smc_clc.c | 196 +++++++++++++++++++++++++++++++----------------------- > 1 file changed, 114 insertions(+), 82 deletions(-) > > diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c > index 0fcb035..52b4ea9 100644 > --- a/net/smc/smc_clc.c > +++ b/net/smc/smc_clc.c > @@ -998,6 +998,111 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) > return reason_code; > } > > +static void smcd_clc_prep_confirm_accept(struct smc_connection *conn, > + struct smc_clc_msg_accept_confirm_v2 *clc_v2, > + int first_contact, u8 version, > + u8 *eid, struct smc_init_info *ini, > + int *fce_len, > + struct smc_clc_first_contact_ext_v2x *fce_v2x, > + 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; Why is this cast neccessary? (Here as well as in smcr_clc_prep_confirm_accept and in smc_clc_send_confirm_accept) smc_clc_msg_accept_confirm_v2 has hdr and d0 as well. IMO, it would be a nice seperate patch to get rid of the 2 type defs for smc_clc_msg_accept_confirm and smc_clc_msg_accept_confirm_v2 and all the related casting anyhow. > + memcpy(clc->hdr.eyecatcher, SMCD_EYECATCHER, > + sizeof(SMCD_EYECATCHER)); > + clc->hdr.typev1 = SMC_TYPE_D; > + clc->d0.gid = htonll(smcd->ops->get_local_gid(smcd)); > + clc->d0.token = htonll(conn->rmb_desc->token); > + clc->d0.dmbe_size = conn->rmbe_size_comp; > + clc->d0.dmbe_idx = 0; > + memcpy(&clc->d0.linkid, conn->lgr->id, SMC_LGR_ID_SIZE); > + if (version == SMC_V1) { > + clc->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN); > + } else { > + clc_v2->d1.chid = htons(smc_ism_get_chid(smcd)); > + if (eid && eid[0]) > + memcpy(clc_v2->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); > + } > + memcpy(trl->eyecatcher, SMCD_EYECATCHER, > + sizeof(SMCD_EYECATCHER)); > +} > + > +static void smcr_clc_prep_confirm_accept(struct smc_connection *conn, > + struct smc_clc_msg_accept_confirm_v2 *clc_v2, > + int first_contact, u8 version, > + u8 *eid, struct smc_init_info *ini, > + int *fce_len, > + struct smc_clc_first_contact_ext_v2x *fce_v2x, > + 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; Why is this cast neccessary? smc_clc_msg_accept_confirm_v2 has hdr and r0 as well. > + memcpy(clc->hdr.eyecatcher, SMC_EYECATCHER, > + sizeof(SMC_EYECATCHER)); > + clc->hdr.typev1 = SMC_TYPE_R; > + clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN); ^^ this is overwritten below, so no need to set it here. > + memcpy(clc->r0.lcl.id_for_peer, local_systemid, > + sizeof(local_systemid)); > + memcpy(&clc->r0.lcl.gid, link->gid, SMC_GID_SIZE); > + memcpy(&clc->r0.lcl.mac, &link->smcibdev->mac[link->ibport - 1], > + ETH_ALEN); > + hton24(clc->r0.qpn, link->roce_qp->qp_num); > + clc->r0.rmb_rkey = > + htonl(conn->rmb_desc->mr[link->link_idx]->rkey); > + clc->r0.rmbe_idx = 1; /* for now: 1 RMB = 1 RMBE */ > + clc->r0.rmbe_alert_token = htonl(conn->alert_token_local); > + switch (clc->hdr.type) { > + case SMC_CLC_ACCEPT: > + clc->r0.qp_mtu = link->path_mtu; > + break; > + case SMC_CLC_CONFIRM: > + clc->r0.qp_mtu = min(link->path_mtu, link->peer_mtu); > + break; > + } > + clc->r0.rmbe_size = conn->rmbe_size_comp; > + clc->r0.rmb_dma_addr = conn->rmb_desc->is_vm ? > + cpu_to_be64((uintptr_t)conn->rmb_desc->cpu_addr) : > + cpu_to_be64((u64)sg_dma_address > + (conn->rmb_desc->sgt[link->link_idx].sgl)); > + hton24(clc->r0.psn, link->psn_initial); > + if (version == SMC_V1) { > + clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN); > + } else { > + if (eid && eid[0]) > + memcpy(clc_v2->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); > + len += *fce_len; > + fce_v2x->fce_v2_base.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]); > + } > + } > + clc_v2->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, > @@ -1006,11 +1111,10 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc, > { > struct smc_clc_first_contact_ext_v2x fce_v2x; > struct smc_connection *conn = &smc->conn; > - struct smcd_dev *smcd = conn->lgr->smcd; > struct smc_clc_msg_accept_confirm *clc; > struct smc_clc_fce_gid_ext gle; > struct smc_clc_msg_trail trl; > - int i, len, fce_len; > + int i, fce_len; > struct kvec vec[5]; > struct msghdr msg; > > @@ -1019,86 +1123,14 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc, > clc->hdr.version = version; /* SMC version */ > if (first_contact) > clc->hdr.typev2 |= SMC_FIRST_CONTACT_MASK; > - if (conn->lgr->is_smcd) { > - /* SMC-D specific settings */ > - memcpy(clc->hdr.eyecatcher, SMCD_EYECATCHER, > - sizeof(SMCD_EYECATCHER)); > - clc->hdr.typev1 = SMC_TYPE_D; > - clc->d0.gid = htonll(smcd->ops->get_local_gid(smcd)); > - clc->d0.token = htonll(conn->rmb_desc->token); > - clc->d0.dmbe_size = conn->rmbe_size_comp; > - clc->d0.dmbe_idx = 0; > - memcpy(&clc->d0.linkid, conn->lgr->id, SMC_LGR_ID_SIZE); > - if (version == SMC_V1) { > - clc->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN); > - } else { > - clc_v2->d1.chid = htons(smc_ism_get_chid(smcd)); > - if (eid && eid[0]) > - memcpy(clc_v2->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); > - } > - memcpy(trl.eyecatcher, SMCD_EYECATCHER, > - sizeof(SMCD_EYECATCHER)); > - } else { > - struct smc_link *link = conn->lnk; > - > - /* SMC-R specific settings */ > - memcpy(clc->hdr.eyecatcher, SMC_EYECATCHER, > - sizeof(SMC_EYECATCHER)); > - clc->hdr.typev1 = SMC_TYPE_R; > - clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN); > - memcpy(clc->r0.lcl.id_for_peer, local_systemid, > - sizeof(local_systemid)); > - memcpy(&clc->r0.lcl.gid, link->gid, SMC_GID_SIZE); > - memcpy(&clc->r0.lcl.mac, &link->smcibdev->mac[link->ibport - 1], > - ETH_ALEN); > - hton24(clc->r0.qpn, link->roce_qp->qp_num); > - clc->r0.rmb_rkey = > - htonl(conn->rmb_desc->mr[link->link_idx]->rkey); > - clc->r0.rmbe_idx = 1; /* for now: 1 RMB = 1 RMBE */ > - clc->r0.rmbe_alert_token = htonl(conn->alert_token_local); > - switch (clc->hdr.type) { > - case SMC_CLC_ACCEPT: > - clc->r0.qp_mtu = link->path_mtu; > - break; > - case SMC_CLC_CONFIRM: > - clc->r0.qp_mtu = min(link->path_mtu, link->peer_mtu); > - break; > - } > - clc->r0.rmbe_size = conn->rmbe_size_comp; > - clc->r0.rmb_dma_addr = conn->rmb_desc->is_vm ? > - cpu_to_be64((uintptr_t)conn->rmb_desc->cpu_addr) : > - cpu_to_be64((u64)sg_dma_address > - (conn->rmb_desc->sgt[link->link_idx].sgl)); > - hton24(clc->r0.psn, link->psn_initial); > - if (version == SMC_V1) { > - clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN); > - } else { > - if (eid && eid[0]) > - memcpy(clc_v2->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); > - len += fce_len; > - fce_v2x.fce_v2_base.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]); > - } > - } > - clc_v2->hdr.length = htons(len); > - } > - memcpy(trl.eyecatcher, SMC_EYECATCHER, sizeof(SMC_EYECATCHER)); > - } > - > + if (conn->lgr->is_smcd) > + smcd_clc_prep_confirm_accept(conn, clc_v2, first_contact, > + version, eid, ini, &fce_len, > + &fce_v2x, &trl); > + else > + smcr_clc_prep_confirm_accept(conn, clc_v2, first_contact, > + version, eid, ini, &fce_len, > + &fce_v2x, &gle, &trl); > memset(&msg, 0, sizeof(msg)); > i = 0; > vec[i].iov_base = clc_v2;
On 2023/12/11 17:47, Alexandra Winter wrote: > > > On 09.12.23 03:50, Wen Gu wrote: >> >> >> On 2023/12/8 15:40, Wen Gu wrote: >> >>> There is a large if-else block in smc_clc_send_confirm_accept() and it >>> is better to split it into two sub-functions. >>> >>> Suggested-by: Alexandra Winter <wintera@linux.ibm.com> >>> Signed-off-by: Wen Gu <guwen@linux.alibaba.com> >>> --- >>> net/smc/smc_clc.c | 196 +++++++++++++++++++++++++++++++----------------------- >>> 1 file changed, 114 insertions(+), 82 deletions(-) >>> >>> diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c >>> index 0fcb035..52b4ea9 100644 >>> --- a/net/smc/smc_clc.c >>> +++ b/net/smc/smc_clc.c >>> @@ -998,6 +998,111 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) >>> return reason_code; >>> } >>> +static void smcd_clc_prep_confirm_accept(struct smc_connection *conn, >>> + struct smc_clc_msg_accept_confirm_v2 *clc_v2, >> >> checkpatch will complain 'Alignment should match open parenthesis' here. >> But in order to make the length less than 80 columns, there seems to be >> no other good way. >> >>> + int first_contact, u8 version, >>> + u8 *eid, struct smc_init_info *ini, >>> + int *fce_len, >>> + struct smc_clc_first_contact_ext_v2x *fce_v2x, >>> + struct smc_clc_msg_trail *trl) >>> +{ >> <...> >> >>> + >>> +static void smcr_clc_prep_confirm_accept(struct smc_connection *conn, >>> + struct smc_clc_msg_accept_confirm_v2 *clc_v2, >> >> And here. >> >>> + int first_contact, u8 version, >>> + u8 *eid, struct smc_init_info *ini, >>> + int *fce_len, >>> + struct smc_clc_first_contact_ext_v2x *fce_v2x, >>> + struct smc_clc_fce_gid_ext *gle, >>> + struct smc_clc_msg_trail *trl) >>> +{ >> <...> >> > > > You could shorten the names of the functions Thank you. I thought about that too, but I think shortening the name may have an impact on the understanding. I think the following may be another way out and checkpatch is happy: +static void +smcd_clc_prep_confirm_accept(struct smc_connection *conn, + struct smc_clc_msg_accept_confirm_v2 *clc_v2, + int first_contact, u8 version, + u8 *eid, struct smc_init_info *ini, + int *fce_len, + struct smc_clc_first_contact_ext_v2x *fce_v2x, + struct smc_clc_msg_trail *trl) and +static void +smcr_clc_prep_confirm_accept(struct smc_connection *conn, + struct smc_clc_msg_accept_confirm_v2 *clc_v2, + int first_contact, u8 version, + u8 *eid, struct smc_init_info *ini, + int *fce_len, + struct smc_clc_first_contact_ext_v2x *fce_v2x, + struct smc_clc_fce_gid_ext *gle, + struct smc_clc_msg_trail *trl)
On 2023/12/11 18:43, Alexandra Winter wrote: > > > On 08.12.23 08:40, Wen Gu wrote: >> There is a large if-else block in smc_clc_send_confirm_accept() and it >> is better to split it into two sub-functions. >> >> Suggested-by: Alexandra Winter <wintera@linux.ibm.com> >> Signed-off-by: Wen Gu <guwen@linux.alibaba.com> >> --- > > Thank you very much Wen Gu for improving the codebase. > I'm glad I could help. > >> net/smc/smc_clc.c | 196 +++++++++++++++++++++++++++++++----------------------- >> 1 file changed, 114 insertions(+), 82 deletions(-) >> >> diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c >> index 0fcb035..52b4ea9 100644 >> --- a/net/smc/smc_clc.c >> +++ b/net/smc/smc_clc.c >> @@ -998,6 +998,111 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) >> return reason_code; >> } >> >> +static void smcd_clc_prep_confirm_accept(struct smc_connection *conn, >> + struct smc_clc_msg_accept_confirm_v2 *clc_v2, >> + int first_contact, u8 version, >> + u8 *eid, struct smc_init_info *ini, >> + int *fce_len, >> + struct smc_clc_first_contact_ext_v2x *fce_v2x, >> + 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; > > Why is this cast neccessary? (Here as well as in smcr_clc_prep_confirm_accept > and in smc_clc_send_confirm_accept) > smc_clc_msg_accept_confirm_v2 has hdr and d0 as well. I think the cast is to imply that v2 is an expansion of v1, or v1 is the base of v2. So here using clc(v1) reperesents their common set. If we use smc_clc_msg_accept_confirm_v2 for all, I think readers may be tempted to check whether the hdr and d0 in 'smc_clc_msg_accept_confirm_v2' are also applicable to v1. And there are settings below that are specific for v1. It may be confusing if we change it like this: if (version == SMC_V1) { clc_v2->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN); } else { > > IMO, it would be a nice seperate patch to get rid of the 2 type defs for > smc_clc_msg_accept_confirm and smc_clc_msg_accept_confirm_v2 > and all the related casting anyhow. > Do you mean to define only smc_clc_msg_accept_confirm_v2 or define with the name of smc_clc_msg_accept_confirm but the contents of smc_clc_msg_accept_confirm_v2? I have a different opinion on this, since I think the smc_clc_msg_accept_confirm and smc_clc_msg_accept_confirm_v2 clearly shows the difference between v1 and v2 messages and remind people what is currently working on. So I perfer to keep them. Am I missing something? > > >> + memcpy(clc->hdr.eyecatcher, SMCD_EYECATCHER, >> + sizeof(SMCD_EYECATCHER)); >> + clc->hdr.typev1 = SMC_TYPE_D; >> + clc->d0.gid = htonll(smcd->ops->get_local_gid(smcd)); >> + clc->d0.token = htonll(conn->rmb_desc->token); >> + clc->d0.dmbe_size = conn->rmbe_size_comp; >> + clc->d0.dmbe_idx = 0; >> + memcpy(&clc->d0.linkid, conn->lgr->id, SMC_LGR_ID_SIZE); >> + if (version == SMC_V1) { >> + clc->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN); >> + } else { >> + clc_v2->d1.chid = htons(smc_ism_get_chid(smcd)); >> + if (eid && eid[0]) >> + memcpy(clc_v2->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); >> + } >> + memcpy(trl->eyecatcher, SMCD_EYECATCHER, >> + sizeof(SMCD_EYECATCHER)); >> +} >> + >> +static void smcr_clc_prep_confirm_accept(struct smc_connection *conn, >> + struct smc_clc_msg_accept_confirm_v2 *clc_v2, >> + int first_contact, u8 version, >> + u8 *eid, struct smc_init_info *ini, >> + int *fce_len, >> + struct smc_clc_first_contact_ext_v2x *fce_v2x, >> + 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; > > Why is this cast neccessary? > smc_clc_msg_accept_confirm_v2 has hdr and r0 as well. > Similar thought as SMCD. >> + memcpy(clc->hdr.eyecatcher, SMC_EYECATCHER, >> + sizeof(SMC_EYECATCHER)); >> + clc->hdr.typev1 = SMC_TYPE_R; >> + clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN); > > ^^ this is overwritten below, so no need to set it here. > Good catch! It will be removed. Thank you. <...>
On 11.12.23 13:15, Wen Gu wrote: >>> + clc = (struct smc_clc_msg_accept_confirm *)clc_v2; >> >> Why is this cast neccessary? (Here as well as in smcr_clc_prep_confirm_accept >> and in smc_clc_send_confirm_accept) >> smc_clc_msg_accept_confirm_v2 has hdr and d0 as well. > > I think the cast is to imply that v2 is an expansion of v1, or v1 is the base of v2. > So here using clc(v1) reperesents their common set. > > If we use smc_clc_msg_accept_confirm_v2 for all, I think readers may be tempted to > check whether the hdr and d0 in 'smc_clc_msg_accept_confirm_v2' are also applicable to v1. > > And there are settings below that are specific for v1. It may be confusing if we > change it like this: > > if (version == SMC_V1) { > clc_v2->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN); > } else { > > >> >> IMO, it would be a nice seperate patch to get rid of the 2 type defs for >> smc_clc_msg_accept_confirm and smc_clc_msg_accept_confirm_v2 >> and all the related casting anyhow. >> > > Do you mean to define only smc_clc_msg_accept_confirm_v2 or define with the name > of smc_clc_msg_accept_confirm but the contents of smc_clc_msg_accept_confirm_v2? > > I have a different opinion on this, since I think the smc_clc_msg_accept_confirm > and smc_clc_msg_accept_confirm_v2 clearly shows the difference between v1 and > v2 messages and remind people what is currently working on. So I perfer to keep them. > Am I missing something? > This is a discussion about coding style, readability and maintainability (avoid future errors). And the code works today and the rest is opinions. That said, let me list some arguments why I don't like the casts. Casts in general break the type checking of the compiler. In some places e.g. clc.d0 points to struct smc_clc_msg_accept_confirm in other places it points to struct smc_clc_msg_accept_confirm_v2. This makes it hard to find all places where e.g. d0 is altered. (e.g. with an IDE). You say: "smc_clc_msg_accept_confirm > and smc_clc_msg_accept_confirm_v2 clearly shows the difference between v1 and > v2 messages" But that is not even the case in the code that this patch changes: In smcd_clc_prep_confirm_accept() you pass a struct smc_clc_msg_accept_confirm_v2 cast it to v1 (even in the v2 case) and then use the v1 layout for the common fields and the v1-only fields. So I don't think that helps very much. The v2 messages were explicitely defined for compatibility. i.e. all v1 fields are still available. It would be good to see that in the code as well. With 2 differnet structs you don't emphasize that. With future changes somebody could easily make a mistake that the 2 structures don't have the same size anymore. And then the casting can lead to out-of-bound error that are hard to find. We want v2 to be the usual case and v1 to be the exception for backwards compatibility. FOr historic reasons, the code looks as if v2 is the exception. I'd rather point out the remaining v1 cases. I could envision something like: 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]; __be64 gid_ext; } __packed d1; }; }; And then only use this one structure.
On 2023/12/11 21:35, Alexandra Winter wrote: > > > On 11.12.23 13:15, Wen Gu wrote: >>>> + clc = (struct smc_clc_msg_accept_confirm *)clc_v2; >>> >>> Why is this cast neccessary? (Here as well as in smcr_clc_prep_confirm_accept >>> and in smc_clc_send_confirm_accept) >>> smc_clc_msg_accept_confirm_v2 has hdr and d0 as well. >> >> I think the cast is to imply that v2 is an expansion of v1, or v1 is the base of v2. >> So here using clc(v1) reperesents their common set. >> >> If we use smc_clc_msg_accept_confirm_v2 for all, I think readers may be tempted to >> check whether the hdr and d0 in 'smc_clc_msg_accept_confirm_v2' are also applicable to v1. >> >> And there are settings below that are specific for v1. It may be confusing if we >> change it like this: >> >> if (version == SMC_V1) { >> clc_v2->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN); >> } else { >> >> >>> >>> IMO, it would be a nice seperate patch to get rid of the 2 type defs for >>> smc_clc_msg_accept_confirm and smc_clc_msg_accept_confirm_v2 >>> and all the related casting anyhow. >>> >> >> Do you mean to define only smc_clc_msg_accept_confirm_v2 or define with the name >> of smc_clc_msg_accept_confirm but the contents of smc_clc_msg_accept_confirm_v2? >> >> I have a different opinion on this, since I think the smc_clc_msg_accept_confirm >> and smc_clc_msg_accept_confirm_v2 clearly shows the difference between v1 and >> v2 messages and remind people what is currently working on. So I perfer to keep them. >> Am I missing something? >> > > > This is a discussion about coding style, readability and maintainability (avoid future errors). > And the code works today and the rest is opinions. That said, let me list some arguments why > I don't like the casts. > > Casts in general break the type checking of the compiler. > > In some places e.g. clc.d0 points to struct smc_clc_msg_accept_confirm in other > places it points to struct smc_clc_msg_accept_confirm_v2. > This makes it hard to find all places where e.g. d0 is altered. (e.g. with an IDE). > > You say: "smc_clc_msg_accept_confirm >> and smc_clc_msg_accept_confirm_v2 clearly shows the difference between v1 and >> v2 messages" > But that is not even the case in the code that this patch changes: > In smcd_clc_prep_confirm_accept() you pass a struct smc_clc_msg_accept_confirm_v2 > cast it to v1 (even in the v2 case) and then use the v1 layout for the common fields and > the v1-only fields. So I don't think that helps very much. > > The v2 messages were explicitely defined for compatibility. i.e. > all v1 fields are still available. It would be good to see that in the code as well. > With 2 differnet structs you don't emphasize that. > > With future changes somebody could easily make a mistake that the 2 structures don't > have the same size anymore. And then the casting can lead to out-of-bound error that > are hard to find. > > We want v2 to be the usual case and v1 to be the exception for backwards compatibility. > FOr historic reasons, the code looks as if v2 is the exception. I'd rather point out the > remaining v1 cases. > > > > I could envision something like: > > 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]; > __be64 gid_ext; > } __packed d1; > }; > }; > > And then only use this one structure. > Thank you Sandy for the detailed explanation. What I considered, as mentioned above, is that if the two are combined, it may be difficult to distinguish according to the name what situation I am in, v1 or v2? But I do agree with your concern about the potential errors that caused by future divergence of the two struct if they are defined separately. I will try to combine them into one struct in a seperate patch. Thank you.
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c index 0fcb035..52b4ea9 100644 --- a/net/smc/smc_clc.c +++ b/net/smc/smc_clc.c @@ -998,6 +998,111 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) return reason_code; } +static void smcd_clc_prep_confirm_accept(struct smc_connection *conn, + struct smc_clc_msg_accept_confirm_v2 *clc_v2, + int first_contact, u8 version, + u8 *eid, struct smc_init_info *ini, + int *fce_len, + struct smc_clc_first_contact_ext_v2x *fce_v2x, + 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; + clc->d0.gid = htonll(smcd->ops->get_local_gid(smcd)); + clc->d0.token = htonll(conn->rmb_desc->token); + clc->d0.dmbe_size = conn->rmbe_size_comp; + clc->d0.dmbe_idx = 0; + memcpy(&clc->d0.linkid, conn->lgr->id, SMC_LGR_ID_SIZE); + if (version == SMC_V1) { + clc->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN); + } else { + clc_v2->d1.chid = htons(smc_ism_get_chid(smcd)); + if (eid && eid[0]) + memcpy(clc_v2->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); + } + memcpy(trl->eyecatcher, SMCD_EYECATCHER, + sizeof(SMCD_EYECATCHER)); +} + +static void smcr_clc_prep_confirm_accept(struct smc_connection *conn, + struct smc_clc_msg_accept_confirm_v2 *clc_v2, + int first_contact, u8 version, + u8 *eid, struct smc_init_info *ini, + int *fce_len, + struct smc_clc_first_contact_ext_v2x *fce_v2x, + 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; + clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN); + memcpy(clc->r0.lcl.id_for_peer, local_systemid, + sizeof(local_systemid)); + memcpy(&clc->r0.lcl.gid, link->gid, SMC_GID_SIZE); + memcpy(&clc->r0.lcl.mac, &link->smcibdev->mac[link->ibport - 1], + ETH_ALEN); + hton24(clc->r0.qpn, link->roce_qp->qp_num); + clc->r0.rmb_rkey = + htonl(conn->rmb_desc->mr[link->link_idx]->rkey); + clc->r0.rmbe_idx = 1; /* for now: 1 RMB = 1 RMBE */ + clc->r0.rmbe_alert_token = htonl(conn->alert_token_local); + switch (clc->hdr.type) { + case SMC_CLC_ACCEPT: + clc->r0.qp_mtu = link->path_mtu; + break; + case SMC_CLC_CONFIRM: + clc->r0.qp_mtu = min(link->path_mtu, link->peer_mtu); + break; + } + clc->r0.rmbe_size = conn->rmbe_size_comp; + clc->r0.rmb_dma_addr = conn->rmb_desc->is_vm ? + cpu_to_be64((uintptr_t)conn->rmb_desc->cpu_addr) : + cpu_to_be64((u64)sg_dma_address + (conn->rmb_desc->sgt[link->link_idx].sgl)); + hton24(clc->r0.psn, link->psn_initial); + if (version == SMC_V1) { + clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN); + } else { + if (eid && eid[0]) + memcpy(clc_v2->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); + len += *fce_len; + fce_v2x->fce_v2_base.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]); + } + } + clc_v2->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, @@ -1006,11 +1111,10 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc, { struct smc_clc_first_contact_ext_v2x fce_v2x; struct smc_connection *conn = &smc->conn; - struct smcd_dev *smcd = conn->lgr->smcd; struct smc_clc_msg_accept_confirm *clc; struct smc_clc_fce_gid_ext gle; struct smc_clc_msg_trail trl; - int i, len, fce_len; + int i, fce_len; struct kvec vec[5]; struct msghdr msg; @@ -1019,86 +1123,14 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc, clc->hdr.version = version; /* SMC version */ if (first_contact) clc->hdr.typev2 |= SMC_FIRST_CONTACT_MASK; - if (conn->lgr->is_smcd) { - /* SMC-D specific settings */ - memcpy(clc->hdr.eyecatcher, SMCD_EYECATCHER, - sizeof(SMCD_EYECATCHER)); - clc->hdr.typev1 = SMC_TYPE_D; - clc->d0.gid = htonll(smcd->ops->get_local_gid(smcd)); - clc->d0.token = htonll(conn->rmb_desc->token); - clc->d0.dmbe_size = conn->rmbe_size_comp; - clc->d0.dmbe_idx = 0; - memcpy(&clc->d0.linkid, conn->lgr->id, SMC_LGR_ID_SIZE); - if (version == SMC_V1) { - clc->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN); - } else { - clc_v2->d1.chid = htons(smc_ism_get_chid(smcd)); - if (eid && eid[0]) - memcpy(clc_v2->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); - } - memcpy(trl.eyecatcher, SMCD_EYECATCHER, - sizeof(SMCD_EYECATCHER)); - } else { - struct smc_link *link = conn->lnk; - - /* SMC-R specific settings */ - memcpy(clc->hdr.eyecatcher, SMC_EYECATCHER, - sizeof(SMC_EYECATCHER)); - clc->hdr.typev1 = SMC_TYPE_R; - clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN); - memcpy(clc->r0.lcl.id_for_peer, local_systemid, - sizeof(local_systemid)); - memcpy(&clc->r0.lcl.gid, link->gid, SMC_GID_SIZE); - memcpy(&clc->r0.lcl.mac, &link->smcibdev->mac[link->ibport - 1], - ETH_ALEN); - hton24(clc->r0.qpn, link->roce_qp->qp_num); - clc->r0.rmb_rkey = - htonl(conn->rmb_desc->mr[link->link_idx]->rkey); - clc->r0.rmbe_idx = 1; /* for now: 1 RMB = 1 RMBE */ - clc->r0.rmbe_alert_token = htonl(conn->alert_token_local); - switch (clc->hdr.type) { - case SMC_CLC_ACCEPT: - clc->r0.qp_mtu = link->path_mtu; - break; - case SMC_CLC_CONFIRM: - clc->r0.qp_mtu = min(link->path_mtu, link->peer_mtu); - break; - } - clc->r0.rmbe_size = conn->rmbe_size_comp; - clc->r0.rmb_dma_addr = conn->rmb_desc->is_vm ? - cpu_to_be64((uintptr_t)conn->rmb_desc->cpu_addr) : - cpu_to_be64((u64)sg_dma_address - (conn->rmb_desc->sgt[link->link_idx].sgl)); - hton24(clc->r0.psn, link->psn_initial); - if (version == SMC_V1) { - clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN); - } else { - if (eid && eid[0]) - memcpy(clc_v2->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); - len += fce_len; - fce_v2x.fce_v2_base.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]); - } - } - clc_v2->hdr.length = htons(len); - } - memcpy(trl.eyecatcher, SMC_EYECATCHER, sizeof(SMC_EYECATCHER)); - } - + if (conn->lgr->is_smcd) + smcd_clc_prep_confirm_accept(conn, clc_v2, first_contact, + version, eid, ini, &fce_len, + &fce_v2x, &trl); + else + smcr_clc_prep_confirm_accept(conn, clc_v2, first_contact, + version, eid, ini, &fce_len, + &fce_v2x, &gle, &trl); memset(&msg, 0, sizeof(msg)); i = 0; vec[i].iov_base = clc_v2;
There is a large if-else block in smc_clc_send_confirm_accept() and it is better to split it into two sub-functions. Suggested-by: Alexandra Winter <wintera@linux.ibm.com> Signed-off-by: Wen Gu <guwen@linux.alibaba.com> --- net/smc/smc_clc.c | 196 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 114 insertions(+), 82 deletions(-)