On Mon, Jul 08, 2019 at 05:51:37PM +0300, Ilias Apalodimas wrote:
> 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)
Thanks for the confirmation. I agree things are not totally clear as I
had to learn how all of it worked to do this. We can work on that.
>
> > >
> > > > + 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.
> >
I'll reply to myself here and note that you are correct that we need to
fixup the error case, but it actually belongs in patch 4 in the series
since that is the patch that adds page_pool support. I'll reply to that
one in just a min once I've tested my patch.
> > >
> > > 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!
So as someone who ends up doing some of this work after the trail has
already been blazed upstream on other drivers, etc, I definitely
understand the desire to keep things more common. I think the page_pool
bits are a nice step in that direction, so I enjoy any attempts to help
make repeated tasks easier for everyone.
>
> >
> > >
> > > > + 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
