Re: [net PATCH] octeontx2-af: Fix marking couple of structure as __packed

[Jacob Keller <jacob.e.keller@intel.com>]

On 12/18/2023 12:27 AM, Suman Ghosh wrote:
> Couple of structures was not marked as __packed which may have some
> performance implication. This patch fixes the same and mark them as
> __packed.

Not sure I follow why lack of __packed would have performance
implications? I get that __packed is important to ensure layout is
correct or to ensure the whole structure has the right size rather than
unexpected gaps. I'd guess maybe because the structures size would
include padding without __packed, leading to a lot of gaps when
combining several structures together...

I did test on my system with pahole, and even without __packed, I don't
get any gaps in the npc_lt_def_cfg structure:


> struct npc_lt_def_cfg {
>         struct npc_lt_def          rx_ol2;               /*     0     3 */
>         struct npc_lt_def          rx_oip4;              /*     3     3 */
>         struct npc_lt_def          rx_iip4;              /*     6     3 */
>         struct npc_lt_def          rx_oip6;              /*     9     3 */
>         struct npc_lt_def          rx_iip6;              /*    12     3 */
>         struct npc_lt_def          rx_otcp;              /*    15     3 */
>         struct npc_lt_def          rx_itcp;              /*    18     3 */
>         struct npc_lt_def          rx_oudp;              /*    21     3 */
>         struct npc_lt_def          rx_iudp;              /*    24     3 */
>         struct npc_lt_def          rx_osctp;             /*    27     3 */
>         struct npc_lt_def          rx_isctp;             /*    30     3 */
>         struct npc_lt_def_ipsec    rx_ipsec[2];          /*    33    10 */
>         struct npc_lt_def          pck_ol2;              /*    43     3 */
>         struct npc_lt_def          pck_oip4;             /*    46     3 */
>         struct npc_lt_def          pck_oip6;             /*    49     3 */
>         struct npc_lt_def          pck_iip4;             /*    52     3 */
>         struct npc_lt_def_apad     rx_apad0;             /*    55     4 */
>         struct npc_lt_def_apad     rx_apad1;             /*    59     4 */
>         struct npc_lt_def_color    ovlan;                /*    63     5 */
>         /* --- cacheline 1 boundary (64 bytes) was 4 bytes ago --- */
>         struct npc_lt_def_color    ivlan;                /*    68     5 */
>         struct npc_lt_def_color    rx_gen0_color;        /*    73     5 */
>         struct npc_lt_def_color    rx_gen1_color;        /*    78     5 */
>         struct npc_lt_def_et       rx_et[2];             /*    83    10 */
> 
>         /* size: 93, cachelines: 2, members: 23 */
>         /* last cacheline: 29 bytes */
> };


However that may not be true across all compilers etc. Also all the
other structures are __packed. Makes sense.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> 
> Fixes: 42006910b5ea ("octeontx2-af: cleanup KPU config data")
> Signed-off-by: Suman Ghosh <sumang@marvell.com>
> ---
>  drivers/net/ethernet/marvell/octeontx2/af/npc.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/npc.h b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> index ab3e39eef2eb..8c0732c9a7ee 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> @@ -528,7 +528,7 @@ struct npc_lt_def {
>  	u8	ltype_mask;
>  	u8	ltype_match;
>  	u8	lid;
> -};
> +} __packed;
>  
>  struct npc_lt_def_ipsec {
>  	u8	ltype_mask;
> @@ -536,7 +536,7 @@ struct npc_lt_def_ipsec {
>  	u8	lid;
>  	u8	spi_offset;
>  	u8	spi_nz;
> -};
> +} __packed;
>  
>  struct npc_lt_def_apad {
>  	u8	ltype_mask;