Message-ID: <AM6PR05MB5879549CFF3A8740FBF49251D1590@AM6PR05MB5879.eurprd05.prod.outlook.com>
Date:   Thu, 28 Mar 2019 09:48:26 +0000
From:   Maxim Mikityanskiy <maximmi@...lanox.com>
To:     Magnus Karlsson <magnus.karlsson@...il.com>,
        Björn Töpel <bjorn.topel@...il.com>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn.topel@...el.com>
CC:     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: Magnus Karlsson <magnus.karlsson@...il.com>
> Sent: 27 March, 2019 16:19
> To: Björn Töpel <bjorn.topel@...il.com>
> Cc: Maxim Mikityanskiy <maximmi@...lanox.com>; Magnus Karlsson
> <magnus.karlsson@...el.com>; Jonathan Lemon <bsd@...com>;
> netdev@...r.kernel.org; Björn Töpel <bjorn.topel@...el.com>; 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 Wed, Mar 27, 2019 at 3:10 PM Björn Töpel <bjorn.topel@...il.com> wrote:
> >
> > On Wed, 27 Mar 2019 at 14:34, Maxim Mikityanskiy <maximmi@...lanox.com>
> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Magnus Karlsson <magnus.karlsson@...il.com>
> > > > Sent: 26 March, 2019 18:24
> > > > To: Jonathan Lemon <bsd@...com>
> > > > Cc: Maxim Mikityanskiy <maximmi@...lanox.com>; Magnus Karlsson
> > > > <magnus.karlsson@...el.com>; netdev@...r.kernel.org; Björn Töpel
> > > > <bjorn.topel@...el.com>; 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 Tue, Mar 26, 2019 at 5:13 PM Jonathan Lemon <bsd@...com> wrote:
> > > > >
> > > > > The rationale (IIRC) was that it would be easier for new users to
> > > > > get started using AF_XDP by providing everything that was needed
> > > > > by default.
> > >
> > > Well, no matter whether the XDP program is compiled separately or
> > > hardcoded as bytecode, it's libbpf's implementation details, and a new
> > > user shouldn't notice any difference in usage.
> > >
> > > However, when the user is no longer new and is not satisfied with the
> > > sample application, they should be able to tweak it. If the sample is
> > > not modifiable, the user is forced to rewrite all the code. The
> > > threshold of entry is low, but then you have to jump a huge step to
> > > start doing something not included into the sample. It doesn't make
> > > sense to me when there is an option to have a modifiable sample without
> > > increasing the threshold of entry which makes further fiddling with the
> > > sample easier.
> > >
> > > > > Passing in XSK_LIBBPF_FLAGS__INHIBIT_PROG to the library will
> > > > > bypass loading the sample program, so a user application may still
> > > > > use the library with their own bpf program.
> > >
> > > Yes, thanks, but it's not what I want, see below.
> > >
> > > > > I'll admit that the change likely makes it harder to simply modify
> > > > > the sample program for other uses, but that's not really the point
> > > > > of the samples.
> > >
> > > I'm not trying to adapt the sample to transform it to some real world
> > > application. But the ability to tinker with sample code is vital to get
> > > understanding on how the feature works. This is the point of samples.
> > >
> > > > > --
> > > > > Jonathan
> > > > >
> > > > > On 26 Mar 2019, at 8:46, Maxim Mikityanskiy wrote:
> > > > >
> > > > > > Hi Magnus and all,
> > > > > >
> > > > > > https://patchwork.ozlabs.org/cover/1045921/
> > > > > >
> > > > > > This series removes xdpsock_kern.c and replaces it by the bytecode
> > > > > > hardcoded in libbpf. I am wondering whether there is some real
> issue
> > > > > > with having the XDP program in a separate C file, just like
> before,
> > > > > > because this change made it far less convenient to modify the XDP
> > > > > > program. Could you give any comments?
> > > > > >
> > > > > > Thanks,
> > > > > > Max
> > > >
> > > > How about we reintroduce a sample C XDP program once we have a reason
> > > > to use one in the xdpsock program, i.e. for something not covered by
> > > > libbpf? I do not have such a use case at the moment, but do you Max?
> > >
> > > Even at the moment the XDP program hardcoded into libbpf doesn't support
> > > shared UMEMs that used to be supported in the old xdpsock. If this
> > > feature is added at some point, it will require modifying both the XDP
> > > program and libbpf. It's an obvious example of a thing not covered by
> > > libbpf.
> > >
> > > There are also two reasons to ship the C code of the XDP program:
> > >
> > > 1. First of all, it's a sample. When someone starts looking at it, they
> > > may want to make some modifications to understand it better. It may not
> > > be enough to just look at the comment above.
> > >
> > > 2. The AF_XDP feature is evolving. Some new things may appear worth
> > > showing in the sample. I want to highlight that I'm not talking about
> > > the case when someone takes xdpsock+libbpf and tries to fit it to their
> > > needs. It's all about putting the reference implementation of new AF_XDP
> > > features to the sample. These features may require modification of both
> > > libbpf and the XDP program.
> > >
> > > In any case, the repository should contain source code and tools to
> > > build it, not binaries. BPF bytecode is not the source code, unless it
> > > was written manually, but the C code in the comment above proves the
> > > opposite. Everyone should be able to modify the code and to rebuild it.
> > > I pointed out three real cases (showing the reference implementation of
> > > shared UMEMs, fiddling with the sample while learning it, adding future
> > > features) when modification of the code is necessary, and other people
> > > may have their own motivations to modify the code.
> > >
> >
> > 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.

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

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

> >
> >
> > Thanks,
> > Björn
> 
> +1 for a shared umem sample app that uses a bpf program in C. Would be
> very useful.

...and we already had one - the old xdpsock.

> /Magnus
> 
> > > Thanks,
> > > Max
> > >
> > > > If so, as you say, it would be good to have an example on how to
> > > > accomplish this using the XSK_LIBBPF_FLAGS__INHIBIT_PROG that Jonathan
> > > > mentioned.
> > > >
> > > > /Magnus

Powered by blists - more mailing lists