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

Wen Gu
2023-12-20T12:17:04Z

On 2023/12/20 19:37, Alexandra Winter wrote:
>
>
> On 19.12.23 15:26, Wen Gu wrote:
>> 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;
>> - u8 eid[SMC_MAX_EID_LEN];
>> - u8 reserved6[8];
>> - } r1;
>> + struct { /* v2 only */
>> + u8 eid[SMC_MAX_EID_LEN];
>> + u8 reserved6[8];
>> + } __packed r1;
>> + };
>> struct { /* SMC-D */
>> struct smcd_clc_msg_accept_confirm_common d0;
>> - __be16 chid;
>> - u8 eid[SMC_MAX_EID_LEN];
>> - u8 reserved5[8];
>> - } d1;
>> + struct { /* v2 only, but 12 bytes reserved in v1 */
>> + __be16 chid;
>> + u8 eid[SMC_MAX_EID_LEN];
>> + u8 reserved5[8];
>> + } __packed d1;
>> + };
>> };
>> };
>
>
> I still think the __packed at the outmost level is the safest place.
> Like you have it now the compiler could place unused memory between
> ro and r1 or between d0 and d1.
> Afaik compilers don't do that, if the blocks are word-aligned, but
> there is no guarantee.
>
> Up to you. My R-b still applies.
> Sandy

Thank you, Sandy.

IIUC, if only outmost level has __packed, it won't work for the inner block.

e.g.

If __packed is added at d1 and r1:

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;
struct { /* v2 only */
u8 eid[SMC_MAX_EID_LEN];
u8 reserved6[8];
} __packed r1;
};
struct { /* SMC-D */
struct smcd_clc_msg_accept_confirm_common d0;
struct { /* v2 only, but 12 bytes reserved in v1 */
__be16 chid;
u8 eid[SMC_MAX_EID_LEN];
u64 gid_ext;
} __packed d1;
};
};
};

According to pahole, it will be:

struct smc_clc_msg_accept_confirm {
struct smc_clc_msg_hdr hdr; /* 0 8 */
union {
struct {
struct smcr_clc_msg_accept_confirm r0; /* 8 56 */
/* --- cacheline 1 boundary (64 bytes) --- */
struct {
u8 eid[32]; /* 64 32 */
u8 reserved6[8]; /* 96 8 */
} r1; /* 64 40 */
}; /* 8 96 */
struct {
struct smcd_clc_msg_accept_confirm_common d0; /* 8 24 */
struct {
__be16 chid; /* 32 2 */
u8 eid[32]; /* 34 32 */
/* --- cacheline 1 boundary (64 bytes) was 2 bytes ago --- */
u64 gid_ext; /* 66 8 */
} __attribute__((__packed__)) d1; /* 32 42 */
} __attribute__((__packed__)); /* 8 66 */
}; /* 8 96 */

/* size: 104, cachelines: 2, members: 2 */
/* last cacheline: 40 bytes */
};

If __packed is added at outmost level:

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;
struct { /* v2 only */
u8 eid[SMC_MAX_EID_LEN];
u8 reserved6[8];
} r1;
};
struct { /* SMC-D */
struct smcd_clc_msg_accept_confirm_common d0;
struct { /* v2 only, but 12 bytes reserved in v1 */
__be16 chid;
u8 eid[SMC_MAX_EID_LEN];
u64 gid_ext;
} d1;
};
};
} __packed;

According to pahole, it will be:

struct smc_clc_msg_accept_confirm {
struct smc_clc_msg_hdr hdr; /* 0 8 */
union {
struct {
struct smcr_clc_msg_accept_confirm r0; /* 8 56 */
/* --- cacheline 1 boundary (64 bytes) --- */
struct {
u8 eid[32]; /* 64 32 */
u8 reserved6[8]; /* 96 8 */
} r1; /* 64 40 */
}; /* 8 96 */
struct {
struct smcd_clc_msg_accept_confirm_common d0; /* 8 24 */
struct {
__be16 chid; /* 32 2 */
u8 eid[32]; /* 34 32 */

/* XXX 6 bytes hole, try to pack */

/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
u64 gid_ext; /* 72 8 */
} d1; /* 32 48 */ <- doesn't work for inner d1.
}; /* 8 72 */
}; /* 8 96 */

/* size: 104, cachelines: 2, members: 2 */
/* last cacheline: 40 bytes */
};

I also considered add them all:

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;
struct { /* v2 only */
u8 eid[SMC_MAX_EID_LEN];
u8 reserved6[8];
} __packed r1;
} __packed;
struct { /* SMC-D */
struct smcd_clc_msg_accept_confirm_common d0;
struct { /* v2 only, but 12 bytes reserved in v1 */
__be16 chid;
u8 eid[SMC_MAX_EID_LEN];
u64 gid_ext;
} __packed d1;
} __packed;
};
} __packed;

but a little bit strange since for only d1 needs to packed, so I kept it as it is now.

Thanks,
Wen Gu