Re: [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2

[Alexandra Winter]

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.