| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ac3c0823-8705-4225-96c8-ed7bc55d1bfc@linux.ibm.com>
Date: Mon, 11 Dec 2023 11:43:22 +0100
From: Alexandra Winter <wintera@...ux.ibm.com>
To: Wen Gu <guwen@...ux.alibaba.com>, wenjia@...ux.ibm.com,
hca@...ux.ibm.com, gor@...ux.ibm.com, agordeev@...ux.ibm.com,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, kgraul@...ux.ibm.com, jaka@...ux.ibm.com
Cc: borntraeger@...ux.ibm.com, svens@...ux.ibm.com,
alibuda@...ux.alibaba.com, tonylu@...ux.alibaba.com,
raspl@...ux.ibm.com, schnelle@...ux.ibm.com,
guangguan.wang@...ux.alibaba.com, linux-s390@...r.kernel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v5 2/9] net/smc: introduce sub-functions for
smc_clc_send_confirm_accept()
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@...ux.ibm.com>
> Signed-off-by: Wen Gu <guwen@...ux.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;
Powered by blists - more mailing lists