Date: Mon, 22 May 2023 18:46:40 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Larysa Zaremba <larysa.zaremba@...el.com>
CC: <bpf@...r.kernel.org>, Stanislav Fomichev <sdf@...gle.com>, "Alexei
 Starovoitov" <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Andrii
 Nakryiko <andrii@...nel.org>, Jakub Kicinski <kuba@...nel.org>, Martin KaFai
 Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>, Yonghong Song
	<yhs@...com>, John Fastabend <john.fastabend@...il.com>, "KP Singh"
	<kpsingh@...nel.org>, Jiri Olsa <jolsa@...nel.org>, Jesse Brandeburg
	<jesse.brandeburg@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
	Anatoly Burakov <anatoly.burakov@...el.com>, Jesper Dangaard Brouer
	<brouer@...hat.com>, Alexander Lobakin <alexandr.lobakin@...el.com>, "Magnus
 Karlsson" <magnus.karlsson@...il.com>, Maryam Tahhan <mtahhan@...hat.com>,
	<xdp-hints@...-project.net>, <netdev@...r.kernel.org>,
	<intel-wired-lan@...ts.osuosl.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND bpf-next 05/15] ice: Introduce ice_xdp_buff

From: Larysa Zaremba <larysa.zaremba@...el.com>
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@...el.com>
> ---
>  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

Powered by blists - more mailing lists