From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52606) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VdN7m-0001tt-L7 for qemu-devel@nongnu.org; Mon, 04 Nov 2013 11:35:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VdN7f-0006tA-Jp for qemu-devel@nongnu.org; Mon, 04 Nov 2013 11:35:34 -0500 Received: from mail-ee0-x230.google.com ([2a00:1450:4013:c00::230]:37151) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VdN7e-0006s8-O6 for qemu-devel@nongnu.org; Mon, 04 Nov 2013 11:35:27 -0500 Received: by mail-ee0-f48.google.com with SMTP id d49so1227603eek.21 for ; Mon, 04 Nov 2013 08:35:25 -0800 (PST) Date: Mon, 4 Nov 2013 17:35:02 +0100 From: Stefan Hajnoczi Message-ID: <20131104163502.GD19804@stefanha-thinkpad.redhat.com> References: <1383041545-16797-1-git-send-email-v.maffione@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1383041545-16797-1-git-send-email-v.maffione@gmail.com> Subject: Re: [Qemu-devel] [PATCH v3] net: Adding netmap network backend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vincenzo Maffione Cc: peter.maydell@linaro.org, stefanha@redhat.com, mtosatti@redhat.com, mjt@tls.msk.ru, qemu-devel@nongnu.org, armbru@redhat.com, aliguori@amazon.com, pbonzini@redhat.com, g.lettieri@iet.unipi.it, lcapitulino@redhat.com, rizzo@iet.unipi.it, rth@twiddle.net On Tue, Oct 29, 2013 at 11:12:25AM +0100, Vincenzo Maffione wrote: Looks pretty good, I think we can merge the next revision. > This patch only contains the simplest netmap backend for QEMU. > In particular, this backend implementation is still not > able to make use of batching on the TX side (frontend -> backend), > which is where most of the TX performance gain comes from. > As you can see from the code, there is an ioctl(NIOCTXSYNC) for each > packet, instead of an ioctl(NIOCTXSYNC) for a batch of packets. > In order to make TX batching possible, we would need to do some > modifications to the generic net/net.c code, adding to the > frontend/backend datapath interface a way to send a batch (this can > be done using a QEMU_NET_PACKET_FLAG_MORE, without changing too > much the existing interface). > We will propose these features in future patches. QEMU_NET_PACKET_FLAG_MORE sounds like a good idea. > +#include "net/net.h" > +#include "clients.h" > +#include "sysemu/sysemu.h" > +#include "qemu/error-report.h" > + > +#include > +#include > +#include > +#include > +#include > +#include Please include system headers first, then QEMU headers. This way QEMU headers cannot interfere with system headers if they define macros. Also, should be "qemu/iov.h": #include #include #include #include #include #include "net/net.h" #include "clients.h" #include "sysemu/sysemu.h" #include "qemu/error-report.h" #include "qemu/iov.h" > +/* > + * Open a netmap device. We assume there is only one queue > + * (which is the case for the VALE bridge). > + */ > +static int netmap_open(NetmapPriv *me) > +{ > + int fd; > + int err; > + size_t 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; This error message is not detailed enough, please include the errstr(3). > + } > + bzero(&req, sizeof(req)); Please use memset(&req, 0, sizeof(req)). Some compilers/tools warn that bzero() is deprecated. For more info, see: http://pubs.opengroup.org/onlinepubs/009695399/functions/bzero.html > + pstrcpy(req.nr_name, sizeof(req.nr_name), me->ifname); > + req.nr_ringid = NETMAP_NO_TX_POLL; > + req.nr_version = NETMAP_API; > + err = ioctl(fd, NIOCREGIF, &req); > + if (err) { > + error_report("Unable to register %s", me->ifname); Lacking errno information here too. > + goto error; > + } > + l = me->memsize = req.nr_memsize; > + > + me->mem = mmap(0, l, PROT_WRITE | PROT_READ, MAP_SHARED, fd, 0); > + if (me->mem == MAP_FAILED) { > + error_report("Unable to mmap"); Lacking errno information here too. > +static ssize_t netmap_receive_iov(NetClientState *nc, > + const struct iovec *iov, int iovcnt) > +{ > + NetmapState *s = DO_UPCAST(NetmapState, nc, nc); > + struct netmap_ring *ring = s->me.tx; > + > + if (ring) { Feel free to reduce indentation by returning early: if (!ring) { return iov_size(iov, iovcnt); /* drop */ } > + uint32_t i = 0; > + uint32_t idx; > + uint8_t *dst; > + int j; > + uint32_t cur = ring->cur; > + uint32_t avail = ring->avail; > + int iov_frag_size; > + int nm_frag_size; > + int offset; > + > + if (avail < iovcnt) { > + /* Not enough netmap slots. */ > + netmap_write_poll(s, true); > + return 0; > + } > + > + for (j = 0; j < iovcnt; j++) { > + iov_frag_size = iov[j].iov_len; > + offset = 0; > + > + /* Split each iovec fragment over more netmap slots, if > + necessary (without performing data copy). */ I don't understand this comment, we always perform data copy. > + while (iov_frag_size) { > + nm_frag_size = MIN(iov_frag_size, ring->nr_buf_size); > + > + if (unlikely(avail == 0)) { > + /* We run out of netmap slots while splitting the > + iovec fragments. */ > + return 0; netmap_write_poll(s, true) missing?