>
> Lorenzo Bianconi <lorenzo@xxxxxxxxxx> writes:
>
> > ...
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > index 102f2c91fdb8..7ad0557dedbd 100644
> > --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > ...
> >
> > @@ -339,8 +337,8 @@ static int ena_xdp_xmit(struct net_device *dev, int
> > n,
> > struct xdp_frame **frames, u32 flags)
> > {
> > struct ena_adapter *adapter = netdev_priv(dev);
> > - int qid, i, err, drops = 0;
> > struct ena_ring *xdp_ring;
> > + int qid, i, nxmit = 0;
> > if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> > return -EINVAL;
> > @@ -360,12 +358,12 @@ static int ena_xdp_xmit(struct net_device *dev,
> > int n,
> > spin_lock(&xdp_ring->xdp_tx_lock);
> > for (i = 0; i < n; i++) {
> > - err = ena_xdp_xmit_frame(xdp_ring, dev, frames[i], 0);
> > /* The descriptor is freed by ena_xdp_xmit_frame in case
> > * of an error.
> > */
>
> Thanks a lot for the patch. It's a good idea. Do you mind removing the
> comment here as well ? ena_xdp_xmit_frame() no longer frees the frame in
> case of an error after this patch.
ack, will do in v3
>
> > - if (err)
> > - drops++;
> > + if (ena_xdp_xmit_frame(xdp_ring, dev, frames[i], 0))
> > + break;
> > + nxmit++;
> > }
> > /* Ring doorbell to make device aware of the packets */
> > @@ -378,7 +376,7 @@ static int ena_xdp_xmit(struct net_device *dev, int
> > n,
> > spin_unlock(&xdp_ring->xdp_tx_lock);
> > /* Return number of packets sent */
> > - return n - drops;
> > + return nxmit;
> > }
> > ...
> > diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> > index 85d9d1b72a33..9f158b3862df 100644
> > --- a/kernel/bpf/devmap.c
> > +++ b/kernel/bpf/devmap.c
> > @@ -344,29 +344,26 @@ static void bq_xmit_all(struct xdp_dev_bulk_queue
> > *bq, u32 flags)
> > sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q, flags);
> > if (sent < 0) {
> > + /* If ndo_xdp_xmit fails with an errno, no frames have
> > + * been xmit'ed.
> > + */
> > err = sent;
> > sent = 0;
> > - goto error;
> > }
> > +
> > drops = bq->count - sent;
> > -out:
> > - bq->count = 0;
> > + if (unlikely(drops > 0)) {
> > + /* If not all frames have been transmitted, it is our
> > + * responsibility to free them
> > + */
> > + for (i = sent; i < bq->count; i++)
> > + xdp_return_frame_rx_napi(bq->q[i]);
> > + }
>
> Wouldn't the logic above be the same even w/o the 'if' condition ?
it is just an optimization to avoid the for loop instruction if sent = bq->count
Regards,
Lorenzo
>
> > + bq->count = 0;
> > trace_xdp_devmap_xmit(bq->dev_rx, dev, sent, drops, err);
> > bq->dev_rx = NULL;
> > __list_del_clearprev(&bq->flush_node);
> > - return;
> > -error:
> > - /* If ndo_xdp_xmit fails with an errno, no frames have been
> > - * xmit'ed and it's our responsibility to them free all.
> > - */
> > - for (i = 0; i < bq->count; i++) {
> > - struct xdp_frame *xdpf = bq->q[i];
> > -
> > - xdp_return_frame_rx_napi(xdpf);
> > - drops++;
> > - }
> > - goto out;
> > }
> > /* __dev_flush is called from xdp_do_flush() which _must_ be
> > signaled
>
> Thanks, Shay
>
Attachment:
signature.asc
Description: PGP signature
