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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Jan 2021 13:29:33 +0100
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Hangbin Liu <liuhangbin@...il.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

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

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

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