On Tue, Feb 16, 2021 at 2:37 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote: > > John Fastabend <john.fastabend@xxxxxxxxx> writes: > > > Toke Høiland-Jørgensen wrote: > >> John Fastabend <john.fastabend@xxxxxxxxx> writes: > >> > >> >> > However, in libxdp we can solve the original problem in a different way, > >> >> > and in fact I already suggested to Magnus that we should do this (see > >> >> > [1]); so one way forward could be to address it during the merge in > >> >> > libxdp? It should be possible to address the original issue (two > >> >> > instances of xdpsock breaking each other when they exit), but > >> >> > applications will still need to do an explicit unload operation before > >> >> > exiting (i.e., the automatic detach on bpf_link fd closure will take > >> >> > more work, and likely require extending the bpf_link kernel support)... > >> >> > > >> >> > >> >> I'd say it's depending on the libbpf 1.0/libxdp merge timeframe. If > >> >> we're months ahead, then I'd really like to see this in libbpf until the > >> >> merge. However, I'll leave that for Magnus/you to decide! > >> > > >> > Did I miss some thread? What does this mean libbpf 1.0/libxdp merge? > >> > >> The idea is to keep libbpf focused on bpf, and move the AF_XDP stuff to > >> libxdp (so the socket stuff in xsk.h). We're adding the existing code > >> wholesale, and keeping API compatibility during the move, so all that's > >> needed is adding -lxdp when compiling. And obviously the existing libbpf > >> code isn't going anywhere until such a time as there's a general > >> backwards compatibility-breaking deprecation in libbpf (which I believe > >> Andrii is planning to do in an upcoming and as-of-yet unannounced v1.0 > >> release). > > > > OK, I would like to keep the basic XDP pieces in libbpf though. For example > > bpf_program__attach_xdp(). This way we don't have one lib to attach > > everything, but XDP. > > The details are still TDB; for now, we're just merging in the XSK code > to the libxdp repo. I expect Andrii to announce his plans for the rest > soonish. I wouldn't expect basic things like that to go away, though :) Yeah, I'll probably post more details this week. Just catching up on stuff after vacation. As mentioned already, all the basic APIs (i.e., APIs like bpf_program__attach_xdp and bpf_set_link_xdp_fd, though I hope we can give the latter a better name) will stay intact. Stay tuned! > > >> > >> While integrating the XSK code into libxdp we're trying to make it > >> compatible with the rest of the library (i.e., multi-prog). Hence my > >> preference to avoid introducing something that makes this harder :) > >> > >> -Toke > >> > > > > OK that makes sense to me thanks. But, I'm missing something (maybe its > > obvious to everyone else?). > > > > When you load an XDP program you should get a reference to it. And then > > XDP program should never be unloaded until that id is removed right? It > > may or may not have an xsk map. Why does adding/removing programs from > > an associated map have any impact on the XDP program? That seems like > > the buggy part to me. No other map behaves this way as far as I can > > tell. Now if the program with the XDP reference closes without pinning > > the map or otherwise doing something with it, sure the map gets destroyed > > and any xsk sockets are lost. > > The original bug comes from the XSK code abstracting away the fact that > an AF_XDP socket needs an XDP program on the interface to work; so if > none exists, the library will just load a program that redirects into > the socket. Which breaks since the xdpsock example application is trying > to be nice and clean up after itself, by removing the XDP program when > it's done with the socket, thus breaking any other programs using that > XDP program. So this patch introduces proper synchronisation on both add > and remove of the XDP program... > > -Toke >
