Date:   Thu, 15 Dec 2022 10:24:49 -0800
From:   William Tu <u9012063@...il.com>
To:     Alexander H Duyck <alexander.duyck@...il.com>
Cc:     netdev@...r.kernel.org, tuc@...are.com, gyang@...are.com,
        doshir@...are.com
Subject: Re: [RFC PATCH v5] vmxnet3: Add XDP support.

On Wed, Dec 14, 2022 at 2:51 PM Alexander H Duyck
<alexander.duyck@...il.com> wrote:
>
> On Wed, 2022-12-14 at 13:55 -0800, William Tu wrote:
> > Thanks for taking a look at this patch!
> >
> > <...>
> > >
> > > > +int
> > > > +vmxnet3_process_xdp(struct vmxnet3_adapter *adapter,
> > > > +                 struct vmxnet3_rx_queue *rq,
> > > > +                 struct Vmxnet3_RxCompDesc *rcd,
> > > > +                 struct vmxnet3_rx_buf_info *rbi,
> > > > +                 struct Vmxnet3_RxDesc *rxd,
> > > > +                 bool *need_flush)
> > > > +{
> > > > +     struct bpf_prog *xdp_prog;
> > > > +     dma_addr_t new_dma_addr;
> > > > +     struct sk_buff *new_skb;
> > > > +     bool rxDataRingUsed;
> > > > +     int ret, act;
> > > > +
> > > > +     ret = VMXNET3_XDP_CONTINUE;
> > > > +     if (unlikely(rcd->len == 0))
> > > > +             return VMXNET3_XDP_TAKEN;
> > > > +
> > > > +     rxDataRingUsed = VMXNET3_RX_DATA_RING(adapter, rcd->rqID);
> > > > +     rcu_read_lock();
> > > > +     xdp_prog = rcu_dereference(rq->xdp_bpf_prog);
> > > > +     if (!xdp_prog) {
> > > > +             rcu_read_unlock();
> > > > +             return VMXNET3_XDP_CONTINUE;
> > > > +     }
> > > > +     act = vmxnet3_run_xdp(rq, rbi, rcd, need_flush, rxDataRingUsed);
> > > > +     rcu_read_unlock();
> > > > +
> > > > +     switch (act) {
> > > > +     case XDP_PASS:
> > > > +             ret = VMXNET3_XDP_CONTINUE;
> > > > +             break;
> > > > +     case XDP_DROP:
> > > > +     case XDP_TX:
> > > > +     case XDP_REDIRECT:
> > > > +     case XDP_ABORTED:
> > > > +     default:
> > > > +             /* Reuse and remap the existing buffer. */
> > > > +             ret = VMXNET3_XDP_TAKEN;
> > > > +             if (rxDataRingUsed)
> > > > +                     return ret;
> > > > +
> > > > +             new_skb = rbi->skb;
> > > > +             new_dma_addr =
> > > > +                     dma_map_single(&adapter->pdev->dev,
> > > > +                                    new_skb->data, rbi->len,
> > > > +                                    DMA_FROM_DEVICE);
> > > > +             if (dma_mapping_error(&adapter->pdev->dev,
> > > > +                                   new_dma_addr)) {
> > > > +                     dev_kfree_skb(new_skb);
> > > > +                     rq->stats.drop_total++;
> > > > +                     return ret;
> > > > +             }
> > > > +             rbi->dma_addr = new_dma_addr;
> > > > +             rxd->addr = cpu_to_le64(rbi->dma_addr);
> > > > +             rxd->len = rbi->len;
> > > > +     }
> > > > +     return ret;
> > > > +}
> > >
> > > FOr XDP_DROP and XDP_ABORTED this makes sense. You might want to add a
> > > trace point in the case of aborted just so you can catch such cases for
> > > debug.
> > Good point, I will add the trace point.
> >
>
> You will probably want to add that trace point in __vmxnet3_run_xdp.

Yes, thanks.

>
> > > However for XDP_TX and XDP_REDIRECT shouldn't both of those be calling
> > > out to seperate functions to either place the frame on a Tx ring or to
> > > hand it off to xdp_do_redirect so that the frame gets routed where it
> > > needs to go? Also don't you run a risk with overwriting frames that
> > > might be waiting on transmit?
> >
> > Yes, I have XDP_TX and XDP_REDIRECT handled in another function,
> > the vmxnet3_run_xdp() and __vmxnet3_run_xdp().
>
> Okay, for the redirect case it looks like you address it by doing a
> memcpy to a freshly allocated page so that saves us that trouble in
> that case.
>
> > How do I avoid overwriting frames that might be waiting on transmit?
> > I checked my vmxnet3_xdp_xmit_back and vmxnet3_xdp_xmit_frame,
> > I think since I called the vmxnet3_xdp_xmit_frame at the rx context,
> > it should be ok?
>
> I don't think you can guarantee that. Normally for TX you would want to
> detach and replace the page unless you have some sort of other
> recycling/reuse taking care of it for you. Normally that is handled via
> page pool.
>
> On the Intel parts I had gotten around that via our split buffer model
> so we just switched to the other half of the page while the Tx sat on
> the first half, and by the time we would have to check again we would
> either detach the page for flip back if it had already been freed by
> the Tx path.
I see your point. So for XDP_TX, I can also do s.t like I did in XDP_REDIRECT,
memcpy to a freshly allocated page so the frame won't get overwritten.
Probably the performance will suffer.
Do you suggest allocating new page or risk buffer overwritten?

>
> > +static int
> > +vmxnet3_xdp_xmit_back(struct vmxnet3_adapter *adapter,
> > +                   struct xdp_frame *xdpf,
> > +                   struct sk_buff *skb)
> >
>
> Also after re-reviewing this I was wondering why you have the skb
> argument for this function? The only caller is passing NULL and I
> wouldn't expect you to be passing an skb since you are working with XDP
> buffers anyway. Seems like you could also drop the argument from
> vmxnet3_xdp_xmit_frame() since you are only passing it NULL as well.

You're right! I don't need to pass skb here. I probably forgot to remove it
when refactoring code. Will remove the two places.
Thanks!

Regards,
William

Powered by blists - more mailing lists