Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <ac3c0823-8705-4225-96c8-ed7bc55d1bfc@linux.ibm.com>
Date:   Mon, 11 Dec 2023 11:43:22 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v5 2/9] net/smc: introduce sub-functions for
 smc_clc_send_confirm_accept()
Content-Language: en-US
To:     Wen Gu <guwen@linux.alibaba.com>, wenjia@linux.ibm.com,
        hca@linux.ibm.com, gor@linux.ibm.com, agordeev@linux.ibm.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, kgraul@linux.ibm.com, jaka@linux.ibm.com
Cc:     borntraeger@linux.ibm.com, svens@linux.ibm.com,
        alibuda@linux.alibaba.com, tonylu@linux.alibaba.com,
        raspl@linux.ibm.com, schnelle@linux.ibm.com,
        guangguan.wang@linux.alibaba.com, linux-s390@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <1702021259-41504-1-git-send-email-guwen@linux.alibaba.com>
 <1702021259-41504-3-git-send-email-guwen@linux.alibaba.com>
From:   Alexandra Winter <wintera@linux.ibm.com>
In-Reply-To: <1702021259-41504-3-git-send-email-guwen@linux.alibaba.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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;