On Mon, Jan 25, 2021 at 12:21:26PM +0100, Toke Høiland-Jørgensen wrote: > Hangbin Liu <liuhangbin@xxxxxxxxx> 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 >
