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

[Wen Gu <guwen@linux.alibaba.com>]

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