Message-ID: <IA3PR11MB89865A81EE4A64B712AAC280E5C4A@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Tue, 4 Nov 2025 07:23:17 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>, "Lobakin, Aleksander"
	<aleksander.lobakin@...el.com>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH iwl-next 2/9] ice: use cacheline groups for ice_rx_ring
 structure



> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@...el.com>
> Sent: Tuesday, November 4, 2025 2:07 AM
> To: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>; Lobakin,
> Aleksander <aleksander.lobakin@...el.com>; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Keller,
> Jacob E <jacob.e.keller@...el.com>
> Subject: [PATCH iwl-next 2/9] ice: use cacheline groups for
> ice_rx_ring structure
> 
> The ice ring structure was reorganized back by commit 65124bbf980c
> ("ice:
> Reorganize tx_buf and ring structs"), and later split into a separate
> ice_rx_ring structure by commit e72bba21355d ("ice: split ice_ring
> onto Tx/Rx separate structs")
> 
> The ice_rx_ring structure has comments left over from this prior
> reorganization indicating which fields belong to which cachelines.
> Unfortunately, these comments are not all accurate. The intended
> layout is for x86_64 systems with a 64-byte cache.
> 
>  * Cacheline 1 spans from the start of the struct to the end of the
> rx_fqes
>    and xdp_buf union. The comments correctly match this.
> 
>  * Cacheline 2 spans from hdr_fqes to the end of hdr_truesize, but the
>    comment indicates it should end xdp and xsk union.
> 
>  * Cacheline 3 spans from the truesize field to the xsk_pool, but the
>    comment wants this to be from the pkt_ctx down to the rcu head
> field.
> 
>  * Cacheline 4 spans from the rx_hdr_len down to the flags field, but
> the
>    comment indicates that it starts back at the ice_channel structure
>    pointer.
> 
>  * Cacheline 5 is indicated to cover the xdp_rxq. Because this field
> is
>    aligned to 64 bytes, this is actually true. However, there is a
> large 45
>    byte gap at the end of cacheline 4.
> 
> The use of comments to indicate cachelines is poor design. In
> practice, comments like this quickly become outdated as developers add
> or remove fields, or as other sub-structures change layout and size
> unexpectedly.
> 
> The ice_rx_ring structure *is* 5 cachelines (320 bytes), but ends up
> having quite a lot of empty space at the end just before xdp_rxq.
> 
> Replace the comments with __cacheline_group_(begin|end)_aligned()
> annotations. These macros enforce alignment to the start of
> cachelines, and enforce padding between groups, thus guaranteeing that
> a following group cannot be part of the same cacheline.
> 
> Doing this changes the layout by effectively spreading the padding at
> the tail of cacheline 4 between groups to ensure that the relevant
> fields are kept as separate cachelines on x86_64 systems. For systems
> with the expected cache line size of 64 bytes, the structure size does
> not change.
> For systems with a larger SMB_CACHE_BYTES size, the structure *will*
> increase in size a fair bit, however we'll now guarantee that each
> group is in a separate cacheline. This has an advantage that updates
> to fields in one group won't trigger cacheline eviction of the other
> groups. This comes at the expense of extra memory footprint for the
> rings.
> 
> If fields get re-arranged, added, or removed, the alignment and
> padding ensure the relevant fields are kept on separate cache lines.
> This could result in unexpected changes in the structure size due to
> padding to keep cachelines separate.
> 
> To catch such changes during early development, add build time
> compiler assertions that check the size of each group to ensure it
> doesn't exceed 64 bytes, the expected cache line size. The assertion
> checks that the size of the group excluding any padding at the end is
> less than the provided size of 64 bytes. This type of check will
> behave the same even on architectures with larger cache sizes. The
> primary aim is to produce a warning if developers add fields into a
> cacheline group which exceeds the size of the expected 64 byte
> groupings.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_txrx.h | 26 +++++++++++++++++++++-
> ----  drivers/net/ethernet/intel/ice/ice_main.c |  2 ++
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h
> b/drivers/net/ethernet/intel/ice/ice_txrx.h
> index e440c55d9e9f..6c708caf3a91 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> @@ -236,7 +236,7 @@ struct ice_tstamp_ring {  }
> ____cacheline_internodealigned_in_smp;
> 
>  struct ice_rx_ring {
> -	/* CL1 - 1st cacheline starts here */
> +	__cacheline_group_begin_aligned(cl1);
>  	void *desc;			/* Descriptor ring memory */
>  	struct page_pool *pp;
>  	struct net_device *netdev;	/* netdev ring maps to */
> @@ -253,8 +253,9 @@ struct ice_rx_ring {
>  		struct libeth_fqe *rx_fqes;
>  		struct xdp_buff **xdp_buf;
>  	};
> +	__cacheline_group_end_aligned(cl1);
> 
> -	/* CL2 - 2nd cacheline starts here */
> +	__cacheline_group_begin_aligned(cl2);
>  	struct libeth_fqe *hdr_fqes;
>  	struct page_pool *hdr_pp;
> 
> @@ -262,8 +263,9 @@ struct ice_rx_ring {
>  		struct libeth_xdp_buff_stash xdp;
>  		struct libeth_xdp_buff *xsk;
>  	};
> +	__cacheline_group_end_aligned(cl2);
> 
> -	/* CL3 - 3rd cacheline starts here */
> +	__cacheline_group_begin_aligned(cl3);
>  	union {
>  		struct ice_pkt_ctx pkt_ctx;
>  		struct {
> @@ -284,7 +286,9 @@ struct ice_rx_ring {
>  	struct ice_ring_stats *ring_stats;
> 
>  	struct rcu_head rcu;		/* to avoid race on free */
> -	/* CL4 - 4th cacheline starts here */
> +	__cacheline_group_end_aligned(cl3);
> +
> +	__cacheline_group_begin_aligned(cl4);
>  	struct ice_channel *ch;
>  	struct ice_tx_ring *xdp_ring;
>  	struct ice_rx_ring *next;	/* pointer to next ring in q_vector
> */
> @@ -298,10 +302,22 @@ struct ice_rx_ring {
>  #define ICE_RX_FLAGS_MULTIDEV		BIT(3)
>  #define ICE_RX_FLAGS_RING_GCS		BIT(4)
>  	u8 flags;
> -	/* CL5 - 5th cacheline starts here */
> +	__cacheline_group_end_aligned(cl4);
> +
> +	__cacheline_group_begin_aligned(cl5);
>  	struct xdp_rxq_info xdp_rxq;
> +	__cacheline_group_end_aligned(cl5);
>  } ____cacheline_internodealigned_in_smp;
> 
> +static inline void ice_rx_ring_struct_check(void) {
> +	CACHELINE_ASSERT_GROUP_SIZE(struct ice_rx_ring, cl1, 64);
> +	CACHELINE_ASSERT_GROUP_SIZE(struct ice_rx_ring, cl2, 64);
> +	CACHELINE_ASSERT_GROUP_SIZE(struct ice_rx_ring, cl3, 64);
> +	CACHELINE_ASSERT_GROUP_SIZE(struct ice_rx_ring, cl4, 64);
> +	CACHELINE_ASSERT_GROUP_SIZE(struct ice_rx_ring, cl5, 64); }
> +
>  struct ice_tx_ring {
>  	/* CL1 - 1st cacheline starts here */
>  	struct ice_tx_ring *next;	/* pointer to next ring in q_vector
> */
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
> b/drivers/net/ethernet/intel/ice/ice_main.c
> index b16ede1f139d..4731dbaca9de 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -5916,6 +5916,8 @@ static int __init ice_module_init(void)  {
>  	int status = -ENOMEM;
> 
> +	ice_rx_ring_struct_check();
> +
>  	pr_info("%s\n", ice_driver_string);
>  	pr_info("%s\n", ice_copyright);
> 
> 
> --
> 2.51.0.rc1.197.g6d975e95c9d7

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>

Powered by blists - more mailing lists