Re: [Qemu-devel] [PATCH v2] netmap backend (revised)

[Stefan Hajnoczi]

On Tue, Jan 22, 2013 at 08:12:15AM +0100, Luigi Rizzo wrote:
> reposting a version without changes that implement bounded
> queues in net/queue.c
> 
> Hi,
> the attached patch implements a qemu backend for the "netmap" API
> thus allowing machines to attach to the VALE software switch as
> well as netmap-supported cards (links below).
> 
> http://info.iet.unipi.it/~luigi/netmap/
> http://info.iet.unipi.it/~luigi/vale/
> 
> This is a cleaned up version of code written last summer.

Looks like a clean software approach to low-level packet I/O.  I guess
the biggest competitor would be a userspace library with NIC drivers
using modern IOMMUs to avoid the security/reliability problem that
previous userspace approaches suffered.  Pretty cool that netmap reuses
kernel NIC driver implementations to avoid duplicating all that code.

I wonder how/if netmaps handles advanced NIC features like multiqueue
and offloads?  But I've only read the webpage, not the papers or source
code :).

> diff --git a/net/Makefile.objs b/net/Makefile.objs
> index a08cd14..068253f 100644
> --- a/net/Makefile.objs
> +++ b/net/Makefile.objs
> @@ -10,3 +10,4 @@ common-obj-$(CONFIG_AIX) += tap-aix.o
>  common-obj-$(CONFIG_HAIKU) += tap-haiku.o
>  common-obj-$(CONFIG_SLIRP) += slirp.o
>  common-obj-$(CONFIG_VDE) += vde.o
> +common-obj-$(CONFIG_NETMAP) += qemu-netmap.o

Please drop the "qemu-" prefix.

> diff --git a/net/net.c b/net/net.c
> index cdd9b04..816c987 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -618,6 +618,9 @@ static int (* const 
> net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])(
>          [NET_CLIENT_OPTIONS_KIND_BRIDGE]    = net_init_bridge,
>  #endif
>          [NET_CLIENT_OPTIONS_KIND_HUBPORT]   = net_init_hubport,
> +#ifdef CONFIG_NETMAP
> +     [NET_CLIENT_OPTIONS_KIND_NETMAP]    = net_init_netmap,
> +#endif

Please use 4 spaces for indentation and run scripts/checkpatch.pl to
scan patches.

>  };
>  
>  
> diff --git a/net/qemu-netmap.c b/net/qemu-netmap.c
> new file mode 100644
> index 0000000..79d7c09
> --- /dev/null
> +++ b/net/qemu-netmap.c
> @@ -0,0 +1,353 @@
> +/*
> + * netmap access for qemu
> + *
> + * Copyright (c) 2012-2013 Luigi Rizzo
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "config-host.h"
> +
> +/* note paths are different for -head and 1.3 */

Please drop this comment from the patch.

> +#define ND(fd, ... ) // debugging
> +#define D(format, ...)                                          \
> +        do {                                                    \
> +                struct timeval __xxts;                          \
> +                gettimeofday(&__xxts, NULL);                    \
> +                printf("%03d.%06d %s [%d] " format "\n",        \
> +                (int)__xxts.tv_sec % 1000, (int)__xxts.tv_usec, \
> +                __FUNCTION__, __LINE__, ##__VA_ARGS__);         \
> +        } while (0)
> +
> +/* rate limited, lps indicates how many per second */
> +#define RD(lps, format, ...)                                    \
> +        do {                                                    \
> +                static int t0, __cnt;                           \
> +                struct timeval __xxts;                          \
> +                gettimeofday(&__xxts, NULL);                    \
> +                if (t0 != __xxts.tv_sec) {                      \
> +                        t0 = __xxts.tv_sec;                     \
> +                        __cnt = 0;                              \
> +                }                                               \
> +                if (__cnt++ < lps)                              \
> +                        D(format, ##__VA_ARGS__);               \
> +        } while (0)

Have you seen docs/tracing.txt?  It can do fprintf(stderr) but also
SystemTap/DTrace and a simple built-in binary tracer.

> +
> +
> +
> +/*
> + * private netmap device info
> + */
> +struct netmap_state {

QEMU code should use:

typedef struct {
    ...
} NetmapState;

See ./HACKING and ./CODING_STYLE.  The code usually avoids struct tags.

> +    int                      fd;
> +    int                      memsize;

size_t?

> +    void             *mem;
> +    struct netmap_if *nifp;
> +    struct netmap_ring       *rx;
> +    struct netmap_ring       *tx;
> +    char             fdname[128];    /* normally /dev/netmap */

PATH_MAX?

> +    char             ifname[128];    /* maybe the nmreq here ? */

IFNAMSIZ?

> +};
> +
> +struct nm_state {
> +    NetClientState   nc;
> +    struct netmap_state      me;
> +    unsigned int     read_poll;
> +    unsigned int     write_poll;

bool for read_poll and write_poll.

> +};
> +
> +// a fast copy routine only for multiples of 64 bytes, non overlapped.
> +static inline void
> +pkt_copy(const void *_src, void *_dst, int l)
> +{
> +        const uint64_t *src = _src;
> +        uint64_t *dst = _dst;
> +#define likely(x)       __builtin_expect(!!(x), 1)
> +#define unlikely(x)       __builtin_expect(!!(x), 0)

Already defined in include/qemu/osdep.h.

> +        if (unlikely(l >= 1024)) {
> +                bcopy(src, dst, l);
> +                return;
> +        }
> +        for (; l > 0; l -= 64) {
> +                *dst++ = *src++;
> +                *dst++ = *src++;
> +                *dst++ = *src++;
> +                *dst++ = *src++;
> +                *dst++ = *src++;
> +                *dst++ = *src++;
> +                *dst++ = *src++;
> +                *dst++ = *src++;
> +        }
> +}

I wonder how different FreeBSD bcopy() is from glibc memcpy() and if the
optimization is even a win.  The glibc code is probably hand-written
assembly that CPU vendors have contributed for specific CPU model
families.

Did you compare glibc memcpy() against pkt_copy()?

> +
> +
> +/*
> + * open a netmap device. We assume there is only one queue
> + * (which is the case for the VALE bridge).
> + */
> +static int netmap_open(struct netmap_state *me)
> +{
> +    int fd, l, err;

Please use "size_t len" instead of "int l".

> +    struct nmreq req;
> +
> +    me->fd = fd = open(me->fdname, O_RDWR);
> +    if (fd < 0) {
> +     error_report("Unable to open netmap device '%s'", me->fdname);
> +     return -1;
> +    }
> +    bzero(&req, sizeof(req));
> +    pstrcpy(req.nr_name, sizeof(req.nr_name), me->ifname);
> +    req.nr_ringid = 0;
> +    req.nr_version = NETMAP_API;
> +    err = ioctl(fd, NIOCGINFO, &req);
> +    if (err) {
> +     error_report("cannot get info on %s", me->ifname);
> +     goto error;
> +    }
> +    l = me->memsize = req.nr_memsize;
> +    err = ioctl(fd, NIOCREGIF, &req);
> +    if (err) {
> +     error_report("Unable to register %s", me->ifname);
> +     goto error;
> +    }
> +
> +    me->mem = mmap(0, l, PROT_WRITE | PROT_READ, MAP_SHARED, fd, 0);
> +    if (me->mem == MAP_FAILED) {
> +     error_report("Unable to mmap");
> +     me->mem = NULL;
> +     goto error;
> +    }
> +
> +    me->nifp = NETMAP_IF(me->mem, req.nr_offset);
> +    me->tx = NETMAP_TXRING(me->nifp, 0);
> +    me->rx = NETMAP_RXRING(me->nifp, 0);
> +    return 0;
> +
> +error:
> +    close(me->fd);
> +    return -1;
> +}
> +
> +// XXX do we need the can-send routine ?
> +static int netmap_can_send(void *opaque)
> +{
> +    struct nm_state *s = opaque;
> +
> +    return qemu_can_send_packet(&s->nc);
> +}

Yes, I think it is a good idea.  We only receive packets if our peer can
also receive them.  (Why we still need queues is another question but at
least .read_poll() should be implemented IMO.)

> +
> +static void netmap_send(void *opaque);
> +static void netmap_writable(void *opaque);
> +
> +/*
> + * set the handlers for the device
> + */
> +static void netmap_update_fd_handler(struct nm_state *s)
> +{
> +#if 1
> +    qemu_set_fd_handler2(s->me.fd,
> +                         s->read_poll  ? netmap_can_send : NULL,
> +                         s->read_poll  ? netmap_send     : NULL,
> +                         s->write_poll ? netmap_writable : NULL,
> +                         s);
> +#else
> +    qemu_set_fd_handler(s->me.fd,
> +                         s->read_poll  ? netmap_send     : NULL,
> +                         s->write_poll ? netmap_writable : NULL,
> +                         s);
> +#endif

Please drop the #if.

> +}
> +
> +// update the read handler
> +static void netmap_read_poll(struct nm_state *s, int enable)

bool enable

> +{
> +    if (s->read_poll != enable) { /* do nothing if not changed */
> +     s->read_poll = enable;
> +     netmap_update_fd_handler(s);
> +    }
> +}
> +
> +// update the write handler
> +static void netmap_write_poll(struct nm_state *s, int enable)

bool enable

> +{
> +    if (s->write_poll != enable) {
> +     s->write_poll = enable;
> +     netmap_update_fd_handler(s);
> +    }
> +}
> +
> +/*
> + * the fd_write() callback, invoked if the fd is marked as
> + * writable after a poll. Reset the handler and flush any
> + * buffered packets.
> + */
> +static void netmap_writable(void *opaque)
> +{
> +    struct nm_state *s = opaque;
> +
> +    netmap_write_poll(s, 0);
> +    qemu_flush_queued_packets(&s->nc);
> +}

It feels like the net subsystem is missing an opportunity to extract
common code for file descriptor readability/writability.  We're
basically duplicating the code from tap.c.

> +
> +/*
> + * new data guest --> backend
> + */
> +static ssize_t netmap_receive_raw(NetClientState *nc, const uint8_t *buf, 
> size_t size)
> +{
> +    struct nm_state *s = DO_UPCAST(struct nm_state, nc, nc);
> +    struct netmap_ring *ring = s->me.tx;
> +
> +    if (ring) {
> +     /* request an early notification to avoid running dry */
> +     if (ring->avail < ring->num_slots / 2 && s->write_poll == 0) {
> +         netmap_write_poll(s, 1);
> +     }
> +     if (ring->avail == 0) { // cannot write
> +         return 0;
> +     }
> +     uint32_t i = ring->cur;
> +     uint32_t idx = ring->slot[i].buf_idx;
> +     uint8_t *dst = (u_char *)NETMAP_BUF(ring, idx);

Why cast to u_char instead of uint8_t directly?

> +
> +     ring->slot[i].len = size;
> +     pkt_copy(buf, dst, size);

How does netmap guarantee that the buffer is large enough for this
packet?

> +     ring->cur = NETMAP_RING_NEXT(ring, i);
> +     ring->avail--;
> +    }
> +    return size;
> +}
> +
> +// complete a previous send (backend --> guest), enable the fd_read callback

Please use /* */ instead of //.  I'm not sure if we strictly enforce it
but it's rare to see // in QEMU.

> +static void netmap_send(void *opaque)
> +{
> +    struct nm_state *s = opaque;
> +    int sent = 0;
> +    struct netmap_ring *ring = s->me.rx;
> +
> +    /* only check ring->avail, let the packet be queued
> +     * with qemu_send_packet_async() if needed
> +     * XXX until we fix the propagation on the bridge we need to stop early
> +     */
> +    while (ring->avail > 0 && qemu_can_send_packet(&s->nc) ) {
> +     uint32_t i = ring->cur;
> +     uint32_t idx = ring->slot[i].buf_idx;
> +     uint8_t *src = (u_char *)NETMAP_BUF(ring, idx);
> +     int size = ring->slot[i].len;
> +
> +     ring->cur = NETMAP_RING_NEXT(ring, i);
> +     ring->avail--;
> +     sent++;

sent is unused?

> +static void netmap_cleanup(NetClientState *nc)
> +{
> +    struct nm_state *s = DO_UPCAST(struct nm_state, nc, nc);
> +
> +    qemu_purge_queued_packets(nc);
> +
> +    netmap_read_poll(s, 0);
> +    netmap_write_poll(s, 0);

These could be replaced with a single call to netmap_poll(nc, false).

> +    close(s->me.fd);

Missing munmap?

> +int net_init_netmap(const NetClientOptions *opts, const char *name, 
> NetClientState *peer)
> +{
> +    const NetdevNetmapOptions *netmap_opts = opts->netmap;
> +    NetClientState *nc;
> +    struct netmap_state me;
> +    struct nm_state *s;
> +
> +    pstrcpy(me.fdname, sizeof(me.fdname), name ? name : "/dev/netmap");
> +    /* set default name for the port if not supplied */
> +    pstrcpy(me.ifname, sizeof(me.ifname),
> +     netmap_opts->has_ifname ? netmap_opts->ifname : "vale0");
> +    if (netmap_open(&me))

QEMU coding style uses braces, even when the if body is only one line.

> +     return -1;
> +
> +    /* create the object -- XXX use name or ifname ? */

The name should be neither the filename nor the ifname.  It's a separate
namespace and the user gets to decide the name.