On Mon, Feb 15, 2021 at 08:35:29PM +0100, Toke Høiland-Jørgensen wrote: > Björn Töpel <bjorn.topel@xxxxxxxxx> writes: > > > On 2021-02-15 18:07, 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. > >>> > >>> 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. > >> > >> One consideration here is that bpf_link_get_fd_by_id() is a privileged > >> operation (privileged as in CAP_SYS_ADMIN), so this has the side effect > >> of making AF_XDP privileged as well. Is that the intention? > >> > > > > We're already using, e.g., bpf_map_get_fd_by_id() which has that > > as well. So we're assuming that for XDP setup already! > > Ah, right, didn't realise that one is CAP_SYS_ADMIN as well; I > remembered this as being specific to the bpf_link operation. > > >> Another is that the AF_XDP code is in the process of moving to libxdp > >> (see in-progress PR [0]), and this approach won't carry over as-is to > >> that model, because libxdp has to pin the bpf_link fds. > >> > > > > I was assuming there were two modes of operations for AF_XDP in libxdp. > > One which is with the multi-program support (which AFAIK is why the > > pinning is required), and one "like the current libbpf" one. For the > > latter Maciej's series would be a good fit, no? > > We haven't added an explicit mode switch for now; libxdp will fall back > to regular interface attach if the kernel doesn't support the needed > features for multi-attach, but if it's possible to just have libxdp > transparently do the right thing I'd much prefer that. So we're still > exploring that (part of which is that Magnus has promised to run some > performance tests to see if there's a difference). > > However, even if there's an explicit mode switch I'd like to avoid > different *semantics* between the two modes if possible, to keep the two > as compatible as possible. And since we can't currently do "auto-detach > on bpf_link fd close" when using multi-prog, introducing this now would > lead to just such a semantic difference. So my preference would be to do > it differently... :) > > >> 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! WDYM by libbpf 1.0/libxdp merge? I glanced through thread and I saw that John was also not aware of that. Not sure where it was discussed? If you're saying 'merge', then is libxdp going to be a part of kernel or as an AF-XDP related guy I would be forced to include yet another repository in the BPF developer toolchain? :< > > Well, as far as libxdp support goes, the PR I linked is pretty close to > being mergeable. One of the few outstanding issues is whether we should > solve just this issue before merging, actually :) > > Not sure exactly which timeframe Andrii is envisioning for libbpf 1.0, > but last I heard he'll announce something next week. > > > Bottom line; I'd *really* like bpf_link behavior (process scoped) for > > AF_XDP sooner than later! ;-) > > Totally agree that we should solve the multi-process coexistence > problem! And as I said, I think we can do so in libxdp by using the same > synchronisation mechanism we use for setting up the multi-prog > dispatcher. So it doesn't *have* to hold things up :) Am I reading this right or you're trying to reject the fix of the long standing issue due to a PR that is not ready yet on a standalone project/library? :P Once libxdp is the standard way of playing with AF-XDP and there are actual users of that, then I'm happy to work/help on that issue. Spawning a few xdpsock instances on the same interface has been a standard/easiest way of measuring the scalability of AF-XDP ZC implementations. This has been a real PITA for quite a long time. So, I second Bjorn's statement - the sooner we have this fixed, the better. Thanks! I hope I haven't sounded harsh, not my intent at all, Maciej > > -Toke >
