[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c9d908e3-5147-4c54-a2de-ef9254ac5c4f@linux.ibm.com>
Date: Mon, 18 Dec 2023 18:40:11 +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 v6 03/10] net/smc: unify the structs of accept or
confirm message for v1 and v2
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.
Powered by blists - more mailing lists