> -----Original Message----- > From: Keller, Jacob E <[email protected]> > Sent: Tuesday, November 4, 2025 2:07 AM > To: Loktionov, Aleksandr <[email protected]>; Lobakin, > Aleksander <[email protected]>; Nguyen, Anthony L > <[email protected]>; Kitszel, Przemyslaw > <[email protected]> > Cc: [email protected]; [email protected]; Keller, > Jacob E <[email protected]> > 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 <[email protected]> > --- > 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 <[email protected]>
