Message-ID: <AM6PR05MB587971AEEABBFCE0546704CBD15A0@AM6PR05MB5879.eurprd05.prod.outlook.com>
Date:   Fri, 29 Mar 2019 08:13:34 +0000
From:   Maxim Mikityanskiy <maximmi@...lanox.com>
To:     Björn Töpel <bjorn.topel@...il.com>
CC:     Magnus Karlsson <magnus.karlsson@...il.com>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        Jonathan Lemon <bsd@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Eran Ben Elisha <eranbe@...lanox.com>,
        Tariq Toukan <tariqt@...lanox.com>,
        Saeed Mahameed <saeedm@...lanox.com>
Subject: RE: New xdpsock sample

> -----Original Message-----
> From: Björn Töpel <bjorn.topel@...il.com>
> Sent: 28 March, 2019 14:35
> To: Maxim Mikityanskiy <maximmi@...lanox.com>
> Cc: Magnus Karlsson <magnus.karlsson@...il.com>; Magnus Karlsson
> <magnus.karlsson@...el.com>; Björn Töpel <bjorn.topel@...el.com>; Jonathan
> Lemon <bsd@...com>; netdev@...r.kernel.org; Daniel Borkmann
> <daniel@...earbox.net>; Eran Ben Elisha <eranbe@...lanox.com>; Tariq Toukan
> <tariqt@...lanox.com>; Saeed Mahameed <saeedm@...lanox.com>
> Subject: Re: New xdpsock sample
> 
> On Thu, 28 Mar 2019 at 10:48, Maxim Mikityanskiy <maximmi@...lanox.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?

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

Powered by blists - more mailing lists