Maciej Fijalkowski [off-list ref] writes:

On Thu, Mar 25, 2021 at 12:38:07AM +0100, Toke Høiland-Jørgensen wrote:

quoted

Maciej Fijalkowski [off-list ref] writes:

quoted

On Mon, Mar 22, 2021 at 10:47:09PM +0100, 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.

Consider the scenario below:

// load xdp prog and xskmap and add entry to xskmap at idx 10
$ sudo ./xdpsock -i ens801f0 -t -q 10

// add entry to xskmap at idx 11
$ sudo ./xdpsock -i ens801f0 -t -q 11

terminate one of the processes and another one is unable to work due to
the fact that the XDP prog was unloaded 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.

Provide backward compatibility by checking if underlying system is
bpf_link capable. Do this by looking up/creating bpf_link on loopback
device. If it failed in any way, stick with netlink-based XDP prog.
Otherwise, use bpf_link-based logic.

So how is the caller supposed to know which of the cases happened?
Presumably they need to do their own cleanup in that case? AFAICT you're
changing the code to always clobber the existing XDP program on detach
in the fallback case, which seems like a bit of an aggressive change? :)

Sorry Toke, I was offline yesterday.
Yeah once again I went too far and we shouldn't do:

bpf_set_link_xdp_fd(xsk->ctx->ifindex, -1, 0);

if xsk_lookup_bpf_maps(xsk) returned non-zero value which implies that the
underlying prog is not AF_XDP related.

closing prog_fd (and link_fd under the condition that system is bpf_link
capable) is enough for that case.

I think the same thing goes for further down? With your patch, if the
code takes the else branch (after checking prog_id), and then ends up
going to err_set_bpf_maps, it'll now also do an unconditional
bpf_set_link_xdp_fd(), where before it was checking prog_id again and
only unloading if it previously loaded the program...

Hmm it's messy, I think we need a bit of refactoring here. Note that old
code was missing a close on ctx->xsks_map_fd if there was an error on
xsk_set_bpf_maps(xsk) and prog_id != 0 - given that
xsk_lookup_bpf_maps(xsk) succeeded, we therefore have a valid map fd that
we need to take care of on error path, for !prog_id case it was taken care
of within xsk_delete_bpf_maps(xsk).

So how about a diff below (on top of this patch), where we separate paths
based on prog_id value retrieved earlier? xsk_set_bpf_maps(xsk) is
repeated but this way I feel like it's more clear with cleanup/error
paths.

Wdyt?

Yeah, that's much easier to follow! Nice :)

-Toke