Maciej Fijalkowski [off-list ref] writes:

On Mon, Feb 15, 2021 at 08:35:29PM +0100, Toke Høiland-Jørgensen wrote:

quoted

Björn Töpel [off-list ref] writes:

quoted

On 2021-02-15 18:07, Toke Høiland-Jørgensen wrote:

quoted

Maciej Fijalkowski [off-list ref] writes:

quoted

Currently, if there are multiple xdpsock instances running on a single
interface and in case one of the instances is terminated, the rest of
them are left in an inoperable state due to the fact of unloaded XDP
prog from interface.

To address that, step away from setting bpf prog in favour of bpf_link.
This means that refcounting of BPF resources will be done automatically
by bpf_link itself.

When setting up BPF resources during xsk socket creation, check whether
bpf_link for a given ifindex already exists via set of calls to
bpf_link_get_next_id -> bpf_link_get_fd_by_id -> bpf_obj_get_info_by_fd
and comparing the ifindexes from bpf_link and xsk socket.

One consideration here is that bpf_link_get_fd_by_id() is a privileged
operation (privileged as in CAP_SYS_ADMIN), so this has the side effect
of making AF_XDP privileged as well. Is that the intention?

We're already using, e.g., bpf_map_get_fd_by_id() which has that
as well. So we're assuming that for XDP setup already!

Ah, right, didn't realise that one is CAP_SYS_ADMIN as well; I
remembered this as being specific to the bpf_link operation.

quoted

quoted

Another is that the AF_XDP code is in the process of moving to libxdp
(see in-progress PR [0]), and this approach won't carry over as-is to
that model, because libxdp has to pin the bpf_link fds.

I was assuming there were two modes of operations for AF_XDP in libxdp.
One which is with the multi-program support (which AFAIK is why the
pinning is required), and one "like the current libbpf" one. For the
latter Maciej's series would be a good fit, no?

We haven't added an explicit mode switch for now; libxdp will fall back
to regular interface attach if the kernel doesn't support the needed
features for multi-attach, but if it's possible to just have libxdp
transparently do the right thing I'd much prefer that. So we're still
exploring that (part of which is that Magnus has promised to run some
performance tests to see if there's a difference).

However, even if there's an explicit mode switch I'd like to avoid
different *semantics* between the two modes if possible, to keep the two
as compatible as possible. And since we can't currently do "auto-detach
on bpf_link fd close" when using multi-prog, introducing this now would
lead to just such a semantic difference. So my preference would be to do
it differently... :)

quoted

quoted

However, in libxdp we can solve the original problem in a different way,
and in fact I already suggested to Magnus that we should do this (see
[1]); so one way forward could be to address it during the merge in
libxdp? It should be possible to address the original issue (two
instances of xdpsock breaking each other when they exit), but
applications will still need to do an explicit unload operation before
exiting (i.e., the automatic detach on bpf_link fd closure will take
more work, and likely require extending the bpf_link kernel support)...

I'd say it's depending on the libbpf 1.0/libxdp merge timeframe. If
we're months ahead, then I'd really like to see this in libbpf until the
merge. However, I'll leave that for Magnus/you to decide!

WDYM by libbpf 1.0/libxdp merge? I glanced through thread and I saw that
John was also not aware of that. Not sure where it was discussed?

If you're saying 'merge', then is libxdp going to be a part of kernel or
as an AF-XDP related guy I would be forced to include yet another
repository in the BPF developer toolchain? :<

As I replied to John, we're trying to do this in as compatible and
painless a way as possible. In particular, we'll maintain (source) API
compatibility with the code currently in libbpf. And yeah, currently
libxdp lives outside the kernel tree. Not sure whether we'll end up
moving it into the kernel tree, as Björn noted up-thread there are
arguments in both directions :)

quoted

Well, as far as libxdp support goes, the PR I linked is pretty close to
being mergeable. One of the few outstanding issues is whether we should
solve just this issue before merging, actually :)

Not sure exactly which timeframe Andrii is envisioning for libbpf 1.0,
but last I heard he'll announce something next week.

quoted

Bottom line; I'd *really* like bpf_link behavior (process scoped) for
AF_XDP sooner than later! ;-)

Totally agree that we should solve the multi-process coexistence
problem! And as I said, I think we can do so in libxdp by using the same
synchronisation mechanism we use for setting up the multi-prog
dispatcher. So it doesn't *have* to hold things up :)

Am I reading this right or you're trying to reject the fix of the long
standing issue due to a PR that is not ready yet on a standalone
project/library? :P

Haha, no, that is not what I'm saying. As I said up-thread I agree that
this is something we should fix, obviously. I'm just suggesting we fix
it in a way that will also be compatible with libxdp and multiprog so we
won't have to introduce yet-another-flag that users will have to decide
on.

However, now that I'm looking at your patch again I think my fears may
have been overblown. I got hung up on the bit in the commit message
where you said "refcounting of BPF resources will be done automatically
by bpf_link itself", and wrongly assumed that you were exposing the
bpf_link fd to the application. But I see now that it's kept in the
private xsk_ctx structure, and applications still just call
xsk_socket__delete(). So in libxdp we can just implement the same API
with a different synchronisation mechanism; that's fine. Apologies for
jumping to conclusions :/

Once libxdp is the standard way of playing with AF-XDP and there are
actual users of that, then I'm happy to work/help on that issue.

That is good to know, thanks! I opened an issue in the xdp-tools
repository to track this for the libxdp side (Magnus and I agreed that
we'll merge the existing code first, then fix this on top):
https://github.com/xdp-project/xdp-tools/issues/93

As noted above, the mechanism may have to change, but I think it's
possible to achieve the same thing in a libxdp context :)

-Toke