On Fri, Mar 29, 2019 at 1:16 AM Maxim Mikityanskiy <maxi...@mellanox.com> wrote: > > > -----Original Message----- > > From: Björn Töpel <bjorn.to...@gmail.com> > > Sent: 28 March, 2019 14:35 > > To: Maxim Mikityanskiy <maxi...@mellanox.com> > > Cc: Magnus Karlsson <magnus.karls...@gmail.com>; Magnus Karlsson > > <magnus.karls...@intel.com>; Björn Töpel <bjorn.to...@intel.com>; Jonathan > > Lemon <b...@fb.com>; netdev@vger.kernel.org; Daniel Borkmann > > <dan...@iogearbox.net>; Eran Ben Elisha <era...@mellanox.com>; Tariq Toukan > > <tar...@mellanox.com>; Saeed Mahameed <sae...@mellanox.com> > > Subject: Re: New xdpsock sample > > > > On Thu, 28 Mar 2019 at 10:48, Maxim Mikityanskiy <maxi...@mellanox.com> > > wrote: > > > > > [...] > > > > > Thanks for the good input, Max! The rationale for making the sample > > > > > simpler, was that most people was just C&Ping from it and used it in > > > > > their own code, so we aimed for a simple "fits-most-people" sample. > > > > > > I don't think it's easier for them to use binaries in their own code > > > than proper sources. Having the XDP program built from sources in libbpf > > > doesn't complicate the sample in any way, though. > > > > > > > Correct, but then Clang would be a libbpf build dependency. > > OK, that's the first argument that makes sense for me. You're right, now > libbpf doesn't depend on clang. However, I think pretty much every > application using libbpf also needs clang to build BPF programs, so > adding a clang dependency to libbpf itself shouldn't change anything. > And we already have the clang dependency in samples/bpf. Or do you have > an example of an application where libbpf is used, but clang is not? > We are adding AF_XDP support to OVS and we'd like to use libbpf, but not clang/llvm. The reason is that we don't have much logic in the XDP program, simply we want to have a high speed packet I/O from driver to userspace. Then the rest of processing is in userspace. In this case, I don't want to add clang/llvm into our build process just to compile a couple lines of eBPF code to do AF_XDP. Or even in the future, when we put more logic in eBPF/XDP program, I'd still like it to be eBPF binary, not C code. After all, the chance that OVS users understand eBPF and want to modify it is very low.
The current version of xdpsock_user + libbpf meets our requirement perfectly. I started with the old xdpsock_user.c and xdpsock_kern.c, and ported to OVS. I have to add clang/llvm support, copy/paste lots of code, and manage eBPF maps by myself. Switching to use libbpf makes the porting much easier and simpler. For an AF_XDP user, the current approach let them focus on the AF_XDP's API, the umem and rx/tx/fill/compl queue, without worrying about the eBPF/XDP program. On the other hand, I do agree that having xdpsock_kern.c has its benefit. It makes better understanding of how the underlying XDP program forwards packets to AF_XDP sockets. Regards, William > Another option is to make XSK support optional in libbpf, then the clang > dependency will be optional. > > Another (crazy) option is to make a separate libxsk that would depend on > libbpf and clang and contain only the XSK code. > > > Maybe > > that's ok? If that is added, I'd be happy do remove the raw BPF > > instructions. > > OK for me, for the reasons explained above. I don't know who else should > approve it. > > > Personally, I don't have a hard time reading a couple of > > lines of assembly > > Neither have I. But it doesn't mean we should read and write programs in > assembly. I'm perfectly able to understand and modify this XDP program, > but it's just not the way the programs should be written. I don't need > to explain why C is better here - you understand it yourself :) > > > but I see your point and agree. Having it as C-code > > would be better for the libbpf developers -- if the Clang build > > dependency is ok. > > > > > > > Let's make an "advanced user" sample as well, and add shared umem > > > > > support to libbpf! > > > > > > Why create another sample if we have this one? Actually, how making > > > another sample fixes this one? The issue is in libbpf anyway. It has a > > > blob inside with no tools to regenerate it from C sources. And this lib > > > is not even a sample, it can be used by real applications. Of course, it > > > should be editable, otherwise no new feature can be added (without > > > manually writing bytecode), and it's not the matter of shared UMEM. > > > > > > > Magnus and I took the route to simplify the sample, to make it easier > > for new users. I still think that was the right path. Should there be > > a sample showcasing all the knobs/pulleys? Sure. > > As Jonathan points, libbpf dictates the layout of xsks_map, so we won't > be able to provide an advanced example with shared UMEM unless we modify > libbpf as well. > > > > > > ...and as always, patches are very much welcome! > > > > > > Good approach - to drop a feature and wait until someone submits a patch > > > to restore it. And how do you imagine that patch that adds back shared > > > UMEM? The blob in libbpf has to be edited to accomplish this. It makes > > > unnecessary trouble for anyone trying to contribute. > > > > > > > I appreciate that you are looking into the code/design with > > constructive remarks -- but please refrain from being snarky. > > I'm sorry if it sounded snarky, I just wanted to convey my point. I > agree with moving the common AF_XDP code into a lib to make writing new > applications easier and to avoid repeating the same code. I don't think > it made the sample simpler though - there are more abstraction layers > now, and it became more generic. And, of course, we lost an important > piece of functionality. So, regarding the issue with the samples, I > think it would be good to have both the old and the new one > simultaneously. Together they would cover all needs: the old one > illustrates how to use AF_XDP and is good as sample code for newcomers, > because it's straightforward and not universal, and the new one > illustrates how to use libbpf to build the real applications, using the > generic abstractions libbpf provides. (It doesn't resolve the libbpf > issue with bytecode, but it's just my opinion on the samples). > > > > > Björn
