Maciej Fijalkowski wrote: > On Tue, Feb 16, 2021 at 11:15:41AM -0800, John Fastabend wrote: > > Toke Høiland-Jørgensen wrote: > > > Björn Töpel <bjorn.topel@xxxxxxxxx> writes: > > > > > > > On 2021-02-15 21:49, John Fastabend wrote: > > > >> Maciej Fijalkowski wrote: > > > >>> 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. > > > >>> > > > >>> If there's no bpf_link yet, create one for a given XDP prog and unload > > > >>> explicitly existing prog if XDP_FLAGS_UPDATE_IF_NOEXIST is not set. > > > >>> > > > >>> If bpf_link is already at a given ifindex and underlying program is not > > > >>> AF-XDP one, bail out or update the bpf_link's prog given the presence of > > > >>> XDP_FLAGS_UPDATE_IF_NOEXIST. > > > >>> > > > >>> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx> [...] > > > > I'm not a real user of AF_XDP (yet.), but here is how I would expect it > > to work based on how the sockmap pieces work, which are somewhat similar > > given they also deal with sockets. > > Don't want to be picky, but I suppose sockmap won't play with ndo_bpf() > and that's what was bothering us. > > > > > Program > > (1) load and pin an XDP BPF program > > - obj = bpf_object__open(prog); > > - bpf_object__load_xattr(&attr); > > - bpf_program__pin() > > (2) pin the map, find map_xsk using any of the map APIs > > - bpf_map__pin(map_xsk, path_to_pin) > > (3) attach to XDP > > - link = bpf_program__attach_xdp() > > - bpf_link__pin() > > > > At this point you have a BPF program loaded, a xsk map, and a link all > > pinned and ready. And we can add socks using the process found in > > `enter_xsks_into_map` in the sample. This can be the same program that > > loaded/pinned the XDP program or some other program it doesn't really > > matter. > > > > - create xsk fd > > . xsk_umem__create() > > . xsk_socket__create > > - open map @ pinned path > > - bpf_map_update_elem(xsks_map, &key, &fd, 0); > > > > Then it looks like we don't have any conflicts? The XDP program is pinned > > and exists in its normal scope. The xsk elements can be added/deleted > > as normal. > > The only difference from what you wrote up is the resource pinning, when > compared to what we currently have + the set I am proposing. > > So, if you're saying it looks like we don't have any conflicts and I am > saying that the flow is what we have, then...? :) > > You would have to ask Magnus what was behind the decision to avoid API > from tools/lib/bpf/libbpf.c but rather directly call underlying functions > from tools/lib/bpf/bpf.c. Nevertheless, it doesn't really make a big > difference to me. > > > If the XDP program is removed and the map referencing (using > > normal ref rules) reaches zero its also deleted. Above is more or less > > the same flow we use for any BPF program so looks good to me. > > We have to be a bit more specific around the "XDP program is removed". > Is it removed from the network interface? That's the thing that we want to > avoid when there are other xsk sockets active on a given interface. What I'm suggesting here is to use the normal rules for when an XDP program is removed from the network interface. I don't think we should do anything extra to keep the XDP program attached/loaded just because it has a xsk map which may or may not have some entries in it. If the user wants this behavior they should pin the bpf_link pointer associated with the xdp program. This is the same as any other program/map I have in my BPF system. > > With bpf_link, XDP prog is removed only when bpf_link's refcount reaches > zero, via link->ops->release() callback that is invoked from > bpf_link_put(). > > And the refcount is updated with each process that attaches/detaches from > the bpf_link on interface. > > > > > The trouble seems to pop up when using the higher abstraction APIs > > xsk_setup_xdp_prog and friends I guess? I just see above as already > > fairly easy to use and we have good helpers to create the sockets it looks > > like. Maybe I missed some design considerations. IMO higher level > > abstractions should go in new libxdp and above should stay in libbpf. > > xsk_setup_xdp_prog doesn't really feel like higher level abstraction, as I > mentioned, to me it has one layer of abstraction peeled off. Except it seems to have caused some issues. I don't think I gain much from the API personally vs just doing above steps. But, I appreciate you are just trying to fix it here so patches are a good idea with v2 improvements. And I expect whenever libbpf/libxdp split the above "high level" APIs will land in libxdp. Thanks, John > > > > > /rant off ;) > > > > Thanks, > > John
