From: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
Date: Fri, 12 May 2023 17:25:57 +0200
> In order to use XDP hints via kfuncs we need to put
> RX descriptor and ring pointers just next to xdp_buff.
> Same as in hints implementations in other drivers, we archieve
> this through putting xdp_buff into a child structure.
>
> Currently, xdp_buff is stored in the ring structure,
> so replace it with union that includes child structure.
> This way enough memory is available while existing XDP code
> remains isolated from hints.
>
> Size of the new child structure (ice_xdp_buff) is 72 bytes,
> therefore it does not fit into a single cache line.
> To at least place union at the start of cache line, move 'next'
> field from CL3 to CL1, as it isn't used often.
>
> Placing union at the start of cache line makes at least xdp_buff
> and descriptor fit into a single CL,
> ring pointer is used less often, so it can spill into the next CL.
Spill or span?
>
> Signed-off-by: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
> ---
> drivers/net/ethernet/intel/ice/ice_txrx.c | 7 ++++--
> drivers/net/ethernet/intel/ice/ice_txrx.h | 23 ++++++++++++++++---
> drivers/net/ethernet/intel/ice/ice_txrx_lib.h | 11 +++++++++
> 3 files changed, 36 insertions(+), 5 deletions(-)
[...]
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> @@ -260,6 +260,15 @@ enum ice_rx_dtype {
> ICE_RX_DTYPE_SPLIT_ALWAYS = 2,
> };
>
> +struct ice_xdp_buff {
> + struct xdp_buff xdp_buff;
> + union ice_32b_rx_flex_desc *eop_desc; /* Required for all metadata */
Probably can be const here as well after changing all the places
appropriately -- I don't think you write to it anywhere.
> + /* End of the 1st cache line */
> + struct ice_rx_ring *rx_ring;
Can't we get rid of ring dependency? Maybe there's only a couple fields
that could be copied here instead of referencing the ring? I just find
it weird that our drivers often look for something in the ring structure
to parse a descriptor ._.
If not, can't it be const?
> +};
> +
> +static_assert(offsetof(struct ice_xdp_buff, xdp_buff) == 0);
> +
> /* indices into GLINT_ITR registers */
> #define ICE_RX_ITR ICE_IDX_ITR0
> #define ICE_TX_ITR ICE_IDX_ITR1
> @@ -301,7 +310,6 @@ enum ice_dynamic_itr {
> /* descriptor ring, associated with a VSI */
> struct ice_rx_ring {
> /* CL1 - 1st cacheline starts here */
> - struct ice_rx_ring *next; /* pointer to next ring in q_vector */
> void *desc; /* Descriptor ring memory */
> struct device *dev; /* Used for DMA mapping */
> struct net_device *netdev; /* netdev ring maps to */
> @@ -313,12 +321,19 @@ struct ice_rx_ring {
> u16 count; /* Number of descriptors */
> u16 reg_idx; /* HW register index of the ring */
> u16 next_to_alloc;
> - /* CL2 - 2nd cacheline starts here */
> +
> union {
> struct ice_rx_buf *rx_buf;
> struct xdp_buff **xdp_buf;
> };
> - struct xdp_buff xdp;
> + /* CL2 - 2nd cacheline starts here
> + * Size of ice_xdp_buff is 72 bytes,
> + * so it spills into CL3
> + */
> + union {
> + struct ice_xdp_buff xdp_ext;
> + struct xdp_buff xdp;
> + };
...or you can leave just one xdp_ext (naming it just "xdp") -- for now,
this union does literally nothing, as xdp_ext contains xdp at its very
beginning.
> /* CL3 - 3rd cacheline starts here */
> struct bpf_prog *xdp_prog;
> u16 rx_offset;
> @@ -328,6 +343,8 @@ struct ice_rx_ring {
> u16 next_to_clean;
> u16 first_desc;
>
> + struct ice_rx_ring *next; /* pointer to next ring in q_vector */
It can be placed even farther, somewhere near rcu_head -- IIRC it's not
used anywhere on hotpath. Even ::ring_stats below is hotter.
> +
> /* stats structs */
> struct ice_ring_stats *ring_stats;
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h
> index e1d49e1235b3..2835a8348237 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h
> @@ -151,4 +151,15 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring,
> struct sk_buff *skb);
> void
> ice_receive_skb(struct ice_rx_ring *rx_ring, struct sk_buff *skb, u16 vlan_tag);
> +
> +static inline void
> +ice_xdp_set_meta_srcs(struct xdp_buff *xdp,
Not sure about the naming... But can't propose anything :clownface:
ice_xdp_init_buff()? Like xdp_init_buff(), but ice_xdp_buff :D
> + union ice_32b_rx_flex_desc *eop_desc,
> + struct ice_rx_ring *rx_ring)
> +{
> + struct ice_xdp_buff *xdp_ext = (struct ice_xdp_buff *)xdp;
I'd use container_of(), even though it will do the same thing here.
BTW, is having &xdp_buff at offset 0 still a requirement?
> +
> + xdp_ext->eop_desc = eop_desc;
> + xdp_ext->rx_ring = rx_ring;
> +}
> #endif /* !_ICE_TXRX_LIB_H_ */
Thanks,
Olek
