Re: [PATCH bpf-next v2 4/8] xsk: register XDP sockets at bind(), and add new AF_XDP BPF helper

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>
Cc: "Björn Töpel" <bjorn.topel@intel.com>,
	"Björn Töpel" <bjorn.topel@gmail.com>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Network Development" <netdev@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>,
	"Karlsson, Magnus" <magnus.karlsson@intel.com>,
	"Fijalkowski, Maciej" <maciej.fijalkowski@intel.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Jonathan Lemon" <jonathan.lemon@gmail.com>,
	maximmi@nvidia.com, "David S. Miller" <davem@davemloft.net>,
	"Jesper Dangaard Brouer" <hawk@kernel.org>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"Ciara Loftus" <ciara.loftus@intel.com>,
	weqaar.a.janjua@intel.com
Subject: Re: [PATCH bpf-next v2 4/8] xsk: register XDP sockets at bind(), and add new AF_XDP BPF helper
Date: Wed, 20 Jan 2021 13:15:48 -0800	[thread overview]
Message-ID: <CAADnVQ+R5JHhqUFnB_o3nJkkkcEtvO_Vk+xSDFiqP9dZ9H6vxw@mail.gmail.com> (raw)
In-Reply-To: <87pn1z2w38.fsf@toke.dk>

On Wed, Jan 20, 2021 at 12:26 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> This argument, however, I buy: bpf_redirect() is the single-purpose
> helper for redirecting to an ifindex, bpf_redirect_xsk() is the
> single-purpose helper for redirecting to an XSK, and bpf_redirect_map()
> is the generic one that does both of those and more. Fair enough,
> consider me convinced :)
>
> > A lot of back-and-forth for *one* if-statement, but it's kind of a
> > design thing for me. ;-)
>
> Surely you don't mean to imply that you have *better* things to do with
> your time than have a 10-emails-long argument over a single if
> statement? ;)

After reading this thread I think I have to pour cold water on the design.

The performance blip comes from hard coded assumptions:
+ queue_id = xdp->rxq->queue_index;
+ xs = READ_ONCE(dev->_rx[queue_id].xsk);

bpf can have specialized helpers, but imo this is beyond what's reasonable.
Please move such things into the program and try to make
bpf_redirect_map faster.

Making af_xdp non-root is orthogonal. If there is actual need for that
it has to be designed thoroughly and not presented as "this helper may
help to do that".
I don't think "may" will materialize unless people actually work
toward the goal of non-root.