Hi Andy,
> On Mon, Jul 08, 2019 at 11:28:03AM +0300, Ilias Apalodimas wrote:
> > Thanks Andy, Michael
> >
> > > + if (event & BNXT_REDIRECT_EVENT)
> > > + xdp_do_flush_map();
> > > +
> > > if (event & BNXT_TX_EVENT) {
> > > struct bnxt_tx_ring_info *txr = bnapi->tx_ring;
> > > u16 prod = txr->tx_prod;
> > > @@ -2254,9 +2257,23 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
> > >
> > > for (j = 0; j < max_idx;) {
> > > struct bnxt_sw_tx_bd *tx_buf = &txr->tx_buf_ring[j];
> > > - struct sk_buff *skb = tx_buf->skb;
> > > + struct sk_buff *skb;
> > > int k, last;
> > >
> > > + if (i < bp->tx_nr_rings_xdp &&
> > > + tx_buf->action == XDP_REDIRECT) {
> > > + dma_unmap_single(&pdev->dev,
> > > + dma_unmap_addr(tx_buf, mapping),
> > > + dma_unmap_len(tx_buf, len),
> > > + PCI_DMA_TODEVICE);
> > > + xdp_return_frame(tx_buf->xdpf);
> > > + tx_buf->action = 0;
> > > + tx_buf->xdpf = NULL;
> > > + j++;
> > > + continue;
> > > + }
> > > +
> >
> > Can't see the whole file here and maybe i am missing something, but since
> > you
> > optimize for that and start using page_pool, XDP_TX will be a re-synced (and
> > not remapped) buffer that can be returned to the pool and resynced for
> > device usage.
> > Is that happening later on the tx clean function?
>
> Take a look at the way we treat the buffers in bnxt_rx_xdp() when we
> receive them and then in bnxt_tx_int_xdp() when the transmits have
> completed (for XDP_TX and XDP_REDIRECT). I think we are doing what is
> proper with respect to mapping vs sync for both cases, but I would be
> fine to be corrected.
>
Yea seems to be doing the right thing,
XDP_TX syncs correctly and reuses with bnxt_reuse_rx_data() right?
This might be a bit confusing for someone reading the driver on the first time,
probably because you'll end up with 2 ways of recycling buffers.
Once a buffers get freed on the XDP path it's either fed back to the pool, so
the next requested buffer get served from the pools cache (ndo_xdp_xmit case in
the patch). If the buffer is used for XDP_TX is's synced correctly but recycled
via bnxt_reuse_rx_data() right? Since you are moving to page pool please
consider having a common approach towards the recycling path. I understand that
means tracking buffers types and make sure you do the right thing on 'tx clean'.
I've done something similar on the netsec driver and i do think this might be a
good thing to add on page_pool API
Again this isn't a blocker at least for me but you already have the buffer type
(via tx_buf->action)
> >
> > > + skb = tx_buf->skb;
> > > if (!skb) {
> > > j++;
> > > continue;
> > > @@ -2517,6 +2534,13 @@ static int bnxt_alloc_rx_rings(struct bnxt *bp)
> > > if (rc < 0)
> > > return rc;
> > >
> > > + rc = xdp_rxq_info_reg_mem_model(&rxr->xdp_rxq,
> > > + MEM_TYPE_PAGE_SHARED, NULL);
> > > + if (rc) {
> > > + xdp_rxq_info_unreg(&rxr->xdp_rxq);
> >
> > I think you can use page_pool_free directly here (and pge_pool_destroy once
> > Ivan's patchset gets nerged), that's what mlx5 does iirc. Can we keep that
> > common please?
>
> That's an easy change, I can do that.
>
> >
> > If Ivan's patch get merged please note you'll have to explicitly
> > page_pool_destroy, after calling xdp_rxq_info_unreg() in the general
> > unregister
> > case (not the error habdling here). Sorry for the confusion this might
> > bring!
>
> Funny enough the driver was basically doing that until page_pool_destroy
> was removed (these patches are not new). I saw last week there was
> discussion to add it back, but I did not want to wait to get this on the
> list before that was resolved.
Fair enough
>
> This path works as expected with the code in the tree today so it seemed
> like the correct approach to post something that is working, right? :-)
Yes.
It will continue to work even if you dont change the call in the future.
This is more a 'let's not spread the code' attempt, but removing and re-adding
page_pool_destroy() was/is our mess. We might as well live with the
consequences!
>
> >
> > > + return rc;
> > > + }
> > > +
> > > rc = bnxt_alloc_ring(bp, &ring->ring_mem);
> > > if (rc)
> > > return rc;
> > > @@ -10233,6 +10257,7 @@ static const struct net_device_ops
> > > bnxt_netdev_ops = {
> > [...]
> >
Thanks!
/Ilias
