Date:   Wed, 27 Mar 2019 15:10:16 +0100
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Maxim Mikityanskiy <maximmi@...lanox.com>
Cc:     Magnus Karlsson <magnus.karlsson@...il.com>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Jonathan Lemon <bsd@...com>,
        "netdev@...r.kernel.org" <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, 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.

Let's make an "advanced user" sample as well, and add shared umem
support to libbpf! ...and as always, patches are very much welcome!


Thanks,
Björn


> 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