Re: [PATCH bpf-next v2 1/2] libbpf: add support for using AF_XDP sockets

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Mon, Dec 17, 2018 at 10:12:33AM +0100, Magnus Karlsson wrote:
> On Fri, Dec 14, 2018 at 9:25 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Wed, Dec 12, 2018 at 02:09:48PM +0100, Magnus Karlsson wrote:
> > > This commit adds AF_XDP support to libbpf. The main reason for
> > > this is to facilitate writing applications that use AF_XDP by offering
> > > higher-level APIs that hide many of the details of the AF_XDP
> > > uapi. This is in the same vein as libbpf facilitates XDP adoption by
> > > offering easy-to-use higher level interfaces of XDP
> > > functionality. Hopefully this will facilitate adoption of AF_XDP, make
> > > applications using it simpler and smaller, and finally also make it
> > > possible for applications to benefit from optimizations in the AF_XDP
> > > user space access code. Previously, people just copied and pasted the
> > > code from the sample application into their application, which is not
> > > desirable.
> > >
> > > The interface is composed of two parts:
> > >
> > > * Low-level access interface to the four rings and the packet
> > > * High-level control plane interface for creating and setting
> > >   up umems and af_xdp sockets.
> > >
> > > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > ...
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index 5f68d7b..da99203 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> >
> > may be instead of lib/bpf/libbpf.h the xsk stuff should go to lib/bpf/xsk.h ?
> 
> Yes. Good idea.
> 
> > > @@ -15,6 +15,7 @@
> > >  #include <stdbool.h>
> > >  #include <sys/types.h>  // for size_t
> > >  #include <linux/bpf.h>
> > > +#include <linux/if_xdp.h>
> > >
> > >  #ifdef __cplusplus
> > >  extern "C" {
> > > @@ -355,6 +356,98 @@ LIBBPF_API const struct bpf_line_info *
> > >  bpf_prog_linfo__lfind(const struct bpf_prog_linfo *prog_linfo,
> > >                     __u32 insn_off, __u32 nr_skip);
> > >
> > > +/* Do not access these members directly. Use the functions below. */
> > > +struct xsk_prod_ring {
> > > +     __u32 cached_prod;
> > > +     __u32 cached_cons;
> > > +     __u32 mask;
> > > +     __u32 size;
> > > +     __u32 *producer;
> > > +     __u32 *consumer;
> > > +     void *ring;
> > > +};
> > > +
> > > +/* Do not access these members directly. Use the functions below. */
> > > +struct xsk_cons_ring {
> > > +     __u32 cached_prod;
> > > +     __u32 cached_cons;
> > > +     __u32 mask;
> > > +     __u32 size;
> > > +     __u32 *producer;
> > > +     __u32 *consumer;
> > > +     void *ring;
> > > +};
> >
> > xsk_prod_ring and xsk_cons_ring have exactly the same members,
> > but they're two different structs? why?
> > May be have one 'struct xsk_ring' ?
> 
> They operate on the same ring but represents either the producer or
> the consumer of that same ring. The reason for this is that I want to
> make sure that the user gets a compile time error if he or she tries
> to use for example the function xsk__reserve_prod when user space is a
> consumer of that ring (and the same kind of argument for
> xsk__submit_prod, xsk__peek_cons, and xsk__release_cons). If we move
> to a xsk_ring, we lose this compile time check, or is there another
> better way of doing this in C without getting double definitions or
> using hard to read #defines? The only benefit I see with going to
> xsk_ring is that we get rid of two small inline functions and one
> struct definition in the header file, but the code size will be the
> same. Personally I prefer compile time error checking. But let me know
> what to proceed with.

how about the following?
struct xsk_ring {...};
struct xsk_prod {
  struct xsk_ring r;
};
struct xsk_cons {
  struct xsk_ring r;
};
and compiler will warn when xsk_prod is used instead of xsk_cons.
The methods can be called xsk_prod__* and xsk_cons__*

Also do 'producer' and 'consumer' names really fit?
Typically producer and consumer are two sides of the single ring.