Re: [PATCH v3 bpf-next 06/17] libbpf: xsk: use bpf_link - Toke Høiland-Jørgensen

From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org,
	daniel@iogearbox.net, ast@kernel.org, bjorn.topel@intel.com,
	magnus.karlsson@intel.com, ciara.loftus@intel.com,
	john.fastabend@gmail.com
Subject: Re: [PATCH v3 bpf-next 06/17] libbpf: xsk: use bpf_link
Date: Fri, 26 Mar 2021 19:52:41 +0100	[thread overview]
Message-ID: <87v99dagsm.fsf@toke.dk> (raw)
In-Reply-To: <20210326152318.GA43356@ranger.igk.intel.com>

Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes:

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