[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKH8qBvgkTXFEhd9hOa+SFtqKAXuD=WM_h1TZYdQA0d70_drEA@mail.gmail.com>
Date: Thu, 8 Dec 2022 15:46:55 -0800
From: Stanislav Fomichev <sdf@...gle.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org,
yhs@...com, john.fastabend@...il.com, kpsingh@...nel.org,
haoluo@...gle.com, jolsa@...nel.org,
David Ahern <dsahern@...il.com>,
Jakub Kicinski <kuba@...nel.org>,
Willem de Bruijn <willemb@...gle.com>,
Jesper Dangaard Brouer <brouer@...hat.com>,
Anatoly Burakov <anatoly.burakov@...el.com>,
Alexander Lobakin <alexandr.lobakin@...el.com>,
Magnus Karlsson <magnus.karlsson@...il.com>,
Maryam Tahhan <mtahhan@...hat.com>, xdp-hints@...-project.net,
netdev@...r.kernel.org
Subject: Re: [xdp-hints] [PATCH bpf-next v3 03/12] bpf: XDP metadata RX kfuncs
On Thu, Dec 8, 2022 at 2:39 PM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Stanislav Fomichev <sdf@...gle.com> writes:
>
> > There is an ndo handler per kfunc, the verifier replaces a call to the
> > generic kfunc with a call to the per-device one.
> >
> > For XDP, we define a new kfunc set (xdp_metadata_kfunc_ids) which
> > implements all possible metatada kfuncs. Not all devices have to
> > implement them. If kfunc is not supported by the target device,
> > the default implementation is called instead.
>
> So one unfortunate consequence of this "fallback to the default
> implementation" is that it's really easy to get a step wrong and end up
> with something that doesn't work. Specifically, if you load an XDP
> program that calls the metadata kfuncs, but don't set the ifindex and
> flag on load, the kfunc resolution will work just fine, but you'll end
> up calling the default kfunc implementations (and get no data). I ran
> into this multiple times just now when playing around with it and
> implementing the freplace support.
>
> So I really think it would be a better user experience if we completely
> block (with a nice error message!) the calling of the metadata kfuncs if
> the program is not device-bound...
Oh, right, that's a good point. Having defaults for dev-bound only
makes total sense.
> Another UX thing I ran into is that libbpf will bail out if it can't
> find the kfunc in the kernel vmlinux, even if the code calling the
> function is behind an always-false if statement (which would be
> eliminated as dead code from the verifier). This makes it a bit hard to
> conditionally use them. Should libbpf just allow the load without
> performing the relocation (and let the verifier worry about it), or
> should we have a bpf_core_kfunc_exists() macro to use for checking?
> Maybe both?
I'm not sure how libbpf can allow the load without performing the
relocation; maybe I'm missing something.
IIUC, libbpf uses the kfunc name (from the relocation?) and replaces
it with the kfunc id, right?
Having bpf_core_kfunc_exists would help, but this probably needs
compiler work first to preserve some of the kfunc traces in vmlinux.h?
So yeah, I don't have any good ideas/suggestions here on how to make
it all magically work :-(
> > Upon loading, if BPF_F_XDP_HAS_METADATA is passed via prog_flags,
> > we treat prog_index as target device for kfunc resolution.
>
> [...]
>
> > - if (!bpf_prog_map_compatible(map, prog)) {
> > - bpf_prog_put(prog);
> > - return ERR_PTR(-EINVAL);
> > - }
> > + /* When tail-calling from a non-dev-bound program to a dev-bound one,
> > + * XDP metadata helpers should be disabled. Until it's implemented,
> > + * prohibit adding dev-bound programs to tail-call maps.
> > + */
> > + if (bpf_prog_is_dev_bound(prog->aux))
> > + goto err;
> > +
> > + if (!bpf_prog_map_compatible(map, prog))
> > + goto err;
>
> I think it's better to move the new check into bpf_prog_map_compatible()
> itself; that way it'll cover cpumaps and devmaps as well :)
Will do, thanks!
> -Toke
>
Powered by blists - more mailing lists