netdev - RE: [PATCH net-next 4/4] dpaa2-eth: use bulk enqueue in .ndo_xdp

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]

Message-ID: <DB8PR04MB6828FCFAC2B6755E046B0B05E0D20@DB8PR04MB6828.eurprd04.prod.outlook.com>
Date:   Wed, 22 Apr 2020 07:51:46 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net-next 4/4] dpaa2-eth: use bulk enqueue in .ndo_xdp_xmit

> Subject: Re: [PATCH net-next 4/4] dpaa2-eth: use bulk enqueue in
> .ndo_xdp_xmit
> 
> On Tue, 21 Apr 2020 18:21:54 +0300
> Ioana Ciornei <ioana.ciornei@....com> wrote:
> 
> > Take advantage of the bulk enqueue feature in .ndo_xdp_xmit.
> > We cannot use the XDP_XMIT_FLUSH since the architecture is not capable
> > to store all the frames dequeued in a NAPI cycle so we instead are
> > enqueueing all the frames received in a ndo_xdp_xmit call right away.
> >
> > After setting up all FDs for the xdp_frames received, enqueue multiple
> > frames at a time until all are sent or the maximum number of retries
> > is hit.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>
> > ---
> >  .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  | 60
> > ++++++++++---------
> >  1 file changed, 32 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > index 9a0432cd893c..08b4efad46fd 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > @@ -1933,12 +1933,12 @@ static int dpaa2_eth_xdp_xmit(struct net_device
> *net_dev, int n,
> >  			      struct xdp_frame **frames, u32 flags)  {
> >  	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
> > +	int total_enqueued = 0, retries = 0, enqueued;
> >  	struct dpaa2_eth_drv_stats *percpu_extras;
> >  	struct rtnl_link_stats64 *percpu_stats;
> > +	int num_fds, i, err, max_retries;
> >  	struct dpaa2_eth_fq *fq;
> > -	struct dpaa2_fd fd;
> > -	int drops = 0;
> > -	int i, err;
> > +	struct dpaa2_fd *fds;
> >
> >  	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> >  		return -EINVAL;
> > @@ -1946,41 +1946,45 @@ static int dpaa2_eth_xdp_xmit(struct net_device
> *net_dev, int n,
> >  	if (!netif_running(net_dev))
> >  		return -ENETDOWN;
> >
> > +	/* create the array of frame descriptors */
> > +	fds = kcalloc(n, sizeof(*fds), GFP_ATOMIC);
> 
> I don't like that you have an allocation on the transmit fast-path.
> 
> There are a number of ways you can avoid this.
> 
> Option (1) Given we know that (currently) devmap will max bulk 16 xdp_frames,
> we can have a call-stack local array with struct dpaa2_fd, that contains 16
> elements, sizeof(struct dpaa2_fd)==32 bytes times 16 is
> 512 bytes, so it might be acceptable.  (And add code to alloc if n > 16, to be
> compatible with someone increasing max bulk in devmap).
> 

I didn't know about the 16 max xdp_frames . Thanks.

> Option (2) extend struct dpaa2_eth_priv with an array of 16 struct dpaa2_fd's,
> that can be used as fds storage.

I have more of a noob question here before proceeding with one of the two options.

The ndo_xdp_xmit() callback can be called concurrently from multiple softirq contexts, right?

If the above is true, then I think the dpaa2_eth_ch_xdp is the right struct to place the array of 16 FDs.

Also, is there any caveat to just use DEV_MAP_BULK_SIZE when declaring the array?

Ioana

> 
> > +	if (!fds)
> > +		return -ENOMEM;
> > +
> 
> [...]
> > +	kfree(fds);
> 
>