Re: [RFT] net: ethernet: enetc: do not always access skb_shared_info in the XDP path

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4307 bytes --]

> Hi Lorenzo,
> 
> On Tue, Dec 13, 2022 at 10:46:43AM +0100, Lorenzo Bianconi wrote:
> > Move XDP skb_shared_info structure initialization in from
> > enetc_map_rx_buff_to_xdp() to enetc_add_rx_buff_to_xdp() and do not always
> > access skb_shared_info in the xdp_buff/xdp_frame since it is located in a
> > different cacheline with respect to hard_start and data xdp pointers.
> > Rely on XDP_FLAGS_HAS_FRAGS flag to check if it really necessary to access
> > non-linear part of the xdp_buff/xdp_frame.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> > This patch is based on the following series not applied yet to next-next:
> > https://patchwork.kernel.org/project/netdevbpf/cover/cover.1670680119.git.lorenzo@kernel.org/
> > ---
> >  drivers/net/ethernet/freescale/enetc/enetc.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index cd8f5f0c6b54..2ed6b163f3c8 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > @@ -1305,6 +1305,10 @@ static int enetc_xdp_frame_to_xdp_tx_swbd(struct enetc_bdr *tx_ring,
> >  	xdp_tx_swbd->xdp_frame = NULL;
> >  
> >  	n++;
> > +
> > +	if (!xdp_frame_has_frags(xdp_frame))
> > +		goto out;
> > +
> 
> Tested this with single-buffer devmap XDP_REDIRECT, can't test with
> multi-buffer I think.

ack, thx for testing. I will add this patch to the prvious series.
In oreder to test it with xdp-mb I think you can redirect into a cpumap and
then attach a program on the cpumap to redirect back to the nic, but for the
moment you need to comment out this line:

https://github.com/torvalds/linux/blob/master/net/core/filter.c#L4291

Regards,
Lorenzo

> 
> >  	xdp_tx_swbd = &xdp_tx_arr[n];
> >  
> >  	shinfo = xdp_get_shared_info_from_frame(xdp_frame);
> > @@ -1334,7 +1338,7 @@ static int enetc_xdp_frame_to_xdp_tx_swbd(struct enetc_bdr *tx_ring,
> >  		n++;
> >  		xdp_tx_swbd = &xdp_tx_arr[n];
> >  	}
> > -
> > +out:
> >  	xdp_tx_arr[n - 1].is_eof = true;
> >  	xdp_tx_arr[n - 1].xdp_frame = xdp_frame;
> >  
> > @@ -1390,16 +1394,12 @@ static void enetc_map_rx_buff_to_xdp(struct enetc_bdr *rx_ring, int i,
> >  {
> >  	struct enetc_rx_swbd *rx_swbd = enetc_get_rx_buff(rx_ring, i, size);
> >  	void *hard_start = page_address(rx_swbd->page) + rx_swbd->page_offset;
> > -	struct skb_shared_info *shinfo;
> >  
> >  	/* To be used for XDP_TX */
> >  	rx_swbd->len = size;
> >  
> >  	xdp_prepare_buff(xdp_buff, hard_start - rx_ring->buffer_offset,
> >  			 rx_ring->buffer_offset, size, false);
> > -
> > -	shinfo = xdp_get_shared_info_from_buff(xdp_buff);
> > -	shinfo->nr_frags = 0;
> >  }
> >  
> >  static void enetc_add_rx_buff_to_xdp(struct enetc_bdr *rx_ring, int i,
> > @@ -1407,7 +1407,7 @@ static void enetc_add_rx_buff_to_xdp(struct enetc_bdr *rx_ring, int i,
> >  {
> >  	struct skb_shared_info *shinfo = xdp_get_shared_info_from_buff(xdp_buff);
> >  	struct enetc_rx_swbd *rx_swbd = enetc_get_rx_buff(rx_ring, i, size);
> > -	skb_frag_t *frag = &shinfo->frags[shinfo->nr_frags];
> > +	skb_frag_t *frag;
> >  
> >  	/* To be used for XDP_TX */
> >  	rx_swbd->len = size;
> > @@ -1415,6 +1415,7 @@ static void enetc_add_rx_buff_to_xdp(struct enetc_bdr *rx_ring, int i,
> >  	if (!xdp_buff_has_frags(xdp_buff)) {
> >  		xdp_buff_set_frags_flag(xdp_buff);
> >  		shinfo->xdp_frags_size = size;
> > +		shinfo->nr_frags = 0;
> 
> Tested this and enetc_map_rx_buff_to_xdp() with single-buffer and
> multi-buffer XDP_TX.
> 
> >  	} else {
> >  		shinfo->xdp_frags_size += size;
> >  	}
> > @@ -1422,6 +1423,7 @@ static void enetc_add_rx_buff_to_xdp(struct enetc_bdr *rx_ring, int i,
> >  	if (page_is_pfmemalloc(rx_swbd->page))
> >  		xdp_buff_set_frag_pfmemalloc(xdp_buff);
> >  
> > +	frag = &shinfo->frags[shinfo->nr_frags];
> >  	skb_frag_off_set(frag, rx_swbd->page_offset);
> >  	skb_frag_size_set(frag, size);
> >  	__skb_frag_set_page(frag, rx_swbd->page);
> > -- 
> > 2.38.1
> >
> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]