On Sun, Jan 17, 2021 at 02:57:02PM -0800, John Fastabend wrote:
[...]
> It looks like we could embed xdp_buff in xdp_frame and then keep the metadata
> at the end.
>
> Because you are working performance here wdyt? <- @Jesper as well.
Leave this question to Jesper.
> >
> > - sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q, flags);
> > + if (unlikely(bq->xdp_prog)) {
>
> Whats the rational for making above unlikely()? Seems for users its not
> unlikely. Can you measure a performance increase/decrease here? I think
> its probably fine to just let compiler/prefetcher do its thing here. Or
> I'm not reading this right, but seems users of bq->xdp_prog would disagree
> on unlikely case?
>
> Either way a comment might be nice to give us some insight in 6 months
> why we decided this is unlikely.
I agree that there is no need to use unlikely() here.
>
> > + xdp_drop = dev_map_bpf_prog_run(bq->xdp_prog, bq->q, cnt, dev);
> > + cnt -= xdp_drop;
> > + if (!cnt) {
>
>
> if dev_map_bpf_prog_run() returned sent packets this would read better
> imo.
>
> sent = dev_map_bpf_prog_run(...)
> if (!sent)
> goto out;
>
> > + sent = 0;
> > + drops = xdp_drop;
> > + goto out;
> > + }
> > + }
> > +
> > + sent = dev->netdev_ops->ndo_xdp_xmit(dev, cnt, bq->q, flags);
>
> And, sent = dev->netdev_ops->ndo_xdp_xmit(dev, sent, bq->q, flags);
>
> > if (sent < 0) {
> > err = sent;
> > sent = 0;
> > goto error;
> > }
> > - drops = bq->count - sent;
> > + drops = (cnt - sent) + xdp_drop;
>
> With about 'sent' logic then drops will still be just, drops = bq->count - sent
> and move the calculation below the out label and I think you clean up above
If we use the 'sent' logic, we should also backup the drop value before
xmit as the erro label also need it.
> as well. Did I miss something...
>
> > out:
> > bq->count = 0;
> >
> > trace_xdp_devmap_xmit(bq->dev_rx, dev, sent, drops, err);
> > bq->dev_rx = NULL;
> > + bq->xdp_prog = 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++) {
> > + for (i = 0; i < cnt; i++) {
> > struct xdp_frame *xdpf = bq->q[i];
here it will be "for (i = 0; i < cnt - drops; i++)" to free none xmit'ed
frames.
To make the logic more clear, here is the full code:
[...]
if (bq->xdp_prog) {
sent = dev_map_bpf_prog_run(bq->xdp_prog, bq->q, cnt, dev);
if (!sent)
goto out;
}
/* Backup drops value before xmit as we may need it in error label */
drops = cnt - sent;
sent = dev->netdev_ops->ndo_xdp_xmit(dev, sent, bq->q, flags);
if (sent < 0) {
err = sent;
sent = 0;
goto error;
}
out:
drops = cnt - sent;
bq->count = 0;
trace_xdp_devmap_xmit(bq->dev_rx, dev, sent, drops, err);
bq->dev_rx = NULL;
bq->xdp_prog = 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 < cnt - drops; i++) {
struct xdp_frame *xdpf = bq->q[i];
xdp_return_frame_rx_napi(xdpf);
}
goto out;
}
Thanks
hangbin
