lkml.org 
[lkml]   [2023]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2
From


On 2023/12/18 16:39, Alexandra Winter wrote:
>
>
> 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.

Thank you very much for the suggestions, Sandy.

I checked the history commits:
c758dfddc1b5 ("net/smc: add SMC-D support in CLC messages")
3d9725a6a133 ("net/smc: common routine for CLC accept and confirm")
a7c9c5f4af7f ("net/smc: CLC accept / confirm V2")
e5c4744cfb59 ("net/smc: add SMC-Rv2 connection establishment")

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.

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?

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: */
- 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: */
- __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
};

>
>> };
>
> 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.

Thanks.
>
> [...]

\
 
 \ /
  Last update: 2023-12-18 13:22    [W:0.039 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site