netdev - RE: [PATCH net] net: fec: tx processing does not call XDP APIs if budget is 0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 26 Jul 2023 03:40:54 +0000
From: Wei Fang <wei.fang@....com>
To: Alexander H Duyck <alexander.duyck@...il.com>
CC: dl-linux-imx <linux-imx@....com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "kuba@...nel.org"
	<kuba@...nel.org>, Shenwei Wang <shenwei.wang@....com>, Clark Wang
	<xiaoning.wang@....com>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net] net: fec: tx processing does not call XDP APIs if
 budget is 0

Hi Alexander,

> > @@ -1416,6 +1416,14 @@ fec_enet_tx_queue(struct net_device *ndev,
> u16 queue_id)
> >  			if (!skb)
> >  				goto tx_buf_done;
> >  		} else {
> > +			/* Tx processing cannot call any XDP (or page pool) APIs if
> > +			 * the "budget" is 0. Because NAPI is called with budget of
> > +			 * 0 (such as netpoll) indicates we may be in an IRQ context,
> > +			 * however, we can't use the page pool from IRQ context.
> > +			 */
> > +			if (unlikely(!budget))
> > +				break;
> > +
> >  			xdpf = txq->tx_buf[index].xdp;
> >  			if (bdp->cbd_bufaddr)
> >  				dma_unmap_single(&fep->pdev->dev,
> 
> This statement isn't correct. There are napi enabled and non-napi
> versions of these calls. This is the reason for things like the
> "allow_direct" parameter in page_pool_put_full_page and the
> "napi_direct" parameter in __xdp_return.
> 
> By blocking on these cases you can end up hanging the Tx queue which is
> going to break netpoll as you are going to stall the ring on XDP
> packets if they are already in the queue.
> 
> From what I can tell your driver is using xdp_return_frame in the case
> of an XDP frame which doesn't make use of the NAPI optimizations in
> freeing from what I can tell. The NAPI optimized version is
> xdp_return_frame_rx.
> 
So you mean it is safe to use xdp_return_frame no matter in NAPI context
or non-NAPI context? And xdp_return_frame_rx_napi must be used in NAPI
context? If so, I think I must have misunderstood, then this patch is not necessary.

> > @@ -1508,14 +1516,14 @@ fec_enet_tx_queue(struct net_device *ndev,
> u16 queue_id)
> >  		writel(0, txq->bd.reg_desc_active);
> >  }
> >
> > -static void fec_enet_tx(struct net_device *ndev)
> > +static void fec_enet_tx(struct net_device *ndev, int budget)
> >  {
> >  	struct fec_enet_private *fep = netdev_priv(ndev);
> >  	int i;
> >
> >  	/* Make sure that AVB queues are processed first. */
> >  	for (i = fep->num_tx_queues - 1; i >= 0; i--)
> > -		fec_enet_tx_queue(ndev, i);
> > +		fec_enet_tx_queue(ndev, i, budget);
> >  }
> >
> >  static void fec_enet_update_cbd(struct fec_enet_priv_rx_q *rxq,
> > @@ -1858,7 +1866,7 @@ static int fec_enet_rx_napi(struct napi_struct
> *napi, int budget)
> >
> >  	do {
> >  		done += fec_enet_rx(ndev, budget - done);
> > -		fec_enet_tx(ndev);
> > +		fec_enet_tx(ndev, budget);
> >  	} while ((done < budget) && fec_enet_collect_events(fep));
> >
> >  	if (done < budget) {
> 
> Since you are passing budget, one optimization you could make use of
> would be napi_consume_skb in your Tx path instead of dev_kfree_skb_any.
That's good suggestion, I think I can add this optimization in my XDP_TX support
patch. Thanks!