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