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

Maciej Fijalkowski
2021-01-26T20:54:14Z

On Mon, Jan 25, 2021 at 12:21:26PM +0100, Toke Høiland-Jørgensen wrote:
> Hangbin Liu <liuhangbin@gmail.com> writes:
>
> > On Fri, Jan 22, 2021 at 02:38:40PM +0100, Toke Høiland-Jørgensen wrote:
> >> >> 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;
> >> >
> >> > One more question, do you really have to do that per each bq_xmit_all
> >> > call? Couldn't you clear it in __dev_flush ?
> >> >
> >> > Or IOW - what's the rationale behind storing xdp_prog in
> >> > xdp_dev_bulk_queue. Why can't you propagate the dst->xdp_prog and rely on
> >> > that without that local pointer?
> >> >
> >> > You probably have an answer for that, so maybe include it in commit
> >> > message.
> >> >
> >> > BTW same question for clearing dev_rx. To me this will be the same for all
> >> > bq_xmit_all() calls that will happen within same napi.
> >>
> >> I think you're right: When bq_xmit_all() is called from bq_enqueue(),
> >> another packet will always be enqueued immediately after, so clearing
> >> out all of those things in bq_xmit_all() is redundant. This also
> >> includes the list_del on bq->flush_node, BTW.
> >>
> >> And while we're getting into e micro-optimisations: In bq_enqueue() we
> >> have two checks:
> >>
> >> if (!bq->dev_rx)
> >> bq->dev_rx = dev_rx;
> >>
> >> bq->q[bq->count++] = xdpf;
> >>
> >> if (!bq->flush_node.prev)
> >> list_add(&bq->flush_node, flush_list);
> >>
> >>
> >> those two if() checks can be collapsed into one, since the list and the
> >> dev_rx field are only ever modified together. This will also be the case
> >> for bq->xdp_prog, so putting all three under the same check in
> >> bq_enqueue() and only clearing them in __dev_flush() would be a win, I
> >> suppose - nice catch! :)

Huh, nice further optimization! :) Of course I agree on that.

> >
> > Thanks for the advice, so how about modify it like:
>
> Yup, exactly! :)
>
> -Toke
>