Subject: Re: [PATCH v3 bpf-next 06/17] libbpf: xsk: use bpf_link

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Thu, 25 Mar 2021 00:38:07 +0100

Maciej Fijalkowski <[email-masked]> writes:

> On Mon, Mar 22, 2021 at 10:47:09PM +0100, Toke Høiland-Jørgensen wrote:
>> Maciej Fijalkowski <[email-masked]> writes:
>> 
>> > 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...

> If we agree on that and there's nothing else that I missed, I'll send
> a v4.

Apart from the above, sure!

-Toke