[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87r1m9mfd5.fsf@toke.dk>
Date: Mon, 25 Jan 2021 12:21:26 +0100
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
bpf@...r.kernel.org, netdev@...r.kernel.org,
Jiri Benc <jbenc@...hat.com>,
Jesper Dangaard Brouer <brouer@...hat.com>,
Eelco Chaudron <echaudro@...hat.com>, ast@...nel.org,
Daniel Borkmann <daniel@...earbox.net>,
Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
David Ahern <dsahern@...il.com>,
Andrii Nakryiko <andrii.nakryiko@...il.com>,
Alexei Starovoitov <alexei.starovoitov@...il.com>,
John Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCHv16 bpf-next 1/6] bpf: run devmap xdp_prog on flush
instead of bulk enqueue
Hangbin Liu <liuhangbin@...il.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! :)
>
> Thanks for the advice, so how about modify it like:
Yup, exactly! :)
-Toke
Powered by blists - more mailing lists