Re: [PATCHv14 bpf-next 1/6] bpf: run devmap xdp_prog on flush instead of bulk enqueue

[Hangbin Liu <liuhangbin@gmail.com>]

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