New xdpsock sample

New xdpsock sample

[Maxim Mikityanskiy]

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

Re: New xdpsock sample

[Jonathan Lemon]

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.

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.

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

Re: New xdpsock sample

[Magnus Karlsson]

On Tue, Mar 26, 2019 at 5:13 PM Jonathan Lemon <bsd@fb.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.
>
> 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.
>
> 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.
> --
> 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?
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

RE: New xdpsock sample

[Maxim Mikityanskiy]

> -----Original Message-----
> From: Magnus Karlsson <magnus.karlsson@gmail.com>
> Sent: 26 March, 2019 18:24
> To: Jonathan Lemon <bsd@fb.com>
> Cc: Maxim Mikityanskiy <maximmi@mellanox.com>; Magnus Karlsson
> <magnus.karlsson@intel.com>; netdev@vger.kernel.org; Björn Töpel
> <bjorn.topel@intel.com>; Daniel Borkmann <daniel@iogearbox.net>; Eran Ben
> Elisha <eranbe@mellanox.com>; Tariq Toukan <tariqt@mellanox.com>; Saeed
> Mahameed <saeedm@mellanox.com>
> Subject: Re: New xdpsock sample
>
> On Tue, Mar 26, 2019 at 5:13 PM Jonathan Lemon <bsd@fb.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,
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

Re: New xdpsock sample

[Björn Töpel]

On Wed, 27 Mar 2019 at 14:34, Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> > -----Original Message-----
> > From: Magnus Karlsson <magnus.karlsson@gmail.com>
> > Sent: 26 March, 2019 18:24
> > To: Jonathan Lemon <bsd@fb.com>
> > Cc: Maxim Mikityanskiy <maximmi@mellanox.com>; Magnus Karlsson
> > <magnus.karlsson@intel.com>; netdev@vger.kernel.org; Björn Töpel
> > <bjorn.topel@intel.com>; Daniel Borkmann <daniel@iogearbox.net>; Eran Ben
> > Elisha <eranbe@mellanox.com>; Tariq Toukan <tariqt@mellanox.com>; Saeed
> > Mahameed <saeedm@mellanox.com>
> > Subject: Re: New xdpsock sample
> >
> > On Tue, Mar 26, 2019 at 5:13 PM Jonathan Lemon <bsd@fb.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

Re: New xdpsock sample

[Magnus Karlsson]

On Wed, Mar 27, 2019 at 3:10 PM Björn Töpel <bjorn.topel@gmail.com> wrote:
>
> On Wed, 27 Mar 2019 at 14:34, Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
> >
> > > -----Original Message-----
> > > From: Magnus Karlsson <magnus.karlsson@gmail.com>
> > > Sent: 26 March, 2019 18:24
> > > To: Jonathan Lemon <bsd@fb.com>
> > > Cc: Maxim Mikityanskiy <maximmi@mellanox.com>; Magnus Karlsson
> > > <magnus.karlsson@intel.com>; netdev@vger.kernel.org; Björn Töpel
> > > <bjorn.topel@intel.com>; Daniel Borkmann <daniel@iogearbox.net>; Eran Ben
> > > Elisha <eranbe@mellanox.com>; Tariq Toukan <tariqt@mellanox.com>; Saeed
> > > Mahameed <saeedm@mellanox.com>
> > > Subject: Re: New xdpsock sample
> > >
> > > On Tue, Mar 26, 2019 at 5:13 PM Jonathan Lemon <bsd@fb.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

+1 for a shared umem sample app that uses a bpf program in C. Would be
very useful.

/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

RE: New xdpsock sample

[Maxim Mikityanskiy]

> -----Original Message-----
> From: Magnus Karlsson <magnus.karlsson@gmail.com>
> Sent: 27 March, 2019 16:19
> To: Björn Töpel <bjorn.topel@gmail.com>
> Cc: Maxim Mikityanskiy <maximmi@mellanox.com>; Magnus Karlsson
> <magnus.karlsson@intel.com>; Jonathan Lemon <bsd@fb.com>;
> netdev@vger.kernel.org; Björn Töpel <bjorn.topel@intel.com>; Daniel Borkmann
> <daniel@iogearbox.net>; Eran Ben Elisha <eranbe@mellanox.com>; Tariq Toukan
> <tariqt@mellanox.com>; Saeed Mahameed <saeedm@mellanox.com>
> Subject: Re: New xdpsock sample
> 
> On Wed, Mar 27, 2019 at 3:10 PM Björn Töpel <bjorn.topel@gmail.com> wrote:
> >
> > On Wed, 27 Mar 2019 at 14:34, Maxim Mikityanskiy <maximmi@mellanox.com>
> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Magnus Karlsson <magnus.karlsson@gmail.com>
> > > > Sent: 26 March, 2019 18:24
> > > > To: Jonathan Lemon <bsd@fb.com>
> > > > Cc: Maxim Mikityanskiy <maximmi@mellanox.com>; Magnus Karlsson
> > > > <magnus.karlsson@intel.com>; netdev@vger.kernel.org; Björn Töpel
> > > > <bjorn.topel@intel.com>; Daniel Borkmann <daniel@iogearbox.net>; Eran
> Ben
> > > > Elisha <eranbe@mellanox.com>; Tariq Toukan <tariqt@mellanox.com>;
> Saeed
> > > > Mahameed <saeedm@mellanox.com>
> > > > Subject: Re: New xdpsock sample
> > > >
> > > > On Tue, Mar 26, 2019 at 5:13 PM Jonathan Lemon <bsd@fb.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

Re: New xdpsock sample

[Björn Töpel]

On Thu, 28 Mar 2019 at 10:48, Maxim Mikityanskiy <maximmi@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. Maybe
that's ok? If that is added, I'd be happy do remove the raw BPF
instructions. Personally, I don't have a hard time reading a couple of
lines of assembly, 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.

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


Björn

Re: New xdpsock sample

[Jonathan Lemon]

> On Mar 28, 2019, at 5:35 AM, Björn Töpel <bjorn.topel@gmail.com> wrote:
> 
> 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? 

One problem I ran into yesterday is that the bpf program is dependent on the xsks_map and qidconf_map layout. There really isn’t a way to simply drop a new bpf program in without duplicating chunks of the sample code. So in that respect, the original code was clearer.
— 
Jonathan

RE: New xdpsock sample

[Maxim Mikityanskiy]

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

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

Re: New xdpsock sample

[William Tu]

On Fri, Mar 29, 2019 at 1:16 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> > -----Original Message-----
> > From: Björn Töpel <bjorn.topel@gmail.com>
> > Sent: 28 March, 2019 14:35
> > To: Maxim Mikityanskiy <maximmi@mellanox.com>
> > Cc: Magnus Karlsson <magnus.karlsson@gmail.com>; Magnus Karlsson
> > <magnus.karlsson@intel.com>; Björn Töpel <bjorn.topel@intel.com>; Jonathan
> > Lemon <bsd@fb.com>; netdev@vger.kernel.org; Daniel Borkmann
> > <daniel@iogearbox.net>; Eran Ben Elisha <eranbe@mellanox.com>; Tariq Toukan
> > <tariqt@mellanox.com>; Saeed Mahameed <saeedm@mellanox.com>
> > Subject: Re: New xdpsock sample
> >
> > On Thu, 28 Mar 2019 at 10:48, Maxim Mikityanskiy <maximmi@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