Received-SPF: pass (google.com: domain of linux-kernel+bounces-3172-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <63aa2995-7980-430d-84be-58ce204f5172@linux.ibm.com>
Date: Mon, 18 Dec 2023 09:39:10 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v6 03/10] net/smc: unify the structs of accept or
 confirm message for v1 and v2
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: <1702371151-125258-1-git-send-email-guwen@linux.alibaba.com>
 <1702371151-125258-4-git-send-email-guwen@linux.alibaba.com>
Content-Language: en-US
From: Alexandra Winter <wintera@linux.ibm.com>
In-Reply-To: <1702371151-125258-4-git-send-email-guwen@linux.alibaba.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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.


[...]