Date:   Tue, 16 Feb 2021 13:17:25 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     Toke Høiland-Jørgensen <toke@...hat.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        daniel@...earbox.net, ast@...nel.org, bpf@...r.kernel.org,
        netdev@...r.kernel.org, andrii@...nel.org,
        magnus.karlsson@...el.com, ciara.loftus@...el.com
Subject: Re: [PATCH bpf-next 1/3] libbpf: xsk: use bpf_link

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@...el.com> 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@...el.com>

[...]

> > 
> > 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

Powered by blists - more mailing lists