Date:   Mon, 17 Dec 2018 10:12:33 +0100
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Magnus Karlsson <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        ast@...nel.org, Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Björn Töpel <bjorn.topel@...il.com>,
        "Zhang, Qi Z" <qi.z.zhang@...el.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>
Subject: Re: [PATCH bpf-next v2 1/2] libbpf: add support for using AF_XDP sockets

On Fri, Dec 14, 2018 at 9:25 PM Alexei Starovoitov
<alexei.starovoitov@...il.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@...el.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.

> > +
> > +static inline __u64 *xsk__get_fill_desc(struct xsk_prod_ring *fill,
> > +                                    __u64 idx)
>
> see tools/lib/bpf/README.rst
> the main idea is for "objects" use __ to separate class vs method.
> In this case 'struct xsk_ring' would be an object and
> the name of the method would be:
> static inline __u64 *xsk_ring__get_fill_desc(struct xsk_ring *fill, __u64 idx)

Got it. Will fix.

> > +{
> > +     __u64 *descs = (__u64 *)fill->ring;
> > +
> > +     return &descs[idx & fill->mask];
> > +}
> > +
> > +static inline __u64 *xsk__get_completion_desc(struct xsk_cons_ring *comp,
> > +                                          __u64 idx)
> > +{
> > +     __u64 *descs = (__u64 *)comp->ring;
> > +
> > +     return &descs[idx & comp->mask];
> > +}
> > +
> > +static inline struct xdp_desc *xsk__get_tx_desc(struct xsk_prod_ring *tx,
> > +                                            __u64 idx)
> > +{
> > +     struct xdp_desc *descs = (struct xdp_desc *)tx->ring;
> > +
> > +     return &descs[idx & tx->mask];
> > +}
> > +
> > +static inline struct xdp_desc *xsk__get_rx_desc(struct xsk_cons_ring *rx,
> > +                                            __u64 idx)
> > +{
> > +     struct xdp_desc *descs = (struct xdp_desc *)rx->ring;
> > +
> > +     return &descs[idx & rx->mask];
> > +}
> > +
> > +LIBBPF_API size_t xsk__peek_cons(struct xsk_cons_ring *ring, size_t nb,
> > +                             __u32 *idx);
> > +LIBBPF_API void xsk__release_cons(struct xsk_cons_ring *ring);
> > +LIBBPF_API size_t xsk__reserve_prod(struct xsk_prod_ring *ring, size_t nb,
> > +                                __u32 *idx);
> > +LIBBPF_API void xsk__submit_prod(struct xsk_prod_ring *ring);
>
> if we combine the struct names then above could be:
>
> LIBBPF_API size_t xsk_ring__reserve(struct xsk_ring *ring, size_t nb, __u32 *idx);
> LIBBPF_API void xsk_ring__submit(struct xsk_ring *ring);

The implementations of xsk__peek_cons and and xsk__reserve_prod are
different because one performs producer operations and the other
consumer ones, so they cannot be combined (well at least not without
some added if statements and state that would impact performance).
Same for xsk__release_cons and xsk__submit_prod. But the static inline
functions could be reduced from four to two with an xsk_ring struct.

>
> > +
> > +LIBBPF_API void *xsk__get_data(void *umem_area, __u64 addr);
> > +
> > +#define XSK__DEFAULT_NUM_DESCS      2048
> > +#define XSK__DEFAULT_FRAME_SHIFT    11 /* 2048 bytes */
> > +#define XSK__DEFAULT_FRAME_SIZE     (1 << XSK__DEFAULT_FRAME_SHIFT)
> > +#define XSK__DEFAULT_FRAME_HEADROOM 0
> > +
> > +struct xsk_umem_config {
> > +     __u32 fq_size;
> > +     __u32 cq_size;
> > +     __u32 frame_size;
> > +     __u32 frame_headroom;
> > +};
> > +
> > +struct xsk_xdp_socket_config {
> > +     __u32 rx_size;
> > +     __u32 tx_size;
> > +};
> > +
> > +/* Set config to XSK_DEFAULT_CONFIG to get the default configuration. */
> > +LIBBPF_API int xsk__create_umem(void *umem_area, __u64 size,
> > +                            struct xsk_prod_ring *fq,
> > +                            struct xsk_cons_ring *cq,
> > +                            struct xsk_umem_config *config);
>
> this one looks too low level.
> espcially considering it's usage:
> umem->fd = xsk__create_umem(buffer, size, &umem->fq, &umem->cq, NULL);
>
> may be create an object "struct xsk_umem" ?
> then api will be:
> err = xsk_umem__create(buffer, size, NULL) ?

Makes sense considering the other functions in the library. Will do.

> > +LIBBPF_API int xsk__create_xdp_socket(int umem_fd, struct xsk_cons_ring *rx,
> > +                                  struct xsk_prod_ring *tx,
> > +                                  struct xsk_xdp_socket_config *config);
>
> similar concern here. feels that implementation details are leaking into api.
> The usage of it is:
> xsk->fd = xsk__create_xdp_socket(umem->fd, &xsk->rx, &xsk->tx, NULL);
>
> may be create an object "struct xsk_socket" ?

Yes to this also. I will add a function xsk_socket__fd(struct
xsk_socket) that returns the fd when the user needs it (for bind, poll
and others), in the same spirit as other functionality in the library.
Protest if you do not agree.

Thanks: Magnus

Powered by blists - more mailing lists