Re: [PATCH] Test netmap on FreeBSD

[Vincenzo Maffione]

This is an example command line for a qemu VM with a vtnet interface
connected to a port of a VALE switch called 'vale1':

qemu-system-x86_64 fbsd-head.qcow2 -smp 2 -m 2G -vga std -device
e1000,netdev=mgmt,mac=00:AA:BB:CC:0a:99 -netdev
user,id=mgmt,hostfwd=tcp::20010-:22 -device
virtio-net-pci,netdev=data10,mac=00:AA:BB:CC:0a:0a,ioeventfd=on,mrg_rxbuf=on
-netdev netmap,ifname=vale1:10,id=data10

Cheers,
  Vincenzo

Re: [PATCH] Test netmap on FreeBSD

[Vincenzo Maffione]

Il giorno mar 8 ott 2019 alle ore 11:53 Li-Wen Hsu  ha
scritto:

> On Tue, Oct 8, 2019 at 5:49 PM Stefano Garzarella 
> wrote:
> >
> > Hi Thomas,
> >
> > On Tue, Oct 8, 2019 at 10:49 AM Thomas Huth  wrote:
> > >
> > > FreeBSD offers a package for "netmap", thus let's install it in our
> > > FreeBSD tests to get some compile test coverage for net/netmap.c, too.
> > >
> > > Signed-off-by: Thomas Huth 
> > > ---
> > >  .cirrus.yml  | 2 +-
> > >  tests/vm/freebsd | 3 +++
> > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/.cirrus.yml b/.cirrus.yml
> > > index 8326a3a4b1..44fea2c29e 100644
> > > --- a/.cirrus.yml
> > > +++ b/.cirrus.yml
> > > @@ -8,7 +8,7 @@ freebsd_12_task:
> > >  memory: 8G
> > >install_script: pkg install -y
> > >  bash bison curl cyrus-sasl git glib gmake gnutls gsed
> > > -nettle perl5 pixman pkgconf png usbredir
> > > +netmap nettle perl5 pixman pkgconf png usbredir
> >
> > >script:
> > >  - mkdir build
> > >  - cd build
> > > diff --git a/tests/vm/freebsd b/tests/vm/freebsd
> > > index 2a19461a90..45821702bd 100755
> > > --- a/tests/vm/freebsd
> > > +++ b/tests/vm/freebsd
> > > @@ -54,6 +54,9 @@ class FreeBSDVM(basevm.BaseVM):
> > >  # libs: opengl
> > >  "libepoxy",
> > >  "mesa-libs",
> > > +
> > > +# libs: network
> > > +"netmap",
> >
> > IIRC netmap is shipped with the FreeBSD kernel, so we shouldn't need to
> > install it.
> >
> > I tried to understand what is the 'netmap' pkg and I found the following
> > in my FreeBSD 12 VM:
> >
> > # pkg search netmap
> > netmap-0.1.3_1 Make a graphical representation of the
> surrounding network
> >
> > I don't think we are installing the right 'netmap' (I know, it is a bit
> > confusing :) )
> >
> > I'll tried "make vm-build-freebsd" without this patch and I noticed
> > "netmap supportyes" in the configure output. So I think we're
> > already compiling the netmap backend in the 'vm-buil-freebsd' target and
> > maybe also in cirrus.
> >
> > Maybe Ed and Li-Wen can confirm (or not).
>
> Yes netmap(4) is a built-in and default feature on FreeBSD. netmap
> package from ports is not related to it.
>
> BTW, I haven't tried qemu's netmap feature on FreeBSD.  Is there
> anything we can test this more?
>

This is an example command line for a qemu VM with a vtnet interface
connected to a port of a VALE switch called 'vale1':

qemu-system-x86_64 fbsd-head.qcow2 -smp 2 -m 2G -vga std -device
e1000,netdev=mgmt,mac=00:AA:BB:CC:0a:99 -netdev
user,id=mgmt,hostfwd=tcp::20010-:22 -device
virtio-net-pci,netdev=data10,mac=00:AA:BB:CC:0a:0a,ioeventfd=on,mrg_rxbuf=on
-netdev netmap,ifname=vale1:10,id=data10

Cheers,
  Vincenzo

>
> Best,
> Li-Wen
>

Re: [Qemu-devel] Is network backend netmap worth keeping?

[Vincenzo Maffione]

Yes, indeed.
Netmap is actively maintained on FreeBSD, and QEMU is packaged on FreeBSD
with netmap support enabled.
Also keep in mind that, differently from Linux, the (current) tap driver on
FreeBSD does not support offloads (e.g. IFF_VNET_HDR, TUNSETVNETHDRSIZE and
so on). On the contrary, netmap (VALE) supports the same offloads as tap
does on Linux. This makes inter-VM networking with VALE more appealing on
FreeBSD.

Cheers,
  Vincenzo

Il giorno gio 8 ago 2019 alle ore 15:36 Philippe Mathieu-Daudé <
phi...@redhat.com> ha scritto:

> On 8/8/19 7:38 AM, Jason Wang wrote:
> >
> > On 2019/8/8 下午12:48, Markus Armbruster wrote:
> >> Please excuse the attention-grabbing subject.
> >>
> >> Philippe Mathieu-Daudé  writes:
> >>
> >>> On 8/7/19 10:16 PM, Markus Armbruster wrote:
> >> [...]
>  Can you tell me offhand what I have to install so configure enables
>  CONFIG_NETMAP?
> >>> The steps are listed in tests/docker/dockerfiles/debian-amd64.docker,
> >>> but you can get to this point running:
> >>>
> >>>$ make docker-image-debian-amd64 V=1 DEBUG=1
> >>>
> >>> This will build the docker image with netmap (so you don't have to mess
> >>> with your workstation setup), then build QEMU within the image.
> >> So, to make use of QEMU's netmap backend (CONFIG_NETMAP), you have to
> >> build and install netmap software from sources.  Which pretty much
> >> ensures nobody uses it.  It was added in commit 58952137b0b (Nov 2013).
> >> The commit message points to ,
> >> which gives me "connection timed out" right now.
> >>
> >> On the other hand, it's covered in MAINTAINERS, and has seen
> >> non-janitorial activity as late as Dec 2018 (commit c693fc748a).
> >>
> >> Luigi, Giuseppe, Vincenzo, what's the status of the netmap project?
> >>
> >> Why is the QEMU netmap backend worth keeping?
> >>
> >> Who is using the netmap backend?
> >
> >
> > Netmap was supported by freebsd:
> > https://www.freebsd.org/cgi/man.cgi?query=netmap&sektion=4. So I guess
> > there should be real users.
> >
> >
> >>
> >> How do they obtain a netmap-enabled QEMU?  Compile it from sources
> >> themselves?
> >
> >
> > Yes.
>
> Hmm at least on the FreeBSD setup by vmtest (12.0-RELEASE r341666) we
> don't need to build it from source:
>
> $ make vm-build-freebsd V=1 DEBUG=1
> [...]
> netmap supportyes
> [...]
>
> $ fgrep -r CONFIG_NETMAP .
> ./config-host.mak:CONFIG_NETMAP=y
>

Re: [Qemu-devel] [PATCH] shippable: fix build

[Vincenzo Maffione]

Hi,
  Yes, we added a static library that to ease application development
(libnetmap.a).
It looks like a configure/Makefile issue. Can you reproduce that?

(I'm CC-ing Giuseppe, who actually added libnetmap).

Cheers,
  Vincenzo

Il giorno gio 21 feb 2019 alle ore 10:14 Stefano Garzarella <
sgarz...@redhat.com> ha scritto:

> CCing Vincenzo, one of the maintainer of Netmap.
>
> On Wed, Feb 20, 2019 at 07:00:19PM +0100, Philippe Mathieu-Daudé wrote:
> > On 2/20/19 6:45 PM, Alex Bennée wrote:
> > >
> > > Philippe Mathieu-Daudé  writes:
> > >
> > >> On 2/20/19 5:37 PM, Alex Bennée wrote:
> > >>> Paolo Bonzini  writes:
> > >>>
> >  It looks like something in netmap has changed and compilation fails:
> > 
> > install -D libnetmap.a //usr/local/lib/libnetmap.a
> > install: cannot stat libnetmap.a: No such file or directory
> > 
> >  Add an explicit "make" step to fix it.
> > >>>
> > >>> I took an alternate approach as tracking another projects master
> seems
> > >>> like a bad idea:
> > >>>
> > >>> tests/docker: peg netmap code to a specific version
> > >>>
> > >>> Tracking head is always going to be at the whims of the upstream.
> > >>> Let's use a defined release so things don't magically change under
> us.
> > >>
> > >> Oh now I see your reply...
> > >>
> > >>> Signed-off-by: Alex Bennée 
> > >>>
> > >>> 1 file changed, 1 insertion(+)
> > >>> tests/docker/dockerfiles/debian-amd64.docker | 1 +
> > >>>
> > >>> modified   tests/docker/dockerfiles/debian-amd64.docker
> > >>> @@ -33,6 +33,7 @@ RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> > >>>  apt-get install -y --no-install-recommends \
> > >>>  linux-headers-amd64
> > >>>  RUN git clone https://github.com/luigirizzo/netmap.git
> /usr/src/netmap
> > >>> +RUN cd /usr/src/netmap && git checkout v11.3
> > >>
> > >> ... and I notice I forgot to change directory in my previous
> suggestion.
> > >>
> > >> Why do you take v11.3 and not v11.4?
> > >> I agree v11.3 was closer to what I tested when I introduced this
> > >> dependency, but I'd rather use the latest release.
> > >
> > > Hmm github hid the newer releases by default and has a Latest Release
> > > button by v11.3.
> >
> > I don't understand (this GitHub feature)...
> >
> > I'm talking about this release:
> > https://github.com/luigirizzo/netmap/releases/tag/v11.4
> >
> > The date is more recent that the v11.3. This looks like the latest
> > release to me.
> >
> > Anyway I'm fine if you prefer v11.3.
> > Please send a formal patch because I don't want to confuse scripts
> > adding my R-b in Paolo's patch.
> >
> > Regards,
> >
> > Phil.
> >
>
> --
>
> Stefano Garzarella
> Software Engineer @ Red Hat
>


-- 
Vincenzo

Re: [Qemu-devel] [PATCH v1 0/1] Allow TAP to unnegotiate virtio-net header

[Vincenzo Maffione]

Il giorno mar 19 feb 2019 alle ore 07:48 Jason Wang 
ha scritto:

>
> On 2019/2/18 下午7:53, Vincenzo Maffione wrote:
> > Hi Jason,
> >Thanks for the quick reply.
> >
> > My PV device (to be open sourced soon) uses the QEMU net backend
> interface,
> > in a way similar to virtio-net.
> > For example it uses qemu_set_offload(), qemu_has_vnet_hdr_len(),
> > qemu_using_vnet_hdr(), qemu_send_packet(), etc.
> > This means that the device itself does not know which net backend is
> using,
> > of course.
> > In addition to TAP, also the netmap backend supports the virtio-net
> header,
> > and so the PV device will work with both.
> > Regarding the virtio-net header, the netmap backend is more flexible
> > because you can change the virtio-net header length as you wish (choosing
> > between 0, 10 and 12 bytes).
>
>
> Interesting, actually, there's case that vnet header is not used even
> for virtio-net, e.g for the case of XDP withouth csum support. If we
> don't pass vnet header in this case, we may get even higher PPS.
>

I guess in this case we simply pass a zeroed header. According to my
experience I think it's unlikely that PPS are affected by having 12
additional bytes to copy around, as long as no processing is done on the
header. I may be wrong of course.

>
>
> >
> > My problem cannot be solved by making sure that TAPs are opened without
> > IFF_VNET_HDR. What I want instead
> > is to dynamically change the length of the virtio-net header that the
> > backend accepts, switching between 12 bytes (virtio 1.0 header) and 0 (no
> > header). By "dynamically" I mean while the guest (and the device) is
> > running. This is possible and safe if we get rid of that assert().
> > The virtio-net device does not need that flexibility (once you enable the
> > vtnet header, you never disable that again), so I guess that's why you
> have
> > that assert().
> > My PV device can be reconfigured on the fly in such a way it may or not
> may
> > be aware of the virtio-net header, and as a consequence it may need to
> call
> > qemu_using_vnet_hdr() with true or false (multiple times).
> >
> > Does it make sense?
>
>
> Yes, it is. But I think maybe it's better to send this patch with your
> new PV device implementation.
>
> Sounds good, thanks.

Cheers,
  Vincenzo


> Thanks
>
>
> >
> > Cheers,
> >Vincenzo
> >
> >
> > Il giorno lun 18 feb 2019 alle ore 05:19 Jason Wang  >
> > ha scritto:
> >
> >> On 2019/2/16 上午1:46, Vincenzo Maffione wrote:
> >>> Hi,
> >>> I was doing experiments with a custom paravirtualized net device,
> >>> and I ran into a limitation of the TAP backend. I see from the kernel
> >>> code that it is not possible to set the TAP virtio-net header
> >>> length to something different from 10 or 12, which means that it
> >>> is not possible to set it to 0. That's fine.
> >>> The QEMU implementation of the TAP backend already supports
> >>> the case where virtio-net header is set to 10 or 12 in kernel,
> >>> but the emulated/paravirtualized net device is not using that:
> >>> the TAP backend will just strip/prepend a zeroed header in this case
> >>> (e.g., see the if statement in tap_receive_iov()).
> >>> However, the tap_using_vnet_hdr() has an assert() that prevents
> >>> this situation to happen, and I don't understand why. Maybe it
> >>> is a leftover? I tried to remove the assert and by doing that
> >>> my paravirtualized device was able to stop using the virtio-net
> >>> header.
> >>
> >> Hi:
> >>
> >> If  I understand this correctly, your PV device is API compatible with
> >> TAP? Then you may just adding code to call TUNSETIFF without
> IFF_VNET_HDR?
> >>
> >> Thanks
> >>
> >>
> >>> Vincenzo Maffione (1):
> >>> net: tap: allow net frontends to un-negotiate virtio-net header
> >>>
> >>>net/tap.c | 1 -
> >>>1 file changed, 1 deletion(-)
> >>>
> >
>


-- 
Vincenzo

Re: [Qemu-devel] [PATCH v1 0/1] Allow TAP to unnegotiate virtio-net header

[Vincenzo Maffione]

Hi Jason,
  Thanks for the quick reply.

My PV device (to be open sourced soon) uses the QEMU net backend interface,
in a way similar to virtio-net.
For example it uses qemu_set_offload(), qemu_has_vnet_hdr_len(),
qemu_using_vnet_hdr(), qemu_send_packet(), etc.
This means that the device itself does not know which net backend is using,
of course.
In addition to TAP, also the netmap backend supports the virtio-net header,
and so the PV device will work with both.
Regarding the virtio-net header, the netmap backend is more flexible
because you can change the virtio-net header length as you wish (choosing
between 0, 10 and 12 bytes).

My problem cannot be solved by making sure that TAPs are opened without
IFF_VNET_HDR. What I want instead
is to dynamically change the length of the virtio-net header that the
backend accepts, switching between 12 bytes (virtio 1.0 header) and 0 (no
header). By "dynamically" I mean while the guest (and the device) is
running. This is possible and safe if we get rid of that assert().
The virtio-net device does not need that flexibility (once you enable the
vtnet header, you never disable that again), so I guess that's why you have
that assert().
My PV device can be reconfigured on the fly in such a way it may or not may
be aware of the virtio-net header, and as a consequence it may need to call
qemu_using_vnet_hdr() with true or false (multiple times).

Does it make sense?

Cheers,
  Vincenzo


Il giorno lun 18 feb 2019 alle ore 05:19 Jason Wang 
ha scritto:

>
> On 2019/2/16 上午1:46, Vincenzo Maffione wrote:
> > Hi,
> >I was doing experiments with a custom paravirtualized net device,
> > and I ran into a limitation of the TAP backend. I see from the kernel
> > code that it is not possible to set the TAP virtio-net header
> > length to something different from 10 or 12, which means that it
> > is not possible to set it to 0. That's fine.
> > The QEMU implementation of the TAP backend already supports
> > the case where virtio-net header is set to 10 or 12 in kernel,
> > but the emulated/paravirtualized net device is not using that:
> > the TAP backend will just strip/prepend a zeroed header in this case
> > (e.g., see the if statement in tap_receive_iov()).
> > However, the tap_using_vnet_hdr() has an assert() that prevents
> > this situation to happen, and I don't understand why. Maybe it
> > is a leftover? I tried to remove the assert and by doing that
> > my paravirtualized device was able to stop using the virtio-net
> > header.
>
>
> Hi:
>
> If  I understand this correctly, your PV device is API compatible with
> TAP? Then you may just adding code to call TUNSETIFF without IFF_VNET_HDR?
>
> Thanks
>
>
> >
> > Vincenzo Maffione (1):
> >net: tap: allow net frontends to un-negotiate virtio-net header
> >
> >   net/tap.c | 1 -
> >   1 file changed, 1 deletion(-)
> >
>


-- 
Vincenzo

[Qemu-devel] [PATCH v1 1/1] net: tap: allow net frontends to un-negotiate virtio-net header

[Vincenzo Maffione]

The implementation of the TAP net backend already supports the case
(s->host_vnet_hdr_len && !s->using_vnet_hdr), which means that
the TAP device is expecting the header, while the net frontend
(emulated device) is not aware of it. This case is handled by
stripping or prepending the (zeroed) header on the fly.
However, the function tap_using_vnet_hdr() has an assert() that
explicitly prevents this situation to happen.
This patch removes the assert(), so that net frontends are
free to un-negotiate the header.

Signed-off-by: Vincenzo Maffione 
---
 net/tap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/tap.c b/net/tap.c
index cc8525f154..6f2aca0396 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -264,7 +264,6 @@ static void tap_using_vnet_hdr(NetClientState *nc, bool 
using_vnet_hdr)
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
 assert(nc->info->type == NET_CLIENT_DRIVER_TAP);
-assert(!!s->host_vnet_hdr_len == using_vnet_hdr);
 
 s->using_vnet_hdr = using_vnet_hdr;
 }
-- 
2.20.1

[Qemu-devel] [PATCH v1 0/1] Allow TAP to unnegotiate virtio-net header

[Vincenzo Maffione]

Hi,
  I was doing experiments with a custom paravirtualized net device,
and I ran into a limitation of the TAP backend. I see from the kernel
code that it is not possible to set the TAP virtio-net header
length to something different from 10 or 12, which means that it
is not possible to set it to 0. That's fine.
The QEMU implementation of the TAP backend already supports
the case where virtio-net header is set to 10 or 12 in kernel,
but the emulated/paravirtualized net device is not using that:
the TAP backend will just strip/prepend a zeroed header in this case
(e.g., see the if statement in tap_receive_iov()).
However, the tap_using_vnet_hdr() has an assert() that prevents
this situation to happen, and I don't understand why. Maybe it
is a leftover? I tried to remove the assert and by doing that
my paravirtualized device was able to stop using the virtio-net
header.

Vincenzo Maffione (1):
  net: tap: allow net frontends to un-negotiate virtio-net header

 net/tap.c | 1 -
 1 file changed, 1 deletion(-)

-- 
2.20.1

[Qemu-devel] [PATCH v1 3/3] net: netmap: improve netmap_receive_iov()

[Vincenzo Maffione]

Changes:
  - Save CPU cycles by computing the return value while scanning the
input iovec, rather than calling iov_size() at the end.
  - Remove check for s->tx != NULL, because it cannot happen.
  - Cache ring->tail in a local variable and use it to check for
space in the TX ring. The use of nm_ring_empty() was invalid,
because nobody is updating ring->cur and ring->head at that point.
  - In case we run out of netmap slots in the middle of a packet,
move the wake-up point by advancing ring->cur, but do not
expose the incomplete packet (i.e., by updating also ring->head).

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 29 +
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 852106af29..0cc8f545c5 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -159,21 +159,22 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 {
 NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
 struct netmap_ring *ring = s->tx;
+unsigned int tail = ring->tail;
+ssize_t totlen = 0;
 uint32_t last;
 uint32_t idx;
 uint8_t *dst;
 int j;
 uint32_t i;
 
-if (unlikely(!ring)) {
-/* Drop the packet. */
-return iov_size(iov, iovcnt);
-}
-
-last = i = ring->cur;
+last = i = ring->head;
 
 if (nm_ring_space(ring) < iovcnt) {
-/* Not enough netmap slots. */
+/* Not enough netmap slots. Tell the kernel that we have seen the new
+ * available slots (so that it notifies us again when it has more
+ * ones), but without publishing any new slots to be processed
+ * (e.g., we don't advance ring->head). */
+ring->cur = tail;
 netmap_write_poll(s, true);
 return 0;
 }
@@ -183,14 +184,17 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 int offset = 0;
 int nm_frag_size;
 
+totlen += iov_frag_size;
+
 /* Split each iovec fragment over more netmap slots, if
necessary. */
 while (iov_frag_size) {
 nm_frag_size = MIN(iov_frag_size, ring->nr_buf_size);
 
-if (unlikely(nm_ring_empty(ring))) {
-/* We run out of netmap slots while splitting the
+if (unlikely(i == tail)) {
+/* We ran out of netmap slots while splitting the
iovec fragments. */
+ring->cur = tail;
 netmap_write_poll(s, true);
 return 0;
 }
@@ -212,12 +216,13 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 /* The last slot must not have NS_MOREFRAG set. */
 ring->slot[last].flags &= ~NS_MOREFRAG;
 
-/* Now update ring->cur and ring->head. */
-ring->cur = ring->head = i;
+/* Now update ring->head and ring->cur to publish the new slots and
+ * the new wakeup point. */
+ring->head = ring->cur = i;
 
 ioctl(s->nmd->fd, NIOCTXSYNC, NULL);
 
-return iov_size(iov, iovcnt);
+return totlen;
 }
 
 static ssize_t netmap_receive(NetClientState *nc,
-- 
2.19.2

[Qemu-devel] [PATCH v1 0/3] Improvements to the netmap backend

[Vincenzo Maffione]

Improvements to the netmap backend, mostly to fix the handling of
incomplete multi-slot packets.
This has been tested with and without jumbo frames, using pkt-gen
to send 60, 1500, 5000 or 9000 bytes packets between two VMs connected
through a VALE switch.

Vincenzo Maffione (3):
  net: netmap: small improvements netmap_send()
  net: netmap: simplify netmap_receive()
  net: netmap: improve netmap_receive_iov()

 net/netmap.c | 110 ++-
 1 file changed, 47 insertions(+), 63 deletions(-)

-- 
2.19.2

[Qemu-devel] [PATCH v1 1/3] net: netmap: small improvements netmap_send()

[Vincenzo Maffione]

This change improves the handling of incomplete multi-slot packets
(e.g. with the NS_MOREFRAG set), by advancing ring->head only on
complete packets. The ring->cur pointer is advanced in any case in
order to acknowledge the kernel and move the wake-up point (thus
avoiding repeated wake-ups).
Also don't be verbose when incomplete packets are found.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 31 +++
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 2d11a8f4be..71a8122bdd 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -272,39 +272,46 @@ static void netmap_send(void *opaque)
 {
 NetmapState *s = opaque;
 struct netmap_ring *ring = s->rx;
+unsigned int tail = ring->tail;
 
-/* Keep sending while there are available packets into the netmap
+/* Keep sending while there are available slots in the netmap
RX ring and the forwarding path towards the peer is open. */
-while (!nm_ring_empty(ring)) {
-uint32_t i;
+while (ring->head != tail) {
+uint32_t i = ring->head;
 uint32_t idx;
 bool morefrag;
 int iovcnt = 0;
 int iovsize;
 
+/* Get a (possibly multi-slot) packet. */
 do {
-i = ring->cur;
 idx = ring->slot[i].buf_idx;
 morefrag = (ring->slot[i].flags & NS_MOREFRAG);
-s->iov[iovcnt].iov_base = (u_char *)NETMAP_BUF(ring, idx);
+s->iov[iovcnt].iov_base = (void *)NETMAP_BUF(ring, idx);
 s->iov[iovcnt].iov_len = ring->slot[i].len;
 iovcnt++;
+i = nm_ring_next(ring, i);
+} while (i != tail && morefrag);
 
-ring->cur = ring->head = nm_ring_next(ring, i);
-} while (!nm_ring_empty(ring) && morefrag);
+/* Advance ring->cur to tell the kernel that we have seen the slots. */
+ring->cur = i;
 
-if (unlikely(nm_ring_empty(ring) && morefrag)) {
-RD(5, "[netmap_send] ran out of slots, with a pending"
-   "incomplete packet\n");
+if (unlikely(morefrag)) {
+/* This is a truncated packet, so we can stop without releasing the
+ * incomplete slots by updating ring->head. We will hopefully
+ * re-read the complete packet the next time we are called. */
+break;
 }
 
 iovsize = qemu_sendv_packet_async(&s->nc, s->iov, iovcnt,
 netmap_send_completed);
 
+/* Release the slots to the kernel. */
+ring->head = i;
+
 if (iovsize == 0) {
 /* The peer does not receive anymore. Packet is queued, stop
- * reading from the backend until netmap_send_completed()
- */
+ * reading from the backend until netmap_send_completed(). */
 netmap_read_poll(s, false);
 break;
 }
-- 
2.19.2

[Qemu-devel] [PATCH v1 2/3] net: netmap: simplify netmap_receive()

[Vincenzo Maffione]

Improve code reuse by implementing netmap_receive() with a call
to netmap_receive_iov().

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 50 +++---
 1 file changed, 11 insertions(+), 39 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 71a8122bdd..852106af29 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -154,45 +154,6 @@ static void netmap_writable(void *opaque)
 qemu_flush_queued_packets(&s->nc);
 }
 
-static ssize_t netmap_receive(NetClientState *nc,
-  const uint8_t *buf, size_t size)
-{
-NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
-struct netmap_ring *ring = s->tx;
-uint32_t i;
-uint32_t idx;
-uint8_t *dst;
-
-if (unlikely(!ring)) {
-/* Drop. */
-return size;
-}
-
-if (unlikely(size > ring->nr_buf_size)) {
-RD(5, "[netmap_receive] drop packet of size %d > %d\n",
-(int)size, ring->nr_buf_size);
-return size;
-}
-
-if (nm_ring_empty(ring)) {
-/* No available slots in the netmap TX ring. */
-netmap_write_poll(s, true);
-return 0;
-}
-
-i = ring->cur;
-idx = ring->slot[i].buf_idx;
-dst = (uint8_t *)NETMAP_BUF(ring, idx);
-
-ring->slot[i].len = size;
-ring->slot[i].flags = 0;
-pkt_copy(buf, dst, size);
-ring->cur = ring->head = nm_ring_next(ring, i);
-ioctl(s->nmd->fd, NIOCTXSYNC, NULL);
-
-return size;
-}
-
 static ssize_t netmap_receive_iov(NetClientState *nc,
 const struct iovec *iov, int iovcnt)
 {
@@ -259,6 +220,17 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 return iov_size(iov, iovcnt);
 }
 
+static ssize_t netmap_receive(NetClientState *nc,
+  const uint8_t *buf, size_t size)
+{
+struct iovec iov;
+
+iov.iov_base = (void *)buf;
+iov.iov_len = size;
+
+return netmap_receive_iov(nc, &iov, 1);
+}
+
 /* Complete a previous send (backend --> guest) and enable the
fd_read callback. */
 static void netmap_send_completed(NetClientState *nc, ssize_t len)
-- 
2.19.2

[Qemu-devel] [PATCH] qemu-options.hx: add missing id=chr0 chardev argument in vhost-user example

[Vincenzo Maffione]

In the vhost-user example, a chardev with id chr0 is referenced by the
vhost-user net backend, but the id is not specified in the chardev option.

Signed-off-by: Vincenzo Maffione 
---
 qemu-options.hx | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 5633d3914f..ed22fc479a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2127,7 +2127,7 @@ Example:
 @example
 qemu -m 512 -object 
memory-backend-file,id=mem,size=512M,mem-path=/hugetlbfs,share=on \
  -numa node,memdev=mem \
- -chardev socket,path=/path/to/socket \
+ -chardev socket,id=chr0,path=/path/to/socket \
  -netdev type=vhost-user,id=net0,chardev=chr0 \
  -device virtio-net-pci,netdev=net0
 @end example
-- 
2.11.1

[Qemu-devel] [PATCH v3] net: netmap: probe netmap interface for virtio-net header

[Vincenzo Maffione]

Previous implementation of has_ufo, has_vnet_hdr, has_vnet_hdr_len, etc.
did not really probe for virtio-net header support for the netmap
interface attached to the backend. These callbacks were correct for
VALE ports, but incorrect for hardware NICs, pipes, monitors, etc.

This patch fixes the implementation to work properly with all kinds
of netmap ports.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 59 ++-
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 9710321..1b42728 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -323,20 +323,47 @@ static void netmap_cleanup(NetClientState *nc)
 }
 
 /* Offloading manipulation support callbacks. */
-static bool netmap_has_ufo(NetClientState *nc)
+static int netmap_fd_set_vnet_hdr_len(NetmapState *s, int len)
 {
-return true;
+struct nmreq req;
+
+/* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
+ * length for the netmap adapter associated to 's->ifname'.
+ */
+memset(&req, 0, sizeof(req));
+pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
+req.nr_version = NETMAP_API;
+req.nr_cmd = NETMAP_BDG_VNET_HDR;
+req.nr_arg1 = len;
+
+return ioctl(s->nmd->fd, NIOCREGIF, &req);
 }
 
-static bool netmap_has_vnet_hdr(NetClientState *nc)
+static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
+NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+int prev_len = s->vnet_hdr_len;
+
+/* Check that we can set the new length. */
+if (netmap_fd_set_vnet_hdr_len(s, len)) {
+return false;
+}
+
+/* Restore the previous length. */
+if (netmap_fd_set_vnet_hdr_len(s, prev_len)) {
+error_report("Failed to restore vnet-hdr length %d on %s: %s",
+ prev_len, s->ifname, strerror(errno));
+abort();
+}
+
 return true;
 }
 
-static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
+/* A netmap interface that supports virtio-net headers always
+ * supports UFO, so we use this callback also for the has_ufo hook. */
+static bool netmap_has_vnet_hdr(NetClientState *nc)
 {
-return len == 0 || len == sizeof(struct virtio_net_hdr) ||
-len == sizeof(struct virtio_net_hdr_mrg_rxbuf);
+return netmap_has_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
 }
 
 static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
@@ -347,20 +374,11 @@ static void netmap_set_vnet_hdr_len(NetClientState *nc, 
int len)
 {
 NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
 int err;
-struct nmreq req;
 
-/* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
- * length for the netmap adapter associated to 's->ifname'.
- */
-memset(&req, 0, sizeof(req));
-pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
-req.nr_version = NETMAP_API;
-req.nr_cmd = NETMAP_BDG_VNET_HDR;
-req.nr_arg1 = len;
-err = ioctl(s->nmd->fd, NIOCREGIF, &req);
+err = netmap_fd_set_vnet_hdr_len(s, len);
 if (err) {
-error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
- s->ifname, strerror(errno));
+error_report("Unable to set vnet-hdr length %d on %s: %s",
+ len, s->ifname, strerror(errno));
 } else {
 /* Keep track of the current length. */
 s->vnet_hdr_len = len;
@@ -373,8 +391,7 @@ static void netmap_set_offload(NetClientState *nc, int 
csum, int tso4, int tso6,
 NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
 
 /* Setting a virtio-net header length greater than zero automatically
- * enables the offloadings.
- */
+ * enables the offloadings. */
 if (!s->vnet_hdr_len) {
 netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
 }
@@ -388,7 +405,7 @@ static NetClientInfo net_netmap_info = {
 .receive_iov = netmap_receive_iov,
 .poll = netmap_poll,
 .cleanup = netmap_cleanup,
-.has_ufo = netmap_has_ufo,
+.has_ufo = netmap_has_vnet_hdr,
 .has_vnet_hdr = netmap_has_vnet_hdr,
 .has_vnet_hdr_len = netmap_has_vnet_hdr_len,
 .using_vnet_hdr = netmap_using_vnet_hdr,
-- 
2.7.1

[Qemu-devel] [PATCH v3] net: netmap: implement virtio-net header probe

[Vincenzo Maffione]

Current implementation for netmap backend does not really probe the netmap
kernel module to know whether virtio-net header is supported for a specific
netmap port. This is only correct for VALE ports, but it is not correct for
hardware ports, pipes, etc.

This patch properly implements the missing probe mechanism, fixing the
implementation of the involved hooks (has_vnet_hdr, has_vnet_hdr_len, has_ufo,
...).

CHANGELOG:
  - renamed netmap_do_set_vnet_hdr_len to netmap_fd_set_vnet_hdr_len
  - abort() when failing to restore previous vnet-hdr length
  - moved update to s->vnet_hdr_len to netmap_set_vnet_hdr_len

Vincenzo Maffione (1):
  net: netmap: probe netmap interface for virtio-net header

 net/netmap.c | 59 ++-
 1 file changed, 38 insertions(+), 21 deletions(-)

-- 
2.7.1

Re: [Qemu-devel] [PATCH v2] net: netmap: probe netmap interface for virtio-net header

[Vincenzo Maffione]

2016-02-24 9:21 GMT+01:00 Jason Wang :
>
>
> On 02/23/2016 04:46 PM, Vincenzo Maffione wrote:
>> Previous implementation of has_ufo, has_vnet_hdr, has_vnet_hdr_len, etc.
>> did not really probe for virtio-net header support for the netmap
>> interface attached to the backend. These callbacks were correct for
>> VALE ports, but incorrect for hardware NICs, pipes, monitors, etc.
>>
>> This patch fixes the implementation to work properly with all kinds
>> of netmap ports.
>>
>> Signed-off-by: Vincenzo Maffione 
>
> Looks good overall, just few nits, see below.
>
> Thanks
>
>> ---
>>  net/netmap.c | 62 
>> +++-
>>  1 file changed, 40 insertions(+), 22 deletions(-)
>>
>> diff --git a/net/netmap.c b/net/netmap.c
>> index 9710321..ef64be0 100644
>> --- a/net/netmap.c
>> +++ b/net/netmap.c
>> @@ -323,20 +323,51 @@ static void netmap_cleanup(NetClientState *nc)
>>  }
>>
>>  /* Offloading manipulation support callbacks. */
>> -static bool netmap_has_ufo(NetClientState *nc)
>> +static int netmap_do_set_vnet_hdr_len(NetmapState *s, int len)
>>  {
>
> Like tap, how about rename this function to netmap_fd_set_vnet_hdr_len?
Sure.

>
>> -return true;
>> +struct nmreq req;
>> +int err;
>> +
>> +/* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
>> + * length for the netmap adapter associated to 's->ifname'.
>> + */
>> +memset(&req, 0, sizeof(req));
>> +pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
>> +req.nr_version = NETMAP_API;
>> +req.nr_cmd = NETMAP_BDG_VNET_HDR;
>> +req.nr_arg1 = len;
>> +err = ioctl(s->nmd->fd, NIOCREGIF, &req);
>> +if (err) {
>> +return err;
>> +}
>> +
>> +/* Keep track of the current length. */
>> +s->vnet_hdr_len = len;
>
> How about move this to netmap_set_vnet_hdr_len() to let this function
> only cares about ioctl?

Agree, but it is correct only if we abort() below (and we will).

>
>> +
>> +return 0;
>>  }
>>
>> -static bool netmap_has_vnet_hdr(NetClientState *nc)
>> +static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
>>  {
>> +NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
>> +int prev_len = s->vnet_hdr_len;
>> +
>> +/* Check that we can set the new length. */
>> +if (netmap_do_set_vnet_hdr_len(s, len)) {
>> +return false;
>> +}
>> +
>> +/* Restore the previous length. */
>> +netmap_do_set_vnet_hdr_len(s, prev_len);
>
> Do we need to check and abort() if it fails like tap?

Yes, it's better, also because this should never happen.

Thanks,
  Vincenzo

>
>> +
>>  return true;
>>  }
>>
>> -static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
>> +/* A netmap interface that supports virtio-net headers always
>> + * supports UFO, so we use this callback also for the has_ufo hook. */
>> +static bool netmap_has_vnet_hdr(NetClientState *nc)
>>  {
>> -return len == 0 || len == sizeof(struct virtio_net_hdr) ||
>> -len == sizeof(struct virtio_net_hdr_mrg_rxbuf);
>> +return netmap_has_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
>>  }
>>
>>  static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
>> @@ -347,23 +378,11 @@ static void netmap_set_vnet_hdr_len(NetClientState 
>> *nc, int len)
>>  {
>>  NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
>>  int err;
>> -struct nmreq req;
>>
>> -/* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
>> - * length for the netmap adapter associated to 's->ifname'.
>> - */
>> -memset(&req, 0, sizeof(req));
>> -pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
>> -req.nr_version = NETMAP_API;
>> -req.nr_cmd = NETMAP_BDG_VNET_HDR;
>> -req.nr_arg1 = len;
>> -err = ioctl(s->nmd->fd, NIOCREGIF, &req);
>> +err = netmap_do_set_vnet_hdr_len(s, len);
>>  if (err) {
>>  error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
>>   s->ifname, strerror(errno));
>> -} else {
>> -/* Keep track of the current length. */
>> -s->vnet_hdr_len = len;
>>  }
>>  }
>>
>> @@ -373,8 +392,7 @@ static void netmap_set_offload(NetClientState *nc, int 

[Qemu-devel] [PATCH v2] net: netmap: implement virtio-net header probe

[Vincenzo Maffione]

Current implementation for netmap backend does not really probe the netmap
kernel module to know whether virtio-net header is supported for a specific
netmap port. This is only correct for VALE ports, but it is not correct for
hardware ports, pipes, etc.

This patch properly implements the missing probe mechanism, fixing the
implementation of the involved hooks (has_vnet_hdr, has_vnet_hdr_len, has_ufo,
...).

CHANGELOG:
   - Removed err_report argument from netmap_do_set_vnet_hdr_len(), so that
 the caller decides when to report the error.
   - Error reporting on failure happens in netmap_set_vnet_hdr_len().

Vincenzo Maffione (1):
  net: netmap: probe netmap interface for virtio-net header

 net/netmap.c | 62 +++-
 1 file changed, 40 insertions(+), 22 deletions(-)

-- 
2.7.1

[Qemu-devel] [PATCH v2] net: netmap: probe netmap interface for virtio-net header

[Vincenzo Maffione]

Previous implementation of has_ufo, has_vnet_hdr, has_vnet_hdr_len, etc.
did not really probe for virtio-net header support for the netmap
interface attached to the backend. These callbacks were correct for
VALE ports, but incorrect for hardware NICs, pipes, monitors, etc.

This patch fixes the implementation to work properly with all kinds
of netmap ports.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 62 +++-
 1 file changed, 40 insertions(+), 22 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 9710321..ef64be0 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -323,20 +323,51 @@ static void netmap_cleanup(NetClientState *nc)
 }
 
 /* Offloading manipulation support callbacks. */
-static bool netmap_has_ufo(NetClientState *nc)
+static int netmap_do_set_vnet_hdr_len(NetmapState *s, int len)
 {
-return true;
+struct nmreq req;
+int err;
+
+/* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
+ * length for the netmap adapter associated to 's->ifname'.
+ */
+memset(&req, 0, sizeof(req));
+pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
+req.nr_version = NETMAP_API;
+req.nr_cmd = NETMAP_BDG_VNET_HDR;
+req.nr_arg1 = len;
+err = ioctl(s->nmd->fd, NIOCREGIF, &req);
+if (err) {
+return err;
+}
+
+/* Keep track of the current length. */
+s->vnet_hdr_len = len;
+
+return 0;
 }
 
-static bool netmap_has_vnet_hdr(NetClientState *nc)
+static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
+NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+int prev_len = s->vnet_hdr_len;
+
+/* Check that we can set the new length. */
+if (netmap_do_set_vnet_hdr_len(s, len)) {
+return false;
+}
+
+/* Restore the previous length. */
+netmap_do_set_vnet_hdr_len(s, prev_len);
+
 return true;
 }
 
-static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
+/* A netmap interface that supports virtio-net headers always
+ * supports UFO, so we use this callback also for the has_ufo hook. */
+static bool netmap_has_vnet_hdr(NetClientState *nc)
 {
-return len == 0 || len == sizeof(struct virtio_net_hdr) ||
-len == sizeof(struct virtio_net_hdr_mrg_rxbuf);
+return netmap_has_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
 }
 
 static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
@@ -347,23 +378,11 @@ static void netmap_set_vnet_hdr_len(NetClientState *nc, 
int len)
 {
 NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
 int err;
-struct nmreq req;
 
-/* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
- * length for the netmap adapter associated to 's->ifname'.
- */
-memset(&req, 0, sizeof(req));
-pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
-req.nr_version = NETMAP_API;
-req.nr_cmd = NETMAP_BDG_VNET_HDR;
-req.nr_arg1 = len;
-err = ioctl(s->nmd->fd, NIOCREGIF, &req);
+err = netmap_do_set_vnet_hdr_len(s, len);
 if (err) {
 error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
  s->ifname, strerror(errno));
-} else {
-/* Keep track of the current length. */
-s->vnet_hdr_len = len;
 }
 }
 
@@ -373,8 +392,7 @@ static void netmap_set_offload(NetClientState *nc, int 
csum, int tso4, int tso6,
 NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
 
 /* Setting a virtio-net header length greater than zero automatically
- * enables the offloadings.
- */
+ * enables the offloadings. */
 if (!s->vnet_hdr_len) {
 netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
 }
@@ -388,7 +406,7 @@ static NetClientInfo net_netmap_info = {
 .receive_iov = netmap_receive_iov,
 .poll = netmap_poll,
 .cleanup = netmap_cleanup,
-.has_ufo = netmap_has_ufo,
+.has_ufo = netmap_has_vnet_hdr,
 .has_vnet_hdr = netmap_has_vnet_hdr,
 .has_vnet_hdr_len = netmap_has_vnet_hdr_len,
 .using_vnet_hdr = netmap_using_vnet_hdr,
-- 
2.7.1

Re: [Qemu-devel] [PATCH] net: netmap: probe netmap interface for virtio-net header

[Vincenzo Maffione]

2016-02-18 3:52 GMT+01:00 Jason Wang :
>
>
> On 02/05/2016 06:30 PM, Vincenzo Maffione wrote:
>> Previous implementation of has_ufo, has_vnet_hdr, has_vnet_hdr_len, etc.
>> did not really probe for virtio-net header support for the netmap
>> interface attached to the backend. These callbacks were correct for
>> VALE ports, but incorrect for hardware NICs, pipes, monitors, etc.
>>
>> This patch fixes the implementation to work properly with all kinds
>> of netmap ports.
>>
>> Signed-off-by: Vincenzo Maffione 
>> ---
>>  net/netmap.c | 70 
>> ++--
>>  1 file changed, 44 insertions(+), 26 deletions(-)
>>
>> diff --git a/net/netmap.c b/net/netmap.c
>> index 9710321..f2dcaeb 100644
>> --- a/net/netmap.c
>> +++ b/net/netmap.c
>> @@ -323,20 +323,55 @@ static void netmap_cleanup(NetClientState *nc)
>>  }
>>
>>  /* Offloading manipulation support callbacks. */
>> -static bool netmap_has_ufo(NetClientState *nc)
>> +static int netmap_do_set_vnet_hdr_len(NetmapState *s, int len, bool 
>> err_report)
>
> Passing something like err_report usually means it was the
> responsibility of caller to report. So let's remove the err_report and
> let caller to decide based on the return value.
>

Ok, I'll try to fix this with the next patch.

>>  {
>> -return true;
>> +struct nmreq req;
>> +int err;
>> +
>> +/* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
>> + * length for the netmap adapter associated to 's->ifname'.
>> + */
>> +memset(&req, 0, sizeof(req));
>> +pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
>> +req.nr_version = NETMAP_API;
>> +req.nr_cmd = NETMAP_BDG_VNET_HDR;
>> +req.nr_arg1 = len;
>> +err = ioctl(s->nmd->fd, NIOCREGIF, &req);
>> +if (err) {
>> +if (err_report) {
>> +error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
>> + s->ifname, strerror(errno));
>> +}
>> +return -1;
>> +}
>> +
>> +/* Keep track of the current length. */
>> +s->vnet_hdr_len = len;
>> +
>> +return 0;
>>  }
>>
>> -static bool netmap_has_vnet_hdr(NetClientState *nc)
>> +static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
>>  {
>> +NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
>> +int prev_len = s->vnet_hdr_len;
>> +
>> +/* Check that we can set the new length. */
>> +if (netmap_do_set_vnet_hdr_len(s, len, false)) {
>> +return false;
>> +}
>> +
>> +/* Restore the previous length. */
>> +netmap_do_set_vnet_hdr_len(s, prev_len, true);
>> +
>>  return true;
>>  }
>>
>> -static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
>> +/* A netmap interface that supports virtio-net headers always
>> + * supports UFO, so we use this callback also for the has_ufo hook. */
>> +static bool netmap_has_vnet_hdr(NetClientState *nc)
>>  {
>> -return len == 0 || len == sizeof(struct virtio_net_hdr) ||
>> -len == sizeof(struct virtio_net_hdr_mrg_rxbuf);
>> +return netmap_has_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
>>  }
>>
>>  static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
>> @@ -346,25 +381,8 @@ static void netmap_using_vnet_hdr(NetClientState *nc, 
>> bool enable)
>>  static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
>>  {
>>  NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
>> -int err;
>> -struct nmreq req;
>>
>> -/* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
>> - * length for the netmap adapter associated to 's->ifname'.
>> - */
>> -memset(&req, 0, sizeof(req));
>> -pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
>> -req.nr_version = NETMAP_API;
>> -req.nr_cmd = NETMAP_BDG_VNET_HDR;
>> -req.nr_arg1 = len;
>> -err = ioctl(s->nmd->fd, NIOCREGIF, &req);
>> -if (err) {
>> -error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
>> - s->ifname, strerror(errno));
>> -} else {
>> -/* Keep track of the current length. */
>> -s->vnet_hdr_len = len;
>> -}
>> +netmap_do_set_vnet_hdr_len(s, len, true);
>>  }
>&

[Qemu-devel] [PATCH] net: netmap: probe netmap interface for virtio-net header

[Vincenzo Maffione]

Previous implementation of has_ufo, has_vnet_hdr, has_vnet_hdr_len, etc.
did not really probe for virtio-net header support for the netmap
interface attached to the backend. These callbacks were correct for
VALE ports, but incorrect for hardware NICs, pipes, monitors, etc.

This patch fixes the implementation to work properly with all kinds
of netmap ports.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 70 ++--
 1 file changed, 44 insertions(+), 26 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 9710321..f2dcaeb 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -323,20 +323,55 @@ static void netmap_cleanup(NetClientState *nc)
 }
 
 /* Offloading manipulation support callbacks. */
-static bool netmap_has_ufo(NetClientState *nc)
+static int netmap_do_set_vnet_hdr_len(NetmapState *s, int len, bool err_report)
 {
-return true;
+struct nmreq req;
+int err;
+
+/* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
+ * length for the netmap adapter associated to 's->ifname'.
+ */
+memset(&req, 0, sizeof(req));
+pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
+req.nr_version = NETMAP_API;
+req.nr_cmd = NETMAP_BDG_VNET_HDR;
+req.nr_arg1 = len;
+err = ioctl(s->nmd->fd, NIOCREGIF, &req);
+if (err) {
+if (err_report) {
+error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
+ s->ifname, strerror(errno));
+}
+return -1;
+}
+
+/* Keep track of the current length. */
+s->vnet_hdr_len = len;
+
+return 0;
 }
 
-static bool netmap_has_vnet_hdr(NetClientState *nc)
+static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
+NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+int prev_len = s->vnet_hdr_len;
+
+/* Check that we can set the new length. */
+if (netmap_do_set_vnet_hdr_len(s, len, false)) {
+return false;
+}
+
+/* Restore the previous length. */
+netmap_do_set_vnet_hdr_len(s, prev_len, true);
+
 return true;
 }
 
-static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
+/* A netmap interface that supports virtio-net headers always
+ * supports UFO, so we use this callback also for the has_ufo hook. */
+static bool netmap_has_vnet_hdr(NetClientState *nc)
 {
-return len == 0 || len == sizeof(struct virtio_net_hdr) ||
-len == sizeof(struct virtio_net_hdr_mrg_rxbuf);
+return netmap_has_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
 }
 
 static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
@@ -346,25 +381,8 @@ static void netmap_using_vnet_hdr(NetClientState *nc, bool 
enable)
 static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
 {
 NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
-int err;
-struct nmreq req;
 
-/* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
- * length for the netmap adapter associated to 's->ifname'.
- */
-memset(&req, 0, sizeof(req));
-pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
-req.nr_version = NETMAP_API;
-req.nr_cmd = NETMAP_BDG_VNET_HDR;
-req.nr_arg1 = len;
-err = ioctl(s->nmd->fd, NIOCREGIF, &req);
-if (err) {
-error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
- s->ifname, strerror(errno));
-} else {
-/* Keep track of the current length. */
-s->vnet_hdr_len = len;
-}
+netmap_do_set_vnet_hdr_len(s, len, true);
 }
 
 static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int 
tso6,
@@ -376,7 +394,7 @@ static void netmap_set_offload(NetClientState *nc, int 
csum, int tso4, int tso6,
  * enables the offloadings.
  */
 if (!s->vnet_hdr_len) {
-netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
+netmap_do_set_vnet_hdr_len(s, sizeof(struct virtio_net_hdr), true);
 }
 }
 
@@ -388,7 +406,7 @@ static NetClientInfo net_netmap_info = {
 .receive_iov = netmap_receive_iov,
 .poll = netmap_poll,
 .cleanup = netmap_cleanup,
-.has_ufo = netmap_has_ufo,
+.has_ufo = netmap_has_vnet_hdr,
 .has_vnet_hdr = netmap_has_vnet_hdr,
 .has_vnet_hdr_len = netmap_has_vnet_hdr_len,
 .using_vnet_hdr = netmap_using_vnet_hdr,
-- 
2.7.0

[Qemu-devel] [PATCH] net: netmap: implement virtio-net header probe

[Vincenzo Maffione]

Current implementation for netmap backend does not really probe the netmap
kernel module to know whether virtio-net header is supported for a specific
netmap port. This is only correct for VALE ports, but it is not correct for
hardware ports, pipes, etc.

This patch properly implements the missing probe mechanism, fixing the
implementation of the involved hooks (has_vnet_hdr, has_vnet_hdr_len, has_ufo,
...).

Vincenzo Maffione (1):
  net: netmap: probe netmap interface for virtio-net header

 net/netmap.c | 70 ++--
 1 file changed, 44 insertions(+), 26 deletions(-)

-- 
2.7.0

[Qemu-devel] [PATCH v2] net: netmap: use nm_open() to open netmap ports

[Vincenzo Maffione]

This patch simplifies the netmap backend code by means of the nm_open()
helper function provided by netmap_user.h, which hides the details of
open(), iotcl() and mmap() carried out on the netmap device.

Moreover, the semantic of nm_open() makes it possible to open special
netmap ports (e.g. pipes, monitors) and use special modes (e.g. host rings
only, single queue mode, exclusive access).

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 97 
 1 file changed, 32 insertions(+), 65 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 5558368..27295ab 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -39,21 +39,12 @@
 #include "qemu/error-report.h"
 #include "qemu/iov.h"
 
-/* Private netmap device info. */
-typedef struct NetmapPriv {
-int fd;
-size_t  memsize;
-void*mem;
-struct netmap_if*nifp;
-struct netmap_ring  *rx;
-struct netmap_ring  *tx;
-charfdname[PATH_MAX];/* Normally "/dev/netmap". */
-charifname[IFNAMSIZ];
-} NetmapPriv;
-
 typedef struct NetmapState {
 NetClientState  nc;
-NetmapPriv  me;
+struct nm_desc  *nmd;
+charifname[IFNAMSIZ];
+struct netmap_ring  *tx;
+struct netmap_ring  *rx;
 boolread_poll;
 boolwrite_poll;
 struct ioveciov[IOV_MAX];
@@ -90,44 +81,23 @@ pkt_copy(const void *_src, void *_dst, int l)
  * Open a netmap device. We assume there is only one queue
  * (which is the case for the VALE bridge).
  */
-static void netmap_open(NetmapPriv *me, Error **errp)
+static struct nm_desc *netmap_open(const NetdevNetmapOptions *nm_opts,
+   Error **errp)
 {
-int fd;
-int err;
-size_t l;
+struct nm_desc *nmd;
 struct nmreq req;
 
-me->fd = fd = open(me->fdname, O_RDWR);
-if (fd < 0) {
-error_setg_file_open(errp, errno, me->fdname);
-return;
-}
 memset(&req, 0, sizeof(req));
-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_setg_errno(errp, errno, "Unable to register %s", me->ifname);
-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_setg_errno(errp, errno, "Unable to mmap netmap shared memory");
-me->mem = NULL;
-goto error;
+nmd = nm_open(nm_opts->ifname, &req, NETMAP_NO_TX_POLL,
+  NULL);
+if (nmd == NULL) {
+error_setg_errno(errp, errno, "Failed to nm_open() %s",
+ nm_opts->ifname);
+return NULL;
 }
 
-me->nifp = NETMAP_IF(me->mem, req.nr_offset);
-me->tx = NETMAP_TXRING(me->nifp, 0);
-me->rx = NETMAP_RXRING(me->nifp, 0);
-
-return;
-
-error:
-close(me->fd);
+return nmd;
 }
 
 static void netmap_send(void *opaque);
@@ -136,7 +106,7 @@ static void netmap_writable(void *opaque);
 /* Set the event-loop handlers for the netmap backend. */
 static void netmap_update_fd_handler(NetmapState *s)
 {
-qemu_set_fd_handler(s->me.fd,
+qemu_set_fd_handler(s->nmd->fd,
 s->read_poll ? netmap_send : NULL,
 s->write_poll ? netmap_writable : NULL,
 s);
@@ -188,7 +158,7 @@ static ssize_t netmap_receive(NetClientState *nc,
   const uint8_t *buf, size_t size)
 {
 NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
-struct netmap_ring *ring = s->me.tx;
+struct netmap_ring *ring = s->tx;
 uint32_t i;
 uint32_t idx;
 uint8_t *dst;
@@ -218,7 +188,7 @@ static ssize_t netmap_receive(NetClientState *nc,
 ring->slot[i].flags = 0;
 pkt_copy(buf, dst, size);
 ring->cur = ring->head = nm_ring_next(ring, i);
-ioctl(s->me.fd, NIOCTXSYNC, NULL);
+ioctl(s->nmd->fd, NIOCTXSYNC, NULL);
 
 return size;
 }
@@ -227,7 +197,7 @@ 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;
+struct netmap_ring *ring = s->tx;
 uint32_t last;
 uint32_t idx;
 uint8_t *dst;
@@ -284,7 +254,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 /* Now update ring->cur and ring->head. */
 ring->cur = ring->head = i;
 
-ioctl(s->me.fd, NIOCTXSYNC, NULL);
+ioctl(s->nmd->fd, NIOCTXSYNC, NULL);
 
 return iov_size(iov, iovcnt);
 }
@@ -301,7 +271,7 @@ static void netmap_s

[Qemu-devel] [PATCH v2] net: netmap: use nm_open() to open netmap port

[Vincenzo Maffione]

Current implementation of netmap backend still uses the low level netmap
API to open, register and close netmap ports. However, the netmap_user.h
header provides some helper functions that make these tasks easier, with
the additional advantage of extended interface name semantic.

As an example, the extended semantic allows to open a netmap pipe over
a VALE port by specifying an interface name in the form "valeXX:YY{ZZ".
Other suffixes are used for netmap monitors, exclusive access, and to
specify special binding mode (e.g. single queue, RX-only, TX-only, etc.).
Details can be found in the netmap man page and in the netmap API headers.

CHANGELOG:
 - dropped patch 2, since it requires further rearrangements on our side.

Vincenzo Maffione (1):
  net: netmap: use nm_open() to open netmap ports

 net/netmap.c | 97 
 1 file changed, 32 insertions(+), 65 deletions(-)

-- 
2.7.0

Re: [Qemu-devel] [PATCH 2/2] net: netmap: avoid mmap() when ports use the same shared memory area

[Vincenzo Maffione]

No, you're right, nm_mmap() is not defined for all netmap versions.

It will take a while for us to decide how to manage this. Would it be
possible to go ahead with patch 1/2 only (I will reissue the patch
series)?

Cheers,
  Vincenzo

2016-01-14 4:25 GMT+01:00 Jason Wang :
>
>
> On 01/08/2016 09:15 PM, Vincenzo Maffione wrote:
>> With this patch, nm_open() does not mmap() the netmap device. This
>> operation is performed separately only if the memory area of the
>> port just opened was not known before.
>> A global list of netmap clients is kept to check when matches
>> occur.
>>
>> Signed-off-by: Vincenzo Maffione 
>> ---
>>  net/netmap.c | 38 +-
>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/netmap.c b/net/netmap.c
>> index 27295ab..6a4c01c 100644
>> --- a/net/netmap.c
>> +++ b/net/netmap.c
>> @@ -49,8 +49,12 @@ typedef struct NetmapState {
>>  boolwrite_poll;
>>  struct ioveciov[IOV_MAX];
>>  int vnet_hdr_len;  /* Current virtio-net header length. 
>> */
>> +QTAILQ_ENTRY(NetmapState) next;
>>  } NetmapState;
>>
>> +static QTAILQ_HEAD(, NetmapState) netmap_clients =
>> +   QTAILQ_HEAD_INITIALIZER(netmap_clients);
>> +
>>  #ifndef __FreeBSD__
>>  #define pkt_copy bcopy
>>  #else
>> @@ -78,6 +82,23 @@ pkt_copy(const void *_src, void *_dst, int l)
>>  #endif /* __FreeBSD__ */
>>
>>  /*
>> + * find nm_desc parent with same allocator
>> + */
>> +static struct nm_desc *
>> +netmap_find_memory(struct nm_desc *nmd)
>> +{
>> +NetmapState *s;
>> +
>> +QTAILQ_FOREACH(s, &netmap_clients, next) {
>> +if (nmd->req.nr_arg2 == s->nmd->req.nr_arg2) {
>> +return s->nmd;
>> +}
>> +}
>> +
>> +return NULL;
>> +}
>> +
>> +/*
>>   * Open a netmap device. We assume there is only one queue
>>   * (which is the case for the VALE bridge).
>>   */
>> @@ -86,10 +107,11 @@ static struct nm_desc *netmap_open(const 
>> NetdevNetmapOptions *nm_opts,
>>  {
>>  struct nm_desc *nmd;
>>  struct nmreq req;
>> +int ret;
>>
>>  memset(&req, 0, sizeof(req));
>>
>> -nmd = nm_open(nm_opts->ifname, &req, NETMAP_NO_TX_POLL,
>> +nmd = nm_open(nm_opts->ifname, &req, NETMAP_NO_TX_POLL | 
>> NM_OPEN_NO_MMAP,
>>NULL);
>>  if (nmd == NULL) {
>>  error_setg_errno(errp, errno, "Failed to nm_open() %s",
>> @@ -97,6 +119,17 @@ static struct nm_desc *netmap_open(const 
>> NetdevNetmapOptions *nm_opts,
>>  return NULL;
>>  }
>>
>> +/* Check if we already have a nm_desc that uses the same memory as the 
>> one
>> + * just opened, so that nm_mmap() can skip mmap() and inherit from 
>> parent.
>> + */
>> +ret = nm_mmap(nmd, netmap_find_memory(nmd));
>
> Looks like I could not find nm_mmap() definition in neither qemu or
> freebsd source. Is there anything missed?
>
>> +if (ret) {
>> +error_setg_errno(errp, errno, "Failed to nm_mmap() %s",
>> + nm_opts->ifname);
>> +nm_close(nmd);
>> +return NULL;
>> +}
>> +
>>  return nmd;
>>  }
>>
>> @@ -321,6 +354,8 @@ static void netmap_cleanup(NetClientState *nc)
>>  netmap_poll(nc, false);
>>  nm_close(s->nmd);
>>  s->nmd = NULL;
>> +
>> +QTAILQ_REMOVE(&netmap_clients, s, next);
>>  }
>>
>>  /* Offloading manipulation support callbacks. */
>> @@ -423,6 +458,7 @@ int net_init_netmap(const NetClientOptions *opts,
>>  s->rx = NETMAP_RXRING(nmd->nifp, 0);
>>  s->vnet_hdr_len = 0;
>>  pstrcpy(s->ifname, sizeof(s->ifname), netmap_opts->ifname);
>> +QTAILQ_INSERT_TAIL(&netmap_clients, s, next);
>>  netmap_read_poll(s, true); /* Initially only poll for reads. */
>>
>>  return 0;
>



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH 09/10] virtio: read avail_idx from VQ only when necessary

[Vincenzo Maffione]

2016-01-19 19:48 GMT+01:00 Paolo Bonzini :
>
>
> On 19/01/2016 17:54, Michael S. Tsirkin wrote:
>> On Fri, Jan 15, 2016 at 01:41:57PM +0100, Paolo Bonzini wrote:
>>> From: Vincenzo Maffione 
>>>
>>> The virtqueue_pop() implementation needs to check if the avail ring
>>> contains some pending buffers. To perform this check, it is not
>>> always necessary to fetch the avail_idx in the VQ memory, which is
>>> expensive. This patch introduces a shadow variable tracking avail_idx
>>> and modifies virtio_queue_empty() to access avail_idx in physical
>>> memory only when necessary.
>>>
>>> Signed-off-by: Vincenzo Maffione 
>>> Message-Id: 
>>> 
>>> Signed-off-by: Paolo Bonzini 
>>
>> Is the cost due to the page walk?
>
> Yes, as with all the other patches.  But unlike patches 7 and 10 where
> we just reduce the number of walks, for patch 8 and 9 it's difficult to
> beat a local cache. :)
>
>>> @@ -1579,6 +1595,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
>>> version_id)
>>>  return -1;
>>>  }
>>>  vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
>>> +vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);
>>>  }
>>>  }
>>
>>
>> shadow_avail_idx also should be updated on vhost stop,
>
> That's virtio_queue_set_last_avail_idx, right?
>
> Paolo

Right. Sorry, I missed that.

Vincenzo



-- 
Vincenzo Maffione

[Qemu-devel] [PATCH 2/2] net: netmap: avoid mmap() when ports use the same shared memory area

[Vincenzo Maffione]

With this patch, nm_open() does not mmap() the netmap device. This
operation is performed separately only if the memory area of the
port just opened was not known before.
A global list of netmap clients is kept to check when matches
occur.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 38 +-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/net/netmap.c b/net/netmap.c
index 27295ab..6a4c01c 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -49,8 +49,12 @@ typedef struct NetmapState {
 boolwrite_poll;
 struct ioveciov[IOV_MAX];
 int vnet_hdr_len;  /* Current virtio-net header length. */
+QTAILQ_ENTRY(NetmapState) next;
 } NetmapState;
 
+static QTAILQ_HEAD(, NetmapState) netmap_clients =
+   QTAILQ_HEAD_INITIALIZER(netmap_clients);
+
 #ifndef __FreeBSD__
 #define pkt_copy bcopy
 #else
@@ -78,6 +82,23 @@ pkt_copy(const void *_src, void *_dst, int l)
 #endif /* __FreeBSD__ */
 
 /*
+ * find nm_desc parent with same allocator
+ */
+static struct nm_desc *
+netmap_find_memory(struct nm_desc *nmd)
+{
+NetmapState *s;
+
+QTAILQ_FOREACH(s, &netmap_clients, next) {
+if (nmd->req.nr_arg2 == s->nmd->req.nr_arg2) {
+return s->nmd;
+}
+}
+
+return NULL;
+}
+
+/*
  * Open a netmap device. We assume there is only one queue
  * (which is the case for the VALE bridge).
  */
@@ -86,10 +107,11 @@ static struct nm_desc *netmap_open(const 
NetdevNetmapOptions *nm_opts,
 {
 struct nm_desc *nmd;
 struct nmreq req;
+int ret;
 
 memset(&req, 0, sizeof(req));
 
-nmd = nm_open(nm_opts->ifname, &req, NETMAP_NO_TX_POLL,
+nmd = nm_open(nm_opts->ifname, &req, NETMAP_NO_TX_POLL | NM_OPEN_NO_MMAP,
   NULL);
 if (nmd == NULL) {
 error_setg_errno(errp, errno, "Failed to nm_open() %s",
@@ -97,6 +119,17 @@ static struct nm_desc *netmap_open(const 
NetdevNetmapOptions *nm_opts,
 return NULL;
 }
 
+/* Check if we already have a nm_desc that uses the same memory as the one
+ * just opened, so that nm_mmap() can skip mmap() and inherit from parent.
+ */
+ret = nm_mmap(nmd, netmap_find_memory(nmd));
+if (ret) {
+error_setg_errno(errp, errno, "Failed to nm_mmap() %s",
+ nm_opts->ifname);
+nm_close(nmd);
+return NULL;
+}
+
 return nmd;
 }
 
@@ -321,6 +354,8 @@ static void netmap_cleanup(NetClientState *nc)
 netmap_poll(nc, false);
 nm_close(s->nmd);
 s->nmd = NULL;
+
+QTAILQ_REMOVE(&netmap_clients, s, next);
 }
 
 /* Offloading manipulation support callbacks. */
@@ -423,6 +458,7 @@ int net_init_netmap(const NetClientOptions *opts,
 s->rx = NETMAP_RXRING(nmd->nifp, 0);
 s->vnet_hdr_len = 0;
 pstrcpy(s->ifname, sizeof(s->ifname), netmap_opts->ifname);
+QTAILQ_INSERT_TAIL(&netmap_clients, s, next);
 netmap_read_poll(s, true); /* Initially only poll for reads. */
 
 return 0;
-- 
2.7.0

[Qemu-devel] [PATCH 0/2] net: netmap: use nm_open() to open netmap port

[Vincenzo Maffione]

Current implementation of netmap backend still uses the low level netmap
API to open, register and close netmap ports. However, the netmap_user.h
header provides some helper functions that make these tasks easier, with
the additional advantage of extended interface name semantic.

As an example, the extended semantic allows to open a netmap pipe over
a VALE port by specifying an interface name in the form "valeXX:YY{ZZ".
Other suffixes are used for netmap monitors, exclusive access, and to
specify special binding mode (e.g. single queue, RX-only, TX-only, etc.).
Details can be found in the netmap man page and in the netmap API headers.

Vincenzo Maffione (2):
  net: netmap: use nm_open() to open netmap ports
  net: netmap: avoid mmap() when ports use the same shared memory area

 net/netmap.c | 129 ++-
 1 file changed, 66 insertions(+), 63 deletions(-)

-- 
2.7.0

[Qemu-devel] [PATCH 1/2] net: netmap: use nm_open() to open netmap ports

[Vincenzo Maffione]

This patch simplifies the netmap backend code by means of the nm_open()
helper function provided by netmap_user.h, which hides the details of
open(), iotcl() and mmap() carried out on the netmap device.

Moreover, the semantic of nm_open() makes it possible to open special
netmap ports (e.g. pipes, monitors) and use special modes (e.g. host rings
only, single queue mode, exclusive access).

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 97 
 1 file changed, 32 insertions(+), 65 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 5558368..27295ab 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -39,21 +39,12 @@
 #include "qemu/error-report.h"
 #include "qemu/iov.h"
 
-/* Private netmap device info. */
-typedef struct NetmapPriv {
-int fd;
-size_t  memsize;
-void*mem;
-struct netmap_if*nifp;
-struct netmap_ring  *rx;
-struct netmap_ring  *tx;
-charfdname[PATH_MAX];/* Normally "/dev/netmap". */
-charifname[IFNAMSIZ];
-} NetmapPriv;
-
 typedef struct NetmapState {
 NetClientState  nc;
-NetmapPriv  me;
+struct nm_desc  *nmd;
+charifname[IFNAMSIZ];
+struct netmap_ring  *tx;
+struct netmap_ring  *rx;
 boolread_poll;
 boolwrite_poll;
 struct ioveciov[IOV_MAX];
@@ -90,44 +81,23 @@ pkt_copy(const void *_src, void *_dst, int l)
  * Open a netmap device. We assume there is only one queue
  * (which is the case for the VALE bridge).
  */
-static void netmap_open(NetmapPriv *me, Error **errp)
+static struct nm_desc *netmap_open(const NetdevNetmapOptions *nm_opts,
+   Error **errp)
 {
-int fd;
-int err;
-size_t l;
+struct nm_desc *nmd;
 struct nmreq req;
 
-me->fd = fd = open(me->fdname, O_RDWR);
-if (fd < 0) {
-error_setg_file_open(errp, errno, me->fdname);
-return;
-}
 memset(&req, 0, sizeof(req));
-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_setg_errno(errp, errno, "Unable to register %s", me->ifname);
-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_setg_errno(errp, errno, "Unable to mmap netmap shared memory");
-me->mem = NULL;
-goto error;
+nmd = nm_open(nm_opts->ifname, &req, NETMAP_NO_TX_POLL,
+  NULL);
+if (nmd == NULL) {
+error_setg_errno(errp, errno, "Failed to nm_open() %s",
+ nm_opts->ifname);
+return NULL;
 }
 
-me->nifp = NETMAP_IF(me->mem, req.nr_offset);
-me->tx = NETMAP_TXRING(me->nifp, 0);
-me->rx = NETMAP_RXRING(me->nifp, 0);
-
-return;
-
-error:
-close(me->fd);
+return nmd;
 }
 
 static void netmap_send(void *opaque);
@@ -136,7 +106,7 @@ static void netmap_writable(void *opaque);
 /* Set the event-loop handlers for the netmap backend. */
 static void netmap_update_fd_handler(NetmapState *s)
 {
-qemu_set_fd_handler(s->me.fd,
+qemu_set_fd_handler(s->nmd->fd,
 s->read_poll ? netmap_send : NULL,
 s->write_poll ? netmap_writable : NULL,
 s);
@@ -188,7 +158,7 @@ static ssize_t netmap_receive(NetClientState *nc,
   const uint8_t *buf, size_t size)
 {
 NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
-struct netmap_ring *ring = s->me.tx;
+struct netmap_ring *ring = s->tx;
 uint32_t i;
 uint32_t idx;
 uint8_t *dst;
@@ -218,7 +188,7 @@ static ssize_t netmap_receive(NetClientState *nc,
 ring->slot[i].flags = 0;
 pkt_copy(buf, dst, size);
 ring->cur = ring->head = nm_ring_next(ring, i);
-ioctl(s->me.fd, NIOCTXSYNC, NULL);
+ioctl(s->nmd->fd, NIOCTXSYNC, NULL);
 
 return size;
 }
@@ -227,7 +197,7 @@ 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;
+struct netmap_ring *ring = s->tx;
 uint32_t last;
 uint32_t idx;
 uint8_t *dst;
@@ -284,7 +254,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 /* Now update ring->cur and ring->head. */
 ring->cur = ring->head = i;
 
-ioctl(s->me.fd, NIOCTXSYNC, NULL);
+ioctl(s->nmd->fd, NIOCTXSYNC, NULL);
 
 return iov_size(iov, iovcnt);
 }
@@ -301,7 +271,7 @@ static void netmap_s

Re: [Qemu-devel] [PATCH v2 0/3] virtio: proposal to optimize accesses to VQs

[Vincenzo Maffione]

2015-12-16 16:46 GMT+01:00 Paolo Bonzini :
>
>
> On 16/12/2015 15:25, Vincenzo Maffione wrote:
>>> vhost-net actually had better performance, so virtio-net dataplane
>>> was never committed.  As Michael mentioned, in practice on Linux you
>>> use vhost, and non-Linux hypervisors you do not use QEMU. :)
>>
>> Yes, I understand. However, another possible use-case would using QEMU
>> + virtio-net + netmap backend + Linux (e.g. for QEMU-sandboxed packet
>> generators or packe processors, where very high packet rates are
>> common), where is not possible to use vhost.
>
> Yes, of course.  That was tongue in cheek.  Another possibility for your
> use case is to interface with netmap through vhost-user, but I'm happy
> if you choose to improve virtio.c instead!
>
>>> The main optimization that vring.c has is to cache the translation of
>>> the rings.  Using address_space_map/unmap for rings in virtio.c would be
>>> a noticeable improvement, as your numbers for patch 3 show.  However, by
>>> caching translations you also conveniently "forget" to promptly mark the
>>> pages as dirty.  As you pointed out this is obviously an issue for
>>> migration.  You can then add a notifier for runstate changes.  When
>>> entering RUN_STATE_FINISH_MIGRATE or RUN_STATE_SAVE_VM the rings would
>>> be unmapped, and then remapped the next time the VM starts running again.
>>
>> Ok so it seems feasible with a bit of care. The numbers we've been
>> seing in various experiments have always shown that this optimization
>> could easily double the 2 Mpps packet rate bottleneck.
>
> Cool.  Bonus points for nicely abstracting it so that virtio.c is just a
> user.
>
>>> You also guessed right that there are consistency issues; for these you
>>> can add a MemoryListener that invalidates all mappings.
>>
>> Yeah, but I don't know exactly what kind of inconsinstencies there can
>> be. Maybe the memory we are mapping may be hot-unplugged?
>
> Yes.  Just blow away all mappings in the MemoryListener commit callback.
>
>>> That said, I'm wondering where the cost of address translation lies---is
>>> it cache-unfriendly data structures, locked operations, or simply too
>>> much code to execute?  It was quite surprising to me that on virtio-blk
>>> benchmarks we were spending 5% of the time doing memcpy! (I have just
>>> extracted from my branch the patches to remove that, and sent them to
>>> qemu-devel).
>>
>> I feel it's just too much code (but I may be wrong).
>
> That is likely to be a good guess, but notice that the fast path doesn't
> actually have _that much_ code, because a lot of "if"s that are almost
> always false.
>
> Looking at a profile would be useful.  Is it flat, or does something
> (e.g. address_space_translate) actually stand out?

I'm so sorry, I forget to answer this.

This is what perf top shows while doing the experiment

  12.35%  qemu-system-x86_64   [.] address_space_map
  10.87%  qemu-system-x86_64   [.] vring_desc_read.isra.0
   7.50%  qemu-system-x86_64   [.] address_space_lduw_le
   6.32%  qemu-system-x86_64   [.] address_space_translate
   5.84%  qemu-system-x86_64   [.] address_space_translate_internal
   5.75%  qemu-system-x86_64   [.] phys_page_find
   5.74%  qemu-system-x86_64   [.] qemu_ram_block_from_host
   4.04%  qemu-system-x86_64   [.] address_space_stw_le
   4.02%  qemu-system-x86_64   [.] address_space_write
   3.33%  qemu-system-x86_64   [.] virtio_should_notify

So it seems most of the time is spent while doing translations.

>
>> I'm not sure whether you are thinking that 5% is too much or too little.
>> To me it's too little, showing that most of the overhead it's
>> somewhere else (e.g. memory translation, or backend processing). In a
>> ideal transmission system, most of the overhead should be spent on
>> copying, because it means that you successfully managed to suppress
>> notifications and translation overhead.
>
> On copying data, though---not on copying virtio descriptors.  5% for
> those is entirely wasted time.
>
> Also, note that I'm looking at disk I/O rather than networking, where
> there should be no copies at all.

In the experiment I'm doing there is a per-packet copy from the guest
memory to the netmap backend.

Cheers,
  Vincenzo

>
> Paolo
>
>>> Examples of missing optimizations in exec.c include:
>>>
>>> * caching enough information in RAM MemoryRegions to avoid the calls to
>>> qemu_get_ram_block (e.g. replace mr->ram_addr with a RAMBlock pointer);
>>>
>>> * adding a MRU cache to address_space_lookup_region.
>>>
>>> In particular, the former should be easy if you want to give it a
>>> try---easier than caching ring translations in virtio.c.
>>
>> Thank you so much for the insights :)
>



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH v2 0/3] virtio: proposal to optimize accesses to VQs

[Vincenzo Maffione]

2015-12-16 12:37 GMT+01:00 Paolo Bonzini :
>
>
> On 16/12/2015 11:39, Vincenzo Maffione wrote:
>> No problems.
>>
>> I have some additional (orthogonal) curiosities:
>>
>>   1) Assuming "hw/virtio/dataplane/vring.c" is what I think it is (VQ
>> data structures directly accessible in the host virtual memory, with
>> guest-phyisical-to-host-virtual mapping done statically at setup time)
>> why isn't QEMU using this approach also for virtio-net? I see it is
>> used by virtio-blk only.
>
> Yes, it's a good idea.  vring.c's roots are in the old "virtio-blk
> dataplane" code, which bypassed all of QEMU.  It had a separate event
> loop, a separate I/O queue, a separate implementation of the memory map,
> and a separate implementation of virtio.  virtio-blk dataplane is plenty
> fast, which is why there isn't a vhost-blk.
>
> Right now the only part that survives is the last one, which is vring.c.
>  The event loop has been reconciled with AioContext, the I/O queue uses
> BlockDriverState, and the memory map uses memory_region_find.
>
> virtio-net dataplane also existed, as a possible competitor of
> vhost-net.  However, vhost-net actually had better performance, so
> virtio-net dataplane was never committed.  As Michael mentioned, in
> practice on Linux you use vhost, and non-Linux hypervisors you do not
> use QEMU. :)

Yes, I understand. However, another possible use-case would using QEMU
+ virtio-net + netmap backend + Linux (e.g. for QEMU-sandboxed packet
generators or packe processors, where very high packet rates are
common), where is not possible to use vhost.

>
> Indeed the implementation in virtio.c does kind of suck.  On the other
> hand, vring.c doesn't support migration because it doesn't know how to
> mark guest memory as dirty.  And virtio.c is used by plenty of
> devices---including virtio-blk and virtio-scsi unless you enable usage
> of a separate I/O thread---and converting them to vring.c is bug-prone.
> This is why I would like to drop vring.c and improve virtio.c, rather
> than use vring.c even more.

+1

>
> The main optimization that vring.c has is to cache the translation of
> the rings.  Using address_space_map/unmap for rings in virtio.c would be
> a noticeable improvement, as your numbers for patch 3 show.  However, by
> caching translations you also conveniently "forget" to promptly mark the
> pages as dirty.  As you pointed out this is obviously an issue for
> migration.  You can then add a notifier for runstate changes.  When
> entering RUN_STATE_FINISH_MIGRATE or RUN_STATE_SAVE_VM the rings would
> be unmapped, and then remapped the next time the VM starts running again.
>

Ok so it seems feasible with a bit of care. The numbers we've been
seing in various experiments have always shown that this optimization
could easily double the 2 Mpps packet rate bottleneck.

> You also guessed right that there are consistency issues; for these you
> can add a MemoryListener that invalidates all mappings.

Yeah, but I don't know exactly what kind of inconsinstencies there can
be. Maybe the memory we are mapping may be hot-unplugged?

>
> That said, I'm wondering where the cost of address translation lies---is
> it cache-unfriendly data structures, locked operations, or simply too
> much code to execute?  It was quite surprising to me that on virtio-blk
> benchmarks we were spending 5% of the time doing memcpy! (I have just
> extracted from my branch the patches to remove that, and sent them to
> qemu-devel).

I feel it's just too much code (but I may be wrong).
I'm not sure whether you are thinking that 5% is too much or too little.
To me it's too little, showing that most of the overhead it's
somewhere else (e.g. memory translation, or backend processing). In a
ideal transmission system, most of the overhead should be spent on
copying, because it means that you successfully managed to suppress
notifications and translation overhead.

I've tried out your patches, but unfortunately I don't see any effect
on my transmission tests (fast transmission over virtio-net with
disconnected backend).

>
> Examples of missing optimizations in exec.c include:
>
> * caching enough information in RAM MemoryRegions to avoid the calls to
> qemu_get_ram_block (e.g. replace mr->ram_addr with a RAMBlock pointer);
>
> * adding a MRU cache to address_space_lookup_region.
>
> In particular, the former should be easy if you want to give it a
> try---easier than caching ring translations in virtio.c.
>
> Paolo

Thank you so much for the insights :)

Cheers,
  Vincenzo

Re: [Qemu-devel] [PATCH v2 0/3] virtio: proposal to optimize accesses to VQs

[Vincenzo Maffione]

2015-12-16 12:02 GMT+01:00 Michael S. Tsirkin :
> On Wed, Dec 16, 2015 at 11:39:46AM +0100, Vincenzo Maffione wrote:
>> 2015-12-16 10:34 GMT+01:00 Paolo Bonzini :
>> >
>> >
>> > On 16/12/2015 10:28, Vincenzo Maffione wrote:
>> >> Assuming my TX experiments with disconnected backend (and I disable
>> >> CPU dynamic scaling of performance, etc.):
>> >>   1) after patch 1 and 2, virtio bottleneck jumps from ~1Mpps to 1.910 
>> >> Mpps.
>> >>   2) after patch 1,2 and 3, virtio bottleneck jumps to 2.039 Mpps.
>> >>
>> >> So I see an improvement for patch 3, and I guess it's because we avoid
>> >> an additional memory translation and related overhead. I believe that
>> >> avoiding the memory translation is more beneficial than avoiding the
>> >> variable-sized memcpy.
>> >> I'm not surprised of that, because taking a brief look at what happens
>> >> under the hood when you call an access_memory() function - it looks
>> >> like a lot of operations.
>> >
>> > Great, thanks for confirming!
>> >
>> > Paolo
>>
>> No problems.
>>
>> I have some additional (orthogonal) curiosities:
>>
>>   1) Assuming "hw/virtio/dataplane/vring.c" is what I think it is (VQ
>> data structures directly accessible in the host virtual memory, with
>> guest-phyisical-to-host-virtual mapping done statically at setup time)
>> why isn't QEMU using this approach also for virtio-net? I see it is
>> used by virtio-blk only.
>
> Because on Linux, nothing would be gained as compared to using vhost-net
> in kernel or vhost-user with dpdk.  virtio-net is there for non-Linux
> hosts, keeping it simple is important to avoid e.g. security problems.
> Same as serial, etc.

Ok, thanks for the clarification.

>
>>   2) In any case (vring or not) QEMU dynamically maps data buffers
>> from guest physical memory, for each descriptor to be processed: e1000
>> uses pci_dma_read/pci_dma_write, virtio uses
>> cpu_physical_memory_map()/cpu_physical_memory_unmap(), vring uses the
>> more specialied vring_map()/vring_unmap(). All of these go through
>> expensive lookups and related operations to do the address
>> translation.
>> Have you considered the possibility to cache the translation result to
>> remove this bottleneck (maybe just for virtio devices)? Or is any
>> consistency or migration-related problem that would create issues?
>> Just to give an example of what I'm talking about:
>> https://github.com/vmaffione/qemu/blob/master/hw/net/e1000.c#L349-L423.
>>
>> At very high packet rates, once notifications (kicks and interrupts)
>> have been amortized in some way, memory translation becomes the major
>> bottleneck. And this (1 and 2) is why QEMU virtio implementation
>> cannot achieve the same throughput as bhyve does (5-6 Mpps or more
>> IIRC).
>>
>> Cheers,
>>   Vincenzo
>>
>>
>>
>> --
>> Vincenzo Maffione



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH v2 0/3] virtio: proposal to optimize accesses to VQs

[Vincenzo Maffione]

2015-12-16 10:34 GMT+01:00 Paolo Bonzini :
>
>
> On 16/12/2015 10:28, Vincenzo Maffione wrote:
>> Assuming my TX experiments with disconnected backend (and I disable
>> CPU dynamic scaling of performance, etc.):
>>   1) after patch 1 and 2, virtio bottleneck jumps from ~1Mpps to 1.910 Mpps.
>>   2) after patch 1,2 and 3, virtio bottleneck jumps to 2.039 Mpps.
>>
>> So I see an improvement for patch 3, and I guess it's because we avoid
>> an additional memory translation and related overhead. I believe that
>> avoiding the memory translation is more beneficial than avoiding the
>> variable-sized memcpy.
>> I'm not surprised of that, because taking a brief look at what happens
>> under the hood when you call an access_memory() function - it looks
>> like a lot of operations.
>
> Great, thanks for confirming!
>
> Paolo

No problems.

I have some additional (orthogonal) curiosities:

  1) Assuming "hw/virtio/dataplane/vring.c" is what I think it is (VQ
data structures directly accessible in the host virtual memory, with
guest-phyisical-to-host-virtual mapping done statically at setup time)
why isn't QEMU using this approach also for virtio-net? I see it is
used by virtio-blk only.

  2) In any case (vring or not) QEMU dynamically maps data buffers
from guest physical memory, for each descriptor to be processed: e1000
uses pci_dma_read/pci_dma_write, virtio uses
cpu_physical_memory_map()/cpu_physical_memory_unmap(), vring uses the
more specialied vring_map()/vring_unmap(). All of these go through
expensive lookups and related operations to do the address
translation.
Have you considered the possibility to cache the translation result to
remove this bottleneck (maybe just for virtio devices)? Or is any
consistency or migration-related problem that would create issues?
Just to give an example of what I'm talking about:
https://github.com/vmaffione/qemu/blob/master/hw/net/e1000.c#L349-L423.

At very high packet rates, once notifications (kicks and interrupts)
have been amortized in some way, memory translation becomes the major
bottleneck. And this (1 and 2) is why QEMU virtio implementation
cannot achieve the same throughput as bhyve does (5-6 Mpps or more
IIRC).

Cheers,
  Vincenzo



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH v2 0/3] virtio: proposal to optimize accesses to VQs

[Vincenzo Maffione]

Assuming my TX experiments with disconnected backend (and I disable
CPU dynamic scaling of performance, etc.):
  1) after patch 1 and 2, virtio bottleneck jumps from ~1Mpps to 1.910 Mpps.
  2) after patch 1,2 and 3, virtio bottleneck jumps to 2.039 Mpps.

So I see an improvement for patch 3, and I guess it's because we avoid
an additional memory translation and related overhead. I believe that
avoiding the memory translation is more beneficial than avoiding the
variable-sized memcpy.
I'm not surprised of that, because taking a brief look at what happens
under the hood when you call an access_memory() function - it looks
like a lot of operations.

Cheers,
  Vincenzo

2015-12-16 9:38 GMT+01:00 Paolo Bonzini :
>
>
> On 15/12/2015 23:33, Vincenzo Maffione wrote:
>> This patch slightly rewrites the code to reduce the number of accesses, since
>> many of them seems unnecessary to me. After this reduction, the bottleneck
>> jumps from 1 Mpps to 2 Mpps.
>
> Very nice.  Did you get new numbers with the rebase?  That would help
> measuring the effect of removing variable-sized memcpy (I'll post the
> patches for this shortly; they're entirely in memory.h/exec.c so they're
> not virtio-specific).  A rough measurement from "perf" says they're
> worth about 5%.
>
> Related to this, patch 3 introduces a variable-sized memcpy, because it
> switches from 2 virtio_stl_phys to 1 address_space_write.  I'm curious
> if the effect of this individual patch is positive, negative or neutral.
>  On the other hand, patches 1 and 2 are clear wins.
>
> Paolo
>
>> Patch is not complete (e.g. it still does not properly manage endianess, it 
>> is
>> not clean, etc.). I just wanted to ask if you think the idea makes sense, and
>> a proper patch in this direction would be accepted.



-- 
Vincenzo Maffione

[Qemu-devel] [PATCH v2 1/3] virtio: cache used_idx in a VirtQueue field

[Vincenzo Maffione]

Accessing used_idx in the VQ requires an expensive access to
guest physical memory. Before this patch, 3 accesses are normally
done for each pop/push/notify call. However, since the used_idx is
only written by us, we can track it in our internal data structure.

Signed-off-by: Vincenzo Maffione 
---
 hw/virtio/virtio.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 925bb36..a35bb38 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -71,6 +71,9 @@ struct VirtQueue
 {
 VRing vring;
 uint16_t last_avail_idx;
+
+uint16_t used_idx;
+
 /* Last used index value we have signalled on */
 uint16_t signalled_used;
 
@@ -170,6 +173,7 @@ static inline void vring_used_idx_set(VirtQueue *vq, 
uint16_t val)
 hwaddr pa;
 pa = vq->vring.used + offsetof(VRingUsed, idx);
 virtio_stw_phys(vq->vdev, pa, val);
+vq->used_idx = val;
 }
 
 static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
@@ -261,7 +265,7 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement 
*elem,
 
 virtqueue_unmap_sg(vq, elem, len);
 
-idx = (idx + vring_used_idx(vq)) % vq->vring.num;
+idx = (idx + vq->used_idx) % vq->vring.num;
 
 /* Get a pointer to the next entry in the used ring. */
 vring_used_ring_id(vq, idx, elem->index);
@@ -274,7 +278,7 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
 /* Make sure buffer is written before we update index. */
 smp_wmb();
 trace_virtqueue_flush(vq, count);
-old = vring_used_idx(vq);
+old = vq->used_idx;
 new = old + count;
 vring_used_idx_set(vq, new);
 vq->inuse -= count;
@@ -806,6 +810,7 @@ void virtio_reset(void *opaque)
 vdev->vq[i].vring.avail = 0;
 vdev->vq[i].vring.used = 0;
 vdev->vq[i].last_avail_idx = 0;
+vdev->vq[i].used_idx = 0;
 virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
 vdev->vq[i].signalled_used = 0;
 vdev->vq[i].signalled_used_valid = false;
@@ -1189,7 +1194,7 @@ bool virtio_should_notify(VirtIODevice *vdev, VirtQueue 
*vq)
 v = vq->signalled_used_valid;
 vq->signalled_used_valid = true;
 old = vq->signalled_used;
-new = vq->signalled_used = vring_used_idx(vq);
+new = vq->signalled_used = vq->used_idx;
 return !v || vring_need_event(vring_get_used_event(vq), new, old);
 }
 
@@ -1650,6 +1655,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
version_id)
  vdev->vq[i].last_avail_idx, nheads);
 return -1;
 }
+vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
 }
 }
 
-- 
2.6.4

[Qemu-devel] [PATCH v2 2/3] virtio: read avail_idx from VQ only when necessary

[Vincenzo Maffione]

The virtqueue_pop() implementation needs to check if the avail ring
contains some pending buffers. To perform this check, it is not
always necessary to fetch the avail_idx in the VQ memory, which is
expensive. This patch introduces a shadow variable tracking avail_idx
and modifies virtio_queue_empty() to access avail_idx in physical
memory only when necessary.

Signed-off-by: Vincenzo Maffione 
---
 hw/virtio/virtio.c | 25 +
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a35bb38..5a2ea8a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -70,8 +70,13 @@ typedef struct VRing
 struct VirtQueue
 {
 VRing vring;
+
+/* Next head to pop */
 uint16_t last_avail_idx;
 
+/* Last avail_idx read from VQ. */
+uint16_t shadow_avail_idx;
+
 uint16_t used_idx;
 
 /* Last used index value we have signalled on */
@@ -132,7 +137,8 @@ static inline uint16_t vring_avail_idx(VirtQueue *vq)
 {
 hwaddr pa;
 pa = vq->vring.avail + offsetof(VRingAvail, idx);
-return virtio_lduw_phys(vq->vdev, pa);
+vq->shadow_avail_idx = virtio_lduw_phys(vq->vdev, pa);
+return vq->shadow_avail_idx;
 }
 
 static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
@@ -223,8 +229,14 @@ int virtio_queue_ready(VirtQueue *vq)
 return vq->vring.avail != 0;
 }
 
+/* Fetch avail_idx from VQ memory only when we really need to know if
+ * guest has added some buffers. */
 int virtio_queue_empty(VirtQueue *vq)
 {
+if (vq->shadow_avail_idx != vq->last_avail_idx) {
+return 0;
+}
+
 return vring_avail_idx(vq) == vq->last_avail_idx;
 }
 
@@ -300,7 +312,7 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned int 
idx)
 /* Check it isn't doing very strange things with descriptor numbers. */
 if (num_heads > vq->vring.num) {
 error_report("Guest moved used index from %u to %u",
- idx, vring_avail_idx(vq));
+ idx, vq->shadow_avail_idx);
 exit(1);
 }
 /* On success, callers read a descriptor at vq->last_avail_idx.
@@ -534,9 +546,12 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 struct iovec iov[VIRTQUEUE_MAX_SIZE];
 VRingDesc desc;
 
-if (!virtqueue_num_heads(vq, vq->last_avail_idx)) {
+if (virtio_queue_empty(vq)) {
 return NULL;
 }
+/* Needed after virtio_queue_empty(), see comment in
+ * virtqueue_num_heads(). */
+smp_rmb();
 
 /* When we start there are none of either input nor output. */
 out_num = in_num = 0;
@@ -810,6 +825,7 @@ void virtio_reset(void *opaque)
 vdev->vq[i].vring.avail = 0;
 vdev->vq[i].vring.used = 0;
 vdev->vq[i].last_avail_idx = 0;
+vdev->vq[i].shadow_avail_idx = 0;
 vdev->vq[i].used_idx = 0;
 virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
 vdev->vq[i].signalled_used = 0;
@@ -1183,7 +1199,7 @@ bool virtio_should_notify(VirtIODevice *vdev, VirtQueue 
*vq)
 smp_mb();
 /* Always notify when queue is empty (when feature acknowledge) */
 if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFY_ON_EMPTY) &&
-!vq->inuse && vring_avail_idx(vq) == vq->last_avail_idx) {
+!vq->inuse && virtio_queue_empty(vq)) {
 return true;
 }
 
@@ -1656,6 +1672,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
version_id)
 return -1;
 }
 vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
+vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);
 }
 }
 
-- 
2.6.4

[Qemu-devel] [PATCH v2 3/3] virtio: combine write of an entry into used ring

[Vincenzo Maffione]

Fill in an element of the used ring with a single combined access to the
guest physical memory, rather than using two separated accesses.
This reduces the overhead due to expensive address translation.

Signed-off-by: Vincenzo Maffione 
---
 hw/virtio/virtio.c | 25 -
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5a2ea8a..ecdb140 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -153,18 +153,15 @@ static inline uint16_t vring_get_used_event(VirtQueue *vq)
 return vring_avail_ring(vq, vq->vring.num);
 }
 
-static inline void vring_used_ring_id(VirtQueue *vq, int i, uint32_t val)
+static inline void vring_used_write(VirtQueue *vq, VRingUsedElem *uelem,
+int i)
 {
 hwaddr pa;
-pa = vq->vring.used + offsetof(VRingUsed, ring[i].id);
-virtio_stl_phys(vq->vdev, pa, val);
-}
-
-static inline void vring_used_ring_len(VirtQueue *vq, int i, uint32_t val)
-{
-hwaddr pa;
-pa = vq->vring.used + offsetof(VRingUsed, ring[i].len);
-virtio_stl_phys(vq->vdev, pa, val);
+virtio_tswap32s(vq->vdev, &uelem->id);
+virtio_tswap32s(vq->vdev, &uelem->len);
+pa = vq->vring.used + offsetof(VRingUsed, ring[i]);
+address_space_write(&address_space_memory, pa, MEMTXATTRS_UNSPECIFIED,
+   (void *)uelem, sizeof(VRingUsedElem));
 }
 
 static uint16_t vring_used_idx(VirtQueue *vq)
@@ -273,15 +270,17 @@ void virtqueue_discard(VirtQueue *vq, const 
VirtQueueElement *elem,
 void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
 unsigned int len, unsigned int idx)
 {
+VRingUsedElem uelem;
+
 trace_virtqueue_fill(vq, elem, len, idx);
 
 virtqueue_unmap_sg(vq, elem, len);
 
 idx = (idx + vq->used_idx) % vq->vring.num;
 
-/* Get a pointer to the next entry in the used ring. */
-vring_used_ring_id(vq, idx, elem->index);
-vring_used_ring_len(vq, idx, len);
+uelem.id = elem->index;
+uelem.len = len;
+vring_used_write(vq, &uelem, idx);
 }
 
 void virtqueue_flush(VirtQueue *vq, unsigned int count)
-- 
2.6.4

[Qemu-devel] [PATCH v2 0/3] virtio: proposal to optimize accesses to VQs

[Vincenzo Maffione]

Hi,
  I am doing performance experiments to test how QEMU behaves when the
guest is transmitting (short) network packets at very high packet rates, say
over 1Mpps.
I run a netmap application in the guest to generate high packet rates,
but this is not relevant to this discussion. The only important fact is that
the generator running in the guest is not the bottleneck, and in fact CPU
utilization is low (20%).

Moreover, I'm not considering vhost-net to boost virtio-net HV-side processing,
because I want to do performance unit-tests on the QEMU virtio userspace
implementation (hw/virtio/virtio.c).

In the most common benchmarks - e.g. netperf TCP_STREAM, TCP_RR,
UDP_STREAM, ..., with one end of the communication in the guest, and the
other in the host, for instance with the simplest TAP networking setup - the
virtio-net adapter definitely outperforms the emulated e1000 adapter (and
all the other emulated devices). This was expected because of the great
benefits of I/O paravirtualization.

However, I was surprised to find out that the situation changes drastically
at very high packet rates.

My measurements show that e1000 emulated adapter is able to transmit over
3.5 Mpps when the network backend is disconnected. I disconnect the
backend to see at what packet rate e1000 becomes the bottleneck.

The same experiment, however, shows that virtio-net has a bottleneck at
1Mpps only. Once verified that the TX VQ kicks and TX VQ interrupts are
properly amortized/suppressed, I found out that the bottleneck is partially
due to the way the code accesses the VQ in the guest physical memory, since
each access involves an expensive address space translation. For each VQ
element to process, I counted over 15 accesses, while e1000 has just 2 accesses
to its rings.

This patch slightly rewrites the code to reduce the number of accesses, since
many of them seems unnecessary to me. After this reduction, the bottleneck
jumps from 1 Mpps to 2 Mpps.

Patch is not complete (e.g. it still does not properly manage endianess, it is
not clean, etc.). I just wanted to ask if you think the idea makes sense, and
a proper patch in this direction would be accepted.

Thanks,
  Vincenzo


CHANGELOG:
- rebased on bonzini/dataplane branch
- removed optimization (combined descriptor read) already present in
  the dataplane branch
- 3 separate optimizations splitted into 3 patches

Vincenzo Maffione (3):
  virtio: cache used_idx in a VirtQueue field
  virtio: read avail_idx from VQ only when necessary
  virtio: combine write of an entry into used ring

 hw/virtio/virtio.c | 62 --
 1 file changed, 42 insertions(+), 20 deletions(-)

-- 
2.6.4

Re: [Qemu-devel] [PATCH RFC] virtio: proposal to optimize accesses to VQs

[Vincenzo Maffione]

> @@ -415,15 +399,15 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned 
> int *in_bytes,
>  exit(1);
>  }
>
> -if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_WRITE) {
> -in_total += vring_desc_len(vdev, desc_pa, i);
> +if (desc.flags & VRING_DESC_F_WRITE) {
> +in_total += desc.flags;
>  } else {
> -out_total += vring_desc_len(vdev, desc_pa, i);
> +out_total += desc.flags;
>  }
>  if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
>  goto done;
>  }
> -} while ((i = virtqueue_next_desc(vdev, desc_pa, i, max)) != max);
> +} while ((i = virtqueue_read_next_desc(vdev, &desc, desc_pa, max)) 
> != max);
>
>  if (!indirect)
>  total_bufs = num_bufs;

Here (http://article.gmane.org/gmane.comp.emulators.qemu.block/6620) you meant

+in_total += desc.len;

and

+out_total += desc.len;


2015-12-14 17:06 GMT+01:00 Paolo Bonzini :
>
>
> On 14/12/2015 15:51, Vincenzo Maffione wrote:
>>
>> This patch slightly rewrites the code to reduce the number of accesses, since
>> many of them seems unnecessary to me. After this reduction, the bottleneck
>> jumps from 1 Mpps to 2 Mpps.
>>
>> Patch is not complete (e.g. it still does not properly manage endianess, it 
>> is
>> not clean, etc.). I just wanted to ask if you think the idea makes sense, and
>> a proper patch in this direction would be accepted.
>
> Yes, definitely.   I have even done a very similar change here:
>
> http://article.gmane.org/gmane.comp.emulators.qemu.block/6620
>
> Can you review that patch, and possibly work on top of it?
>
> Thanks,
>
> Paolo



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH RFC] virtio: proposal to optimize accesses to VQs

[Vincenzo Maffione]

2015-12-14 17:06 GMT+01:00 Paolo Bonzini :
>
>
> On 14/12/2015 15:51, Vincenzo Maffione wrote:
>>
>> This patch slightly rewrites the code to reduce the number of accesses, since
>> many of them seems unnecessary to me. After this reduction, the bottleneck
>> jumps from 1 Mpps to 2 Mpps.
>>
>> Patch is not complete (e.g. it still does not properly manage endianess, it 
>> is
>> not clean, etc.). I just wanted to ask if you think the idea makes sense, and
>> a proper patch in this direction would be accepted.
>
> Yes, definitely.   I have even done a very similar change here:
>
> http://article.gmane.org/gmane.comp.emulators.qemu.block/6620
>
> Can you review that patch, and possibly work on top of it?
>
> Thanks,
>
> Paolo

Yes, I will take a look at it.

Cheers,
  Vincenzo


-- 
Vincenzo Maffione

[Qemu-devel] [PATCH RFC] virtio: proposal to optimize accesses to VQs

[Vincenzo Maffione]

Hi,
  I am doing performance experiments to test how QEMU behaves when the
guest is transmitting (short) network packets at very high packet rates, say
over 1Mpps.
I run a netmap application in the guest to generate high packet rates,
but this is not relevant to this discussion. The only important fact is that
the generator running in the guest is not the bottleneck, and in fact CPU
utilization is low (20%).

Moreover, I'm not considering vhost-net to boost virtio-net HV-side processing,
because I want to do performance unit-tests on the QEMU virtio userspace
implementation (hw/virtio/virtio.c).

In the most common benchmarks - e.g. netperf TCP_STREAM, TCP_RR,
UDP_STREAM, ..., with one end of the communication in the guest, and the
other in the host, for instance with the simplest TAP networking setup - the
virtio-net adapter definitely outperforms the emulated e1000 adapter (and
all the other emulated devices). This was expected because of the great
benefits of I/O paravirtualization.

However, I was surprised to find out that the situation changes drastically
at very high packet rates.

My measurements show that e1000 emulated adapter is able to transmit over
3.5 Mpps when the network backend is disconnected. I disconnect the
backend to see at what packet rate e1000 becomes the bottleneck.

The same experiment, however, shows that virtio-net has a bottleneck at
1Mpps only. Once verified that the TX VQ kicks and TX VQ interrupts are
properly amortized/suppressed, I found out that the bottleneck is partially
due to the way the code accesses the VQ in the guest physical memory, since
each access involves an expensive address space translation. For each VQ
element to process, I counted over 15 accesses, while e1000 has just 2 accesses
to its rings.

This patch slightly rewrites the code to reduce the number of accesses, since
many of them seems unnecessary to me. After this reduction, the bottleneck
jumps from 1 Mpps to 2 Mpps.

Patch is not complete (e.g. it still does not properly manage endianess, it is
not clean, etc.). I just wanted to ask if you think the idea makes sense, and
a proper patch in this direction would be accepted.

Thanks,
  Vincenzo

Vincenzo Maffione (1):
  virtio: optimize access to guest physical memory

 hw/virtio/virtio.c | 118 +++--
 1 file changed, 88 insertions(+), 30 deletions(-)

-- 
2.6.3

[Qemu-devel] [PATCH RFC] virtio: optimize access to guest physical memory

[Vincenzo Maffione]

Accessing a location of guest physical memory involves a translation
from guest physical addresses to host virtual address. This patch
removes unnecessary accesses by virtio implementation, so that at
high processing rates the bottleneck goes from ~1Mtts to ~2Mtts.

Signed-off-by: Vincenzo Maffione 
---
 hw/virtio/virtio.c | 118 +++--
 1 file changed, 88 insertions(+), 30 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 1edef59..1f853d6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -21,6 +21,7 @@
 #include "hw/virtio/virtio-bus.h"
 #include "migration/migration.h"
 #include "hw/virtio/virtio-access.h"
+#include "exec/address-spaces.h"
 
 /*
  * The alignment to use between consumer and producer parts of vring.
@@ -71,6 +72,8 @@ struct VirtQueue
 {
 VRing vring;
 uint16_t last_avail_idx;
+uint16_t last_fetched_avail_idx;
+uint16_t last_used_idx;
 /* Last used index value we have signalled on */
 uint16_t signalled_used;
 
@@ -138,6 +141,15 @@ static inline uint16_t vring_desc_next(VirtIODevice *vdev, 
hwaddr desc_pa,
 return virtio_lduw_phys(vdev, pa);
 }
 
+static inline void
+vring_desc_fetch(VirtIODevice *vdev, hwaddr desc_pa, int i, VRingDesc *desc)
+{
+hwaddr pa;
+pa = desc_pa + sizeof(VRingDesc) * i;
+address_space_rw(&address_space_memory, pa, MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)desc, sizeof(VRingDesc), false);
+}
+
 static inline uint16_t vring_avail_flags(VirtQueue *vq)
 {
 hwaddr pa;
@@ -178,6 +190,15 @@ static inline void vring_used_ring_len(VirtQueue *vq, int 
i, uint32_t val)
 virtio_stl_phys(vq->vdev, pa, val);
 }
 
+static inline void
+vring_used_writeback(VirtQueue *vq, int i, VRingUsedElem *ue)
+{
+hwaddr pa;
+pa = vq->vring.used + offsetof(VRingUsed, ring[i]);
+address_space_rw(&address_space_memory, pa, MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)ue, sizeof(VRingUsedElem), true);
+}
+
 static uint16_t vring_used_idx(VirtQueue *vq)
 {
 hwaddr pa;
@@ -190,6 +211,7 @@ static inline void vring_used_idx_set(VirtQueue *vq, 
uint16_t val)
 hwaddr pa;
 pa = vq->vring.used + offsetof(VRingUsed, idx);
 virtio_stw_phys(vq->vdev, pa, val);
+vq->last_used_idx = val;
 }
 
 static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
@@ -277,15 +299,17 @@ void virtqueue_discard(VirtQueue *vq, const 
VirtQueueElement *elem,
 void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
 unsigned int len, unsigned int idx)
 {
+VRingUsedElem ue;
+
 trace_virtqueue_fill(vq, elem, len, idx);
 
 virtqueue_unmap_sg(vq, elem, len);
 
-idx = (idx + vring_used_idx(vq)) % vq->vring.num;
+idx = (idx + vq->last_used_idx) % vq->vring.num;
 
-/* Get a pointer to the next entry in the used ring. */
-vring_used_ring_id(vq, idx, elem->index);
-vring_used_ring_len(vq, idx, len);
+ue.id = elem->index;
+ue.len = len;
+vring_used_writeback(vq, idx, &ue);
 }
 
 void virtqueue_flush(VirtQueue *vq, unsigned int count)
@@ -294,7 +318,7 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
 /* Make sure buffer is written before we update index. */
 smp_wmb();
 trace_virtqueue_flush(vq, count);
-old = vring_used_idx(vq);
+old = vq->last_used_idx;
 new = old + count;
 vring_used_idx_set(vq, new);
 vq->inuse -= count;
@@ -328,6 +352,18 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned int 
idx)
 return num_heads;
 }
 
+static bool virtqueue_empty(VirtQueue *vq)
+{
+if (vq->last_avail_idx == vq->last_fetched_avail_idx) {
+vq->last_fetched_avail_idx = vring_avail_idx(vq);
+/* On success, callers read a descriptor at vq->last_avail_idx.
+ * Make sure descriptor read does not bypass avail index read. */
+smp_rmb();
+}
+
+return vq->last_avail_idx == vq->last_fetched_avail_idx;
+}
+
 static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
 {
 unsigned int head;
@@ -345,18 +381,18 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, 
unsigned int idx)
 return head;
 }
 
-static unsigned virtqueue_next_desc(VirtIODevice *vdev, hwaddr desc_pa,
-unsigned int i, unsigned int max)
+static unsigned virtqueue_next_desc(VirtIODevice *vdev, VRingDesc *desc,
+unsigned int max)
 {
 unsigned int next;
 
 /* If this descriptor says it doesn't chain, we're done. */
-if (!(vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_NEXT)) {
+if (!(desc->flags & VRING_DESC_F_NEXT)) {
 return max;
 }
 
 /* Check they're not leading us off end of descriptors. */
-next = vring_desc_next(vdev, desc_pa, i);
+next = d

[Qemu-devel] [PATCH v3 2/2] net: netmap: use error_setg() helpers in place of error_report()

[Vincenzo Maffione]

This update was required to align error reporting of netmap backend
initialization to the modifications introduced by commit a30ecde.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 4197a9c..5558368 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -90,7 +90,7 @@ pkt_copy(const void *_src, void *_dst, int l)
  * 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)
+static void netmap_open(NetmapPriv *me, Error **errp)
 {
 int fd;
 int err;
@@ -99,9 +99,8 @@ static int netmap_open(NetmapPriv *me)
 
 me->fd = fd = open(me->fdname, O_RDWR);
 if (fd < 0) {
-error_report("Unable to open netmap device '%s' (%s)",
-me->fdname, strerror(errno));
-return -1;
+error_setg_file_open(errp, errno, me->fdname);
+return;
 }
 memset(&req, 0, sizeof(req));
 pstrcpy(req.nr_name, sizeof(req.nr_name), me->ifname);
@@ -109,15 +108,14 @@ static int netmap_open(NetmapPriv *me)
 req.nr_version = NETMAP_API;
 err = ioctl(fd, NIOCREGIF, &req);
 if (err) {
-error_report("Unable to register %s: %s", me->ifname, strerror(errno));
+error_setg_errno(errp, errno, "Unable to register %s", me->ifname);
 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 netmap shared memory: %s",
-strerror(errno));
+error_setg_errno(errp, errno, "Unable to mmap netmap shared memory");
 me->mem = NULL;
 goto error;
 }
@@ -125,11 +123,11 @@ static int netmap_open(NetmapPriv *me)
 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;
+
+return;
 
 error:
 close(me->fd);
-return -1;
 }
 
 static void netmap_send(void *opaque);
@@ -438,9 +436,9 @@ static NetClientInfo net_netmap_info = {
 int net_init_netmap(const NetClientOptions *opts,
 const char *name, NetClientState *peer, Error **errp)
 {
-/* FIXME error_setg(errp, ...) on failure */
 const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
 NetClientState *nc;
+Error *err = NULL;
 NetmapPriv me;
 NetmapState *s;
 
@@ -448,7 +446,9 @@ int net_init_netmap(const NetClientOptions *opts,
 netmap_opts->has_devname ? netmap_opts->devname : "/dev/netmap");
 /* Set default name for the port if not supplied. */
 pstrcpy(me.ifname, sizeof(me.ifname), netmap_opts->ifname);
-if (netmap_open(&me)) {
+netmap_open(&me, &err);
+if (err) {
+error_propagate(errp, err);
 return -1;
 }
 /* Create the object. */
-- 
2.6.2

[Qemu-devel] [PATCH v3 0/2] Fix compilation of netmap backend

[Vincenzo Maffione]

This patch series adds some fixes to the netmap net backend. It contains
two changes:
(1) Fix compilation issue of netmap.c introduced by the reorganization
of struct NetClientOptions
(2) Address the FIXME comment that was asking to use error_setg()
variants in place of error_report()

CHANGELOG:
- removed dead return and use error_setg_file_open() in place
  of error_setg_errno()
- I noticed that net_init_netmap() has to return int, so I restored
  the return statements in that function

Vincenzo Maffione (2):
  net: netmap: Fix compilation issue
  net: netmap: use error_setg() helpers in place of error_report()

 net/netmap.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

-- 
2.6.2

[Qemu-devel] [PATCH v3 1/2] net: netmap: Fix compilation issue

[Vincenzo Maffione]

Reorganization of struct NetClientOptions (commit e4ba22b) caused a
compilation failure of the netmap backend. This patch fixes the issue
by properly accessing the union field.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netmap.c b/net/netmap.c
index 508b829..4197a9c 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -439,7 +439,7 @@ int net_init_netmap(const NetClientOptions *opts,
 const char *name, NetClientState *peer, Error **errp)
 {
 /* FIXME error_setg(errp, ...) on failure */
-const NetdevNetmapOptions *netmap_opts = opts->netmap;
+const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
 NetClientState *nc;
 NetmapPriv me;
 NetmapState *s;
-- 
2.6.2

Re: [Qemu-devel] [PATCH v2 2/2] net: netmap: use error_setg_errno() in place of error_report()

[Vincenzo Maffione]

Ok, preparing them.

2015-11-06 16:35 GMT+01:00 Eric Blake :
> On 11/06/2015 08:13 AM, Vincenzo Maffione wrote:
>> This update was required to align error reporting of netmap backend
>> initialization to the modifications introduced by commit a30ecde.
>>
>> Signed-off-by: Vincenzo Maffione 
>> ---
>>  net/clients.h |  4 ++--
>>  net/netmap.c  | 32 
>>  2 files changed, 18 insertions(+), 18 deletions(-)
>>
>
>> +++ b/net/netmap.c
>
>> @@ -99,9 +99,9 @@ static int netmap_open(NetmapPriv *me)
>>
>>  me->fd = fd = open(me->fdname, O_RDWR);
>>  if (fd < 0) {
>> -error_report("Unable to open netmap device '%s' (%s)",
>> -me->fdname, strerror(errno));
>> -return -1;
>> +error_setg_errno(errp, errno, "Unable to open netmap device '%s'",
>> + me->fdname);
>
> We have error_setg_file_open() for reporting open() failures, if
> consistent messages are desired.
>
>> @@ -125,11 +124,12 @@ static int netmap_open(NetmapPriv *me)
>>  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;
>> +
>> +return;
>>
>>  error:
>>  close(me->fd);
>> -return -1;
>> +return;
>>  }
>
> Dead return, if you wanted to remove it.
>
> Minor enough that I'm okay with it whether or not you make those changes:
> Reviewed-by: Eric Blake 
>
> --
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>



-- 
Vincenzo Maffione

[Qemu-devel] [PATCH v2 2/2] net: netmap: use error_setg_errno() in place of error_report()

[Vincenzo Maffione]

This update was required to align error reporting of netmap backend
initialization to the modifications introduced by commit a30ecde.

Signed-off-by: Vincenzo Maffione 
---
 net/clients.h |  4 ++--
 net/netmap.c  | 32 
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/net/clients.h b/net/clients.h
index d47530e..aefe75e 100644
--- a/net/clients.h
+++ b/net/clients.h
@@ -55,8 +55,8 @@ int net_init_vde(const NetClientOptions *opts, const char 
*name,
 #endif
 
 #ifdef CONFIG_NETMAP
-int net_init_netmap(const NetClientOptions *opts, const char *name,
-NetClientState *peer, Error **errp);
+void net_init_netmap(const NetClientOptions *opts, const char *name,
+ NetClientState *peer, Error **errp);
 #endif
 
 int net_init_vhost_user(const NetClientOptions *opts, const char *name,
diff --git a/net/netmap.c b/net/netmap.c
index 4197a9c..2b0254d 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -90,7 +90,7 @@ pkt_copy(const void *_src, void *_dst, int l)
  * 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)
+static void netmap_open(NetmapPriv *me, Error **errp)
 {
 int fd;
 int err;
@@ -99,9 +99,9 @@ static int netmap_open(NetmapPriv *me)
 
 me->fd = fd = open(me->fdname, O_RDWR);
 if (fd < 0) {
-error_report("Unable to open netmap device '%s' (%s)",
-me->fdname, strerror(errno));
-return -1;
+error_setg_errno(errp, errno, "Unable to open netmap device '%s'",
+ me->fdname);
+return;
 }
 memset(&req, 0, sizeof(req));
 pstrcpy(req.nr_name, sizeof(req.nr_name), me->ifname);
@@ -109,15 +109,14 @@ static int netmap_open(NetmapPriv *me)
 req.nr_version = NETMAP_API;
 err = ioctl(fd, NIOCREGIF, &req);
 if (err) {
-error_report("Unable to register %s: %s", me->ifname, strerror(errno));
+error_setg_errno(errp, errno, "Unable to register %s", me->ifname);
 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 netmap shared memory: %s",
-strerror(errno));
+error_setg_errno(errp, errno, "Unable to mmap netmap shared memory");
 me->mem = NULL;
 goto error;
 }
@@ -125,11 +124,12 @@ static int netmap_open(NetmapPriv *me)
 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;
+
+return;
 
 error:
 close(me->fd);
-return -1;
+return;
 }
 
 static void netmap_send(void *opaque);
@@ -435,12 +435,12 @@ static NetClientInfo net_netmap_info = {
  *
  * ... -net netmap,ifname="..."
  */
-int net_init_netmap(const NetClientOptions *opts,
-const char *name, NetClientState *peer, Error **errp)
+void net_init_netmap(const NetClientOptions *opts,
+ const char *name, NetClientState *peer, Error **errp)
 {
-/* FIXME error_setg(errp, ...) on failure */
 const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
 NetClientState *nc;
+Error *err = NULL;
 NetmapPriv me;
 NetmapState *s;
 
@@ -448,8 +448,10 @@ int net_init_netmap(const NetClientOptions *opts,
 netmap_opts->has_devname ? netmap_opts->devname : "/dev/netmap");
 /* Set default name for the port if not supplied. */
 pstrcpy(me.ifname, sizeof(me.ifname), netmap_opts->ifname);
-if (netmap_open(&me)) {
-return -1;
+netmap_open(&me, &err);
+if (err) {
+error_propagate(errp, err);
+return;
 }
 /* Create the object. */
 nc = qemu_new_net_client(&net_netmap_info, peer, "netmap", name);
@@ -457,7 +459,5 @@ int net_init_netmap(const NetClientOptions *opts,
 s->me = me;
 s->vnet_hdr_len = 0;
 netmap_read_poll(s, true); /* Initially only poll for reads. */
-
-return 0;
 }
 
-- 
2.3.3

[Qemu-devel] [PATCH v2 0/2] Fix compilation of netmap backend

[Vincenzo Maffione]

This patch series adds some fixes to the netmap net backend. It contains
two changes:
(1) Fix compilation issue of netmap.c introduced by the reorganization
of struct NetClientOptions
(2) Address the FIXME comment that was asking to use error_setg()
in place of error_report()

CHANGELOG:
- fixed commit messages to reference appropriate commits
- modify netmap_open() to return error in a local Error
  variable

Vincenzo Maffione (2):
  net: netmap: Fix compilation issue
  net: netmap: use error_setg_errno() in place of error_report()

 net/clients.h |  4 ++--
 net/netmap.c  | 34 +-
 2 files changed, 19 insertions(+), 19 deletions(-)

-- 
2.3.3

[Qemu-devel] [PATCH v2 1/2] net: netmap: Fix compilation issue

[Vincenzo Maffione]

Reorganization of struct NetClientOptions (commit e4ba22b) caused a
compilation failure of the netmap backend. This patch fixes the issue
by properly accessing the union field.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netmap.c b/net/netmap.c
index 508b829..4197a9c 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -439,7 +439,7 @@ int net_init_netmap(const NetClientOptions *opts,
 const char *name, NetClientState *peer, Error **errp)
 {
 /* FIXME error_setg(errp, ...) on failure */
-const NetdevNetmapOptions *netmap_opts = opts->netmap;
+const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
 NetClientState *nc;
 NetmapPriv me;
 NetmapState *s;
-- 
2.3.3

Re: [Qemu-devel] [PATCH 1/2] net: netmap: Fix compilation issue

[Vincenzo Maffione]

2015-11-05 16:01 GMT+01:00 Eric Blake :
> On 11/05/2015 07:52 AM, Vincenzo Maffione wrote:
>> No worries.
>>
>> It needs --enable-netmap (default is --disable-netmap on linux), and
>> --extra-cflags=-I/path/to/netmap/sys for netmap API headers.
>>
>> (netmap code is avaliable here https://github.com/luigirizzo/netmap)
>
> Default-off tends to bit-rot; default-probe is slightly better (compile
> if /path/to/netmap/sys is found during configure, otherwise not).  But
> not the end of the world.

The default-off decision was taken at the time when netmap was merged
in (see the
"[PATCH v3] net: Adding netmap network backend" thread).
The qemu ./configure already has support for probing netmap headers,
so we could actually switch to default-on also on linux (like it
happens for freebsd qemu build).

Thanks for the pointer to the headers. Any
> idea if someone is trying to package netmap development headers for Fedora?

No idea, unfortunately. I personally maintain an AUR package for
Archlinux (https://aur.archlinux.org/packages/netmap/), that's all I
know. Having netmap headers packaged in some distributions is
something we were willing to do, but still nothing has been done,
afaik.


>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH 1/2] net: netmap: Fix compilation issue

[Vincenzo Maffione]

No worries.

It needs --enable-netmap (default is --disable-netmap on linux), and
--extra-cflags=-I/path/to/netmap/sys for netmap API headers.

(netmap code is avaliable here https://github.com/luigirizzo/netmap)

2015-11-05 15:46 GMT+01:00 Eric Blake :
> On 11/05/2015 06:01 AM, Markus Armbruster wrote:
>> Vincenzo Maffione  writes:
>>
>>> Reorganization of struct NetClientOptions caused a compilation failure
>>> of the netmap backend. This patch fixes the issue by properly accessing the
>>> union field.
>>>
>>> Signed-off-by: Vincenzo Maffione 
>>> ---
>>>  net/netmap.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/netmap.c b/net/netmap.c
>>> index 508b829..4197a9c 100644
>>> --- a/net/netmap.c
>>> +++ b/net/netmap.c
>>> @@ -439,7 +439,7 @@ int net_init_netmap(const NetClientOptions *opts,
>>>  const char *name, NetClientState *peer, Error **errp)
>>>  {
>>>  /* FIXME error_setg(errp, ...) on failure */
>>> -const NetdevNetmapOptions *netmap_opts = opts->netmap;
>>> +const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
>>>  NetClientState *nc;
>>>  NetmapPriv me;
>>>  NetmapState *s;
>>
>> Broken in commit e4ba22b.  Recommend to point to that in the commit
>> message.  Thanks for cleaning up the mess we made there!
>
> My fault for not compiling netmap code.  Does it need specific configure
> options?
>
>>
>> Reviewed-by: Markus Armbruster 
>>
>
> Reviewed-by: Eric Blake 
>
> --
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH 1/2] net: netmap: Fix compilation issue

[Vincenzo Maffione]

Sure, no problems. I was looking for that commit, but using git blame
didn't work, since there the data structure seems to be part of
autogenerated code.

2015-11-05 14:01 GMT+01:00 Markus Armbruster :
> Vincenzo Maffione  writes:
>
>> Reorganization of struct NetClientOptions caused a compilation failure
>> of the netmap backend. This patch fixes the issue by properly accessing the
>> union field.
>>
>> Signed-off-by: Vincenzo Maffione 
>> ---
>>  net/netmap.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/netmap.c b/net/netmap.c
>> index 508b829..4197a9c 100644
>> --- a/net/netmap.c
>> +++ b/net/netmap.c
>> @@ -439,7 +439,7 @@ int net_init_netmap(const NetClientOptions *opts,
>>  const char *name, NetClientState *peer, Error **errp)
>>  {
>>  /* FIXME error_setg(errp, ...) on failure */
>> -const NetdevNetmapOptions *netmap_opts = opts->netmap;
>> +const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
>>  NetClientState *nc;
>>  NetmapPriv me;
>>  NetmapState *s;
>
> Broken in commit e4ba22b.  Recommend to point to that in the commit
> message.  Thanks for cleaning up the mess we made there!
>
> Reviewed-by: Markus Armbruster 



-- 
Vincenzo Maffione

[Qemu-devel] [PATCH 2/2] net: netmap: use error_setg_errno() in place of error_report()

[Vincenzo Maffione]

This update was required to align netmap backend to the other
net backends in terms of error reporting.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 4197a9c..45290d8 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -90,7 +90,7 @@ pkt_copy(const void *_src, void *_dst, int l)
  * 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)
+static int netmap_open(NetmapPriv *me, Error **errp)
 {
 int fd;
 int err;
@@ -99,8 +99,8 @@ static int netmap_open(NetmapPriv *me)
 
 me->fd = fd = open(me->fdname, O_RDWR);
 if (fd < 0) {
-error_report("Unable to open netmap device '%s' (%s)",
-me->fdname, strerror(errno));
+error_setg_errno(errp, errno, "Unable to open netmap device '%s'",
+ me->fdname);
 return -1;
 }
 memset(&req, 0, sizeof(req));
@@ -109,15 +109,14 @@ static int netmap_open(NetmapPriv *me)
 req.nr_version = NETMAP_API;
 err = ioctl(fd, NIOCREGIF, &req);
 if (err) {
-error_report("Unable to register %s: %s", me->ifname, strerror(errno));
+error_setg_errno(errp, errno, "Unable to register %s", me->ifname);
 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 netmap shared memory: %s",
-strerror(errno));
+error_setg_errno(errp, errno, "Unable to mmap netmap shared memory");
 me->mem = NULL;
 goto error;
 }
@@ -438,7 +437,6 @@ static NetClientInfo net_netmap_info = {
 int net_init_netmap(const NetClientOptions *opts,
 const char *name, NetClientState *peer, Error **errp)
 {
-/* FIXME error_setg(errp, ...) on failure */
 const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
 NetClientState *nc;
 NetmapPriv me;
@@ -448,7 +446,7 @@ int net_init_netmap(const NetClientOptions *opts,
 netmap_opts->has_devname ? netmap_opts->devname : "/dev/netmap");
 /* Set default name for the port if not supplied. */
 pstrcpy(me.ifname, sizeof(me.ifname), netmap_opts->ifname);
-if (netmap_open(&me)) {
+if (netmap_open(&me, errp)) {
 return -1;
 }
 /* Create the object. */
-- 
2.3.3

[Qemu-devel] [PATCH 1/2] net: netmap: Fix compilation issue

[Vincenzo Maffione]

Reorganization of struct NetClientOptions caused a compilation failure
of the netmap backend. This patch fixes the issue by properly accessing the
union field.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netmap.c b/net/netmap.c
index 508b829..4197a9c 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -439,7 +439,7 @@ int net_init_netmap(const NetClientOptions *opts,
 const char *name, NetClientState *peer, Error **errp)
 {
 /* FIXME error_setg(errp, ...) on failure */
-const NetdevNetmapOptions *netmap_opts = opts->netmap;
+const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
 NetClientState *nc;
 NetmapPriv me;
 NetmapState *s;
-- 
2.3.3

[Qemu-devel] [PATCH 0/2] Fix compilation of netmap backend

[Vincenzo Maffione]

This patch series adds some fixes to the netmap net backend. It contains
two changes:
(1) Fix compilation issue of netmap.c introduced by the reorganization
of struct NetClientOptions
(2) Address the FIXME comment that was asking to use error_setg()
in place of error_report()

Vincenzo Maffione (2):
  net: netmap: Fix compilation issue
  net: netmap: use error_setg_errno() in place of error_report()

 net/netmap.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

-- 
2.3.3

Re: [Qemu-devel] Contribution - L2TPv3 transport

[Vincenzo Maffione]

I think the would be nice if we add a command-line option (or maybe even an
option that can be changed dinamically) for the net backend that
enables/disables the use of the CORK flag, so that the device emulation can
use it anyways, but backend does really use it (and so there are real
effects) only if the user specifies it.

In this way we leave standard QEMU setups unchanged and we enable special
applications (e.g. high packet rate middleboxes) to use the CORK flag to
get higher packet rate, possibly sacrifying a bit of latency.

Cheers,
  Vincenzo


2014-03-05 9:59 GMT+01:00 Stefan Hajnoczi :

> On Tue, Mar 04, 2014 at 09:47:09AM +, Anton Ivanov (antivano) wrote:
> > On 04/03/14 09:36, Stefan Hajnoczi wrote:
> > > On Mon, Mar 03, 2014 at 02:01:00PM +, Anton Ivanov (antivano)
> wrote:
> > >> On 03/03/14 13:27, Stefan Hajnoczi wrote:
> > >>> On Fri, Feb 28, 2014 at 08:28:11AM +, Anton Ivanov (antivano)
> wrote:
> > >>>> 3. Qemu to communicate with the local host, remote vms, network
> devices,
> > >>>> etc at speeds which for a number of use cases exceed the speed of
> the
> > >>>> legacy tap driver.
> > >>> This surprises me.  It's odd that tap performs significantly worse.
> > >>
> > >> Multipacket RX can go a very long way and it does not work on tap's
> > >> emulation of a raw socket. At least in 3.2 :)
> > > Luigi and Vincenzo had ideas on making QEMU's net layer support
> > > multipacket tx using something like TCP_CORK.  This would map to
> > > sendmmsg(2).
> > >
> > > Basically the net client gets multiple .receive() calls but is told to
> > > hold off on submitting the packets.  Then, when it finally gets
> > > uncorked, it can sendmmsg(2).  The only issue is we need to hold on to
> > > the tx buffers longer than normal.
> >
> > Cool, I will be happy to give a hand with that.
> >
> > My main problem so far trying to implement it has been the timers - the
> > qemu internal timer API has no relative timers, only absolute. So you
> > end up with a very high cost of setting and checking a delayed xmit
> timer.
>
> I'm thinking about something simpler that doesn't use a timer:
>
> Rely on the guest to submit a batch of packets for tx.  When processing
> the descriptor ring in the device emulation code (virtio-net, etc), use
> the CORK flag on all packets except the final one.
>
> This essentially hands the contents of the tx ring to the netdev (tap,
> L2TPv3, etc) and then lets them submit the entire batch using
> sendmmsg(2).
>
> When we discussed this previously there was concern about the latency
> added by CORK.
>



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH v2] net: Disable netmap backend when not supported

[Vincenzo Maffione]

Hi,
  I cannot see it in your net tree.

Thanks,
  Vincenzo


2014-02-21 11:05 GMT+01:00 Stefan Hajnoczi :

> On Thu, Feb 20, 2014 at 03:40:43PM +0100, Vincenzo Maffione wrote:
> > This patch fixes configure so that the netmap backend is not compiled in
> if the
> > host doesn't support an API version >= 11. A version upper bound (15)
> has been
> > added so that the netmap API can be extended with some minor features
> without
> > requiring QEMU code modifications.
> >
> > Moreover, some changes have been done to net/netmap.c in order to
> reflect the
> > current netmap API/ABI (11).
> >
> > The NETMAP_WITH_LIBS macro makes possible to include some utilities (e.g.
> > netmap ring macros, D(), RD() and other high level functions) through
> the netmap
> > headers. In this way we get rid of the D and RD macro definitions in the
> QEMU
> > code, and we open the way for further code simplifications that will be
> > introduced by future patches.
> >
> > Signed-off-by: Vincenzo Maffione 
> > ---
> > Note: This patch is against the net-next/net Stefan's branch.
> >
> > Changes against the previous version:
> >(1) more complete commit description
> >(2) add comment in ./configure to explain version checks
> >
> >  configure| 10 +-
> >  net/netmap.c | 55
> +--
> >  2 files changed, 22 insertions(+), 43 deletions(-)
>
> Thanks for adding the explanations, it will make it easier for people
> reading the code to understand this change in the future.
>
> Applied to my net tree:
> https://github.com/stefanha/qemu/commits/net
>
> Stefan
>



-- 
Vincenzo Maffione

[Qemu-devel] [PATCH v2] net: Disable netmap backend when not supported

[Vincenzo Maffione]

This patch fixes configure so that the netmap backend is not compiled in if the
host doesn't support an API version >= 11. A version upper bound (15) has been
added so that the netmap API can be extended with some minor features without
requiring QEMU code modifications.

Moreover, some changes have been done to net/netmap.c in order to reflect the
current netmap API/ABI (11).

The NETMAP_WITH_LIBS macro makes possible to include some utilities (e.g.
netmap ring macros, D(), RD() and other high level functions) through the netmap
headers. In this way we get rid of the D and RD macro definitions in the QEMU
code, and we open the way for further code simplifications that will be
introduced by future patches.

Signed-off-by: Vincenzo Maffione 
---
Note: This patch is against the net-next/net Stefan's branch.

Changes against the previous version:
   (1) more complete commit description
   (2) add comment in ./configure to explain version checks

 configure| 10 +-
 net/netmap.c | 55 +--
 2 files changed, 22 insertions(+), 43 deletions(-)

diff --git a/configure b/configure
index 4648117..e399b3e 100755
--- a/configure
+++ b/configure
@@ -2118,13 +2118,21 @@ EOF
 fi
 
 ##
-# netmap headers probe
+# netmap support probe
+# Apart from looking for netmap headers, we make sure that the host API version
+# supports the netmap backend (>=11). The upper bound (15) is meant to simulate
+# a minor/major version number. Minor new features will be marked with values 
up
+# to 15, and if something happens that requires a change to the backend we will
+# move above 15, submit the backend fixes and modify this two bounds.
 if test "$netmap" != "no" ; then
   cat > $TMPC << EOF
 #include 
 #include 
 #include 
 #include 
+#if (NETMAP_API < 11) || (NETMAP_API > 15)
+#error
+#endif
 int main(void) { return 0; }
 EOF
   if compile_prog "" "" ; then
diff --git a/net/netmap.c b/net/netmap.c
index 73f6d7a..8213304 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -27,6 +27,8 @@
 #include 
 #include 
 #include 
+#include 
+#define NETMAP_WITH_LIBS
 #include 
 #include 
 
@@ -58,31 +60,6 @@ typedef struct NetmapState {
 int vnet_hdr_len;  /* Current virtio-net header length. */
 } NetmapState;
 
-#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, \
-__func__, __LINE__, ##__VA_ARGS__); \
-} while (0)
-
-/* Rate limited version of "D", 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)
-
-
 #ifndef __FreeBSD__
 #define pkt_copy bcopy
 #else
@@ -239,7 +216,7 @@ static ssize_t netmap_receive(NetClientState *nc,
 return size;
 }
 
-if (ring->avail == 0) {
+if (nm_ring_empty(ring)) {
 /* No available slots in the netmap TX ring. */
 netmap_write_poll(s, true);
 return 0;
@@ -252,8 +229,7 @@ static ssize_t netmap_receive(NetClientState *nc,
 ring->slot[i].len = size;
 ring->slot[i].flags = 0;
 pkt_copy(buf, dst, size);
-ring->cur = NETMAP_RING_NEXT(ring, i);
-ring->avail--;
+ring->cur = ring->head = nm_ring_next(ring, i);
 ioctl(s->me.fd, NIOCTXSYNC, NULL);
 
 return size;
@@ -269,7 +245,6 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 uint8_t *dst;
 int j;
 uint32_t i;
-uint32_t avail;
 
 if (unlikely(!ring)) {
 /* Drop the packet. */
@@ -277,9 +252,8 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 }
 
 last = i = ring->cur;
-avail = ring->avail;
 
-if (avail < iovcnt) {
+if (nm_ring_space(ring) < iovcnt) {
 /* Not enough netmap slots. */
 ne

Re: [Qemu-devel] [PATCH 0/3] net: drop implicit peer from offload API

[Vincenzo Maffione]

Hello,
  It looks ok to me.

Cheers
  Vincenzo


2014-02-20 12:14 GMT+01:00 Stefan Hajnoczi :

> This series is based on my net tree, which already has Vincenzo's "Add
> netmap
> backend offloadings support" patch series merged.
>
> After merging the series I realized we were bypassing the net.h API and
> directly accessing nc->info->... in some cases.  This series cleans that
> up, at
> the cost of moving ->peer back up to offload API callers.
>
> I think that's the right thing to do to make net.h APIs consistent (the
> other
> functions don't have implicit ->peer) and avoid bypassing the API.
>
> Stefan Hajnoczi (3):
>   net: remove implicit peer from offload API
>   vhost_net: use offload API instead of bypassing it
>   virtio-net: use qemu_get_queue() where possible
>
>  hw/net/vhost_net.c  |  6 +++---
>  hw/net/virtio-net.c | 12 ++--
>  hw/net/vmxnet3.c| 18 +-
>  include/net/net.h   | 14 +++---
>  net/net.c   | 36 ++------
>  5 files changed, 43 insertions(+), 43 deletions(-)
>
> --
> 1.8.5.3
>
>


-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH] net: Disable netmap backend when not supported

[Vincenzo Maffione]

2014-02-19 16:30 GMT+01:00 Stefan Hajnoczi :

> On Fri, Feb 14, 2014 at 05:40:24PM +0100, Vincenzo Maffione wrote:
> > This patch fixes configure so that netmap is not compiled in if the
> > host doesn't support an API version >= 11.
> >
> > Moreover, some modifications have been done to net/netmap.c in
> > order to reflect the current netmap API (11).
> >
> > Signed-off-by: Vincenzo Maffione 
> > ---
> >  configure|  3 +++
> >  net/netmap.c | 57
> ++---
> >  2 files changed, 17 insertions(+), 43 deletions(-)
> >
> > diff --git a/configure b/configure
> > index 88133a1..61eb932 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2118,6 +2118,9 @@ if test "$netmap" != "no" ; then
> >  #include 
> >  #include 
> >  #include 
> > +#if (NETMAP_API < 11) || (NETMAP_API > 15)
> > +#error
> > +#endif
>
> Why error when NETMAP_API > 15?
>

Well, I believe Luigi can answer this question better than me, so I would
wait for his answer.

>
> > @@ -56,31 +58,6 @@ typedef struct NetmapState {
> >  struct ioveciov[IOV_MAX];
> >  } NetmapState;
> >
> > -#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, \
> > -__func__, __LINE__, ##__VA_ARGS__); \
> > -} while (0)
> > -
> > -/* Rate limited version of "D", 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)
> > -
> > -
> >  #ifndef __FreeBSD__
> >  #define pkt_copy bcopy
> >  #else
>
> Why are you deleting this?
>
Because now equivalent code is contained by "netmap_user.h"

>
> > @@ -237,7 +214,7 @@ static ssize_t netmap_receive(NetClientState *nc,
> >  return size;
> >  }
> >
> > -if (ring->avail == 0) {
> > +if (nm_ring_empty(ring)) {
> >  /* No available slots in the netmap TX ring. */
> >  netmap_write_poll(s, true);
> >  return 0;
> > @@ -250,8 +227,7 @@ static ssize_t netmap_receive(NetClientState *nc,
> >  ring->slot[i].len = size;
> >  ring->slot[i].flags = 0;
> >  pkt_copy(buf, dst, size);
> > -ring->cur = NETMAP_RING_NEXT(ring, i);
> > -ring->avail--;
> > +ring->cur = ring->head = nm_ring_next(ring, i);
> >  ioctl(s->me.fd, NIOCTXSYNC, NULL);
> >
> >  return size;
>
> Are these changes related to the NETMAP_WITH_LIBS macro?  Please do that
> in a separate patch so we keep the version checking change separate from
> the NETMAP_WITH_LIBS change.
>

Yes it is. The macro drives an #ifdef in netmap_user.h, so that we can use
some new functions/macros.
Ok for the splitting. Is it necessary to prepare a patch series or can I
send two different patches?

Thanks,
  Vincenzo



-- 
Vincenzo Maffione

[Qemu-devel] [PATCH] net: Disable netmap backend when not supported

[Vincenzo Maffione]

This patch fixes configure so that netmap is not compiled in if the
host doesn't support an API version >= 11.

Moreover, some modifications have been done to net/netmap.c in
order to reflect the current netmap API (11).

Signed-off-by: Vincenzo Maffione 
---
 configure|  3 +++
 net/netmap.c | 57 ++---
 2 files changed, 17 insertions(+), 43 deletions(-)

diff --git a/configure b/configure
index 88133a1..61eb932 100755
--- a/configure
+++ b/configure
@@ -2118,6 +2118,9 @@ if test "$netmap" != "no" ; then
 #include 
 #include 
 #include 
+#if (NETMAP_API < 11) || (NETMAP_API > 15)
+#error
+#endif
 int main(void) { return 0; }
 EOF
   if compile_prog "" "" ; then
diff --git a/net/netmap.c b/net/netmap.c
index 0ccc497..8a67322 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -27,6 +27,8 @@
 #include 
 #include 
 #include 
+#include 
+#define NETMAP_WITH_LIBS
 #include 
 #include 
 
@@ -56,31 +58,6 @@ typedef struct NetmapState {
 struct ioveciov[IOV_MAX];
 } NetmapState;
 
-#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, \
-__func__, __LINE__, ##__VA_ARGS__); \
-} while (0)
-
-/* Rate limited version of "D", 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)
-
-
 #ifndef __FreeBSD__
 #define pkt_copy bcopy
 #else
@@ -237,7 +214,7 @@ static ssize_t netmap_receive(NetClientState *nc,
 return size;
 }
 
-if (ring->avail == 0) {
+if (nm_ring_empty(ring)) {
 /* No available slots in the netmap TX ring. */
 netmap_write_poll(s, true);
 return 0;
@@ -250,8 +227,7 @@ static ssize_t netmap_receive(NetClientState *nc,
 ring->slot[i].len = size;
 ring->slot[i].flags = 0;
 pkt_copy(buf, dst, size);
-ring->cur = NETMAP_RING_NEXT(ring, i);
-ring->avail--;
+ring->cur = ring->head = nm_ring_next(ring, i);
 ioctl(s->me.fd, NIOCTXSYNC, NULL);
 
 return size;
@@ -267,17 +243,15 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 uint8_t *dst;
 int j;
 uint32_t i;
-uint32_t avail;
 
 if (unlikely(!ring)) {
 /* Drop the packet. */
 return iov_size(iov, iovcnt);
 }
 
-i = ring->cur;
-avail = ring->avail;
+last = i = ring->cur;
 
-if (avail < iovcnt) {
+if (nm_ring_space(ring) < iovcnt) {
 /* Not enough netmap slots. */
 netmap_write_poll(s, true);
 return 0;
@@ -293,7 +267,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 while (iov_frag_size) {
 nm_frag_size = MIN(iov_frag_size, ring->nr_buf_size);
 
-if (unlikely(avail == 0)) {
+if (unlikely(nm_ring_empty(ring))) {
 /* We run out of netmap slots while splitting the
iovec fragments. */
 netmap_write_poll(s, true);
@@ -308,8 +282,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 pkt_copy(iov[j].iov_base + offset, dst, nm_frag_size);
 
 last = i;
-i = NETMAP_RING_NEXT(ring, i);
-avail--;
+i = nm_ring_next(ring, i);
 
 offset += nm_frag_size;
 iov_frag_size -= nm_frag_size;
@@ -318,9 +291,8 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 /* The last slot must not have NS_MOREFRAG set. */
 ring->slot[last].flags &= ~NS_MOREFRAG;
 
-/* Now update ring->cur and ring->avail. */
-ring->cur = i;
-ring->avail = avail;
+/* Now update ring->cur and ring->head. */
+ring->cur = ring->head = i;
 
 ioctl(s->me.fd, NIOCTXSYNC, NULL);
 
@@ -343,7 +315,7 @@ static void netmap_send(void *opaque

Re: [Qemu-devel] net: Adding netmap network backend

[Vincenzo Maffione]

Ok, this patch should preferably go in before the offloadings.


2014-02-14 16:40 GMT+01:00 Stefan Hajnoczi :

> On Fri, Feb 14, 2014 at 02:41:24PM +0100, Vincenzo Maffione wrote:
> >   It is ok for you if I prepare a patch (touching configure and
> > net/netmap.c) that makes sure the host kernel has the minimum netmap
> > version required for QEMU to work (and if not just --disable-netmap)?
>
> Absolutely.
>
> Stefan
>



-- 
Vincenzo Maffione

Re: [Qemu-devel] net: Adding netmap network backend

[Vincenzo Maffione]

Stefan,
  It is ok for you if I prepare a patch (touching configure and
net/netmap.c) that makes sure the host kernel has the minimum netmap
version required for QEMU to work (and if not just --disable-netmap)?

Cheers,
  Vincenzo


2014-02-14 11:24 GMT+01:00 Luigi Rizzo :

>
>
>
> On Fri, Feb 14, 2014 at 2:20 AM, Vincenzo Maffione 
> wrote:
>
>> Yes, for sure we need to do a check.
>>
>> However, this would involve - I think - some non-trivial modifications to
>> net/netmap.c, because without NS_MOREFRAG you cannot split a packet over
>> more "netmap slots/descriptors" (both tx and rx side)
>>
>> Therefore I would ask (manly Luigi, since netmap is in-tree in FreeBSD):
>> Wouldn't it be better to force --disable-netmap when we realize that
>> NETMAP_API (version number for the netmap API) is less than the value
>> required by QEMU? This can be done in ./configure. In this way we keep the
>> QEMU code cleaner.
>>
>
> yes we should do exactly what vincenzo suggests.
>
> cheers
> luigi
>



-- 
Vincenzo Maffione

Re: [Qemu-devel] net: Adding netmap network backend

[Vincenzo Maffione]

Yes, for sure we need to do a check.

However, this would involve - I think - some non-trivial modifications to
net/netmap.c, because without NS_MOREFRAG you cannot split a packet over
more "netmap slots/descriptors" (both tx and rx side)

Therefore I would ask (manly Luigi, since netmap is in-tree in FreeBSD):
Wouldn't it be better to force --disable-netmap when we realize that
NETMAP_API (version number for the netmap API) is less than the value
required by QEMU? This can be done in ./configure. In this way we keep the
QEMU code cleaner.

Thanks,
  Vincenzo


2014-02-14 10:52 GMT+01:00 Stefan Hajnoczi :

> On Wed, Feb 12, 2014 at 10:01:01PM +0400, Michael Tokarev wrote:
> > After this patch, qemu fails to build on freebsd with the following
> > error message:
> >
> > net/netmap.c: In function 'netmap_receive_iov':
> > net/netmap.c:307: error: 'NS_MOREFRAG' undeclared (first use in this
> function)
> > net/netmap.c:307: error: (Each undeclared identifier is reported only
> once
> > net/netmap.c:307: error: for each function it appears in.)
> > net/netmap.c: In function 'netmap_send':
> > net/netmap.c:356: error: 'NS_MOREFRAG' undeclared (first use in this
> function)
> > gmake: *** [net/netmap.o] Error 1
> >
> > I didn't enable netmap support by default, configure enabled it
> > automatically.
> >
> > Freebsd has /usr/include/net/netmap.h and netmap_user.h out of the box.
> > I've no idea what is inside and how compatible it is with whatever
> > qemu expects.
> >
> > I had to explicitly use --disable-netmap for qemu to build on freebsd.
> >
> > This is FreeBSD 9.1-RELEASE, FWIW.
>
> Vincenzo: Do you need to add a ./configure check so NS_MOREFRAG is only
> used when the host kernel supports it?
>



-- 
Vincenzo Maffione

[Qemu-devel] [PATCH v4 2/6] net: extend NetClientInfo for offloading

[Vincenzo Maffione]

Some new callbacks have been added to generalize the operations done
by virtio-net and vmxnet3 frontends to manipulate TAP offloadings.

Signed-off-by: Vincenzo Maffione 
---
 include/net/net.h | 19 +++
 net/net.c | 55 +++
 2 files changed, 74 insertions(+)

diff --git a/include/net/net.h b/include/net/net.h
index 11e1468..7b25394 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -50,6 +50,12 @@ typedef void (NetCleanup) (NetClientState *);
 typedef void (LinkStatusChanged)(NetClientState *);
 typedef void (NetClientDestructor)(NetClientState *);
 typedef RxFilterInfo *(QueryRxFilter)(NetClientState *);
+typedef bool (HasUfo)(NetClientState *);
+typedef bool (HasVnetHdr)(NetClientState *);
+typedef bool (HasVnetHdrLen)(NetClientState *, int);
+typedef void (UsingVnetHdr)(NetClientState *, bool);
+typedef void (SetOffload)(NetClientState *, int, int, int, int, int);
+typedef void (SetVnetHdrLen)(NetClientState *, int);
 
 typedef struct NetClientInfo {
 NetClientOptionsKind type;
@@ -62,6 +68,12 @@ typedef struct NetClientInfo {
 LinkStatusChanged *link_status_changed;
 QueryRxFilter *query_rx_filter;
 NetPoll *poll;
+HasUfo *has_ufo;
+HasVnetHdr *has_vnet_hdr;
+HasVnetHdrLen *has_vnet_hdr_len;
+UsingVnetHdr *using_vnet_hdr;
+SetOffload *set_offload;
+SetVnetHdrLen *set_vnet_hdr_len;
 } NetClientInfo;
 
 struct NetClientState {
@@ -120,6 +132,13 @@ ssize_t qemu_send_packet_async(NetClientState *nc, const 
uint8_t *buf,
 void qemu_purge_queued_packets(NetClientState *nc);
 void qemu_flush_queued_packets(NetClientState *nc);
 void qemu_format_nic_info_str(NetClientState *nc, uint8_t macaddr[6]);
+bool qemu_peer_has_ufo(NetClientState *nc);
+bool qemu_peer_has_vnet_hdr(NetClientState *nc);
+bool qemu_peer_has_vnet_hdr_len(NetClientState *nc, int len);
+void qemu_peer_using_vnet_hdr(NetClientState *nc, bool enable);
+void qemu_peer_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
+   int ecn, int ufo);
+void qemu_peer_set_vnet_hdr_len(NetClientState *nc, int len);
 void qemu_macaddr_default_if_unset(MACAddr *macaddr);
 int qemu_show_nic_models(const char *arg, const char *const *models);
 void qemu_check_nic_model(NICInfo *nd, const char *model);
diff --git a/net/net.c b/net/net.c
index 2c3af20..173673c 100644
--- a/net/net.c
+++ b/net/net.c
@@ -378,6 +378,61 @@ void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
 }
 }
 
+bool qemu_peer_has_ufo(NetClientState *nc)
+{
+if (!nc->peer || !nc->peer->info->has_ufo) {
+return false;
+}
+
+return nc->peer->info->has_ufo(nc->peer);
+}
+
+bool qemu_peer_has_vnet_hdr(NetClientState *nc)
+{
+if (!nc->peer || !nc->peer->info->has_vnet_hdr) {
+return false;
+}
+
+return nc->peer->info->has_vnet_hdr(nc->peer);
+}
+
+bool qemu_peer_has_vnet_hdr_len(NetClientState *nc, int len)
+{
+if (!nc->peer || !nc->peer->info->has_vnet_hdr_len) {
+return false;
+}
+
+return nc->peer->info->has_vnet_hdr_len(nc->peer, len);
+}
+
+void qemu_peer_using_vnet_hdr(NetClientState *nc, bool enable)
+{
+if (!nc->peer || !nc->peer->info->using_vnet_hdr) {
+return;
+}
+
+nc->peer->info->using_vnet_hdr(nc->peer, enable);
+}
+
+void qemu_peer_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
+  int ecn, int ufo)
+{
+if (!nc->peer || !nc->peer->info->set_offload) {
+return;
+}
+
+nc->peer->info->set_offload(nc->peer, csum, tso4, tso6, ecn, ufo);
+}
+
+void qemu_peer_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+if (!nc->peer || !nc->peer->info->set_vnet_hdr_len) {
+return;
+}
+
+nc->peer->info->set_vnet_hdr_len(nc->peer, len);
+}
+
 int qemu_can_send_packet(NetClientState *sender)
 {
 if (!sender->peer) {
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 0/6] Add netmap backend offloadings support

[Vincenzo Maffione]

The purpose of this patch series is to add offloadings support
(TSO/UFO/CSUM) to the netmap network backend, and make it possible
for the paravirtual network frontends (virtio-net and vmxnet3) to
use it.
In order to achieve this, these patches extend the existing
net.h interface to add abstract operations through which a network
frontend can manipulate backend offloading features, instead of
directly calling TAP-specific functions.

Guest-to-guest performance before this patches for virtio-net + netmap:

TCP_STREAM  5.0 Gbps
TCP_RR  12.7 Gbps
UDP_STREAM (64-bytes)   790 Kpps

Guest-to-guest performance after this patches for virtio-net + netmap:

TCP_STREAM  21.4 Gbps
TCP_RR  12.7 Gbps
UDP_STREAM (64-bytes)   790 Kpps

Experiment details:
- Processor: Intel i7-3770K CPU @ 3.50GHz (8 cores)
- Memory @ 1333 MHz
- Host O.S.: Archlinux with Linux 3.11
- Guest O.S.: Archlinux with Linux 3.11

- QEMU command line:
qemu-system-x86_64 archdisk.qcow -snapshot -enable-kvm -device 
virtio-net-pci,ioeventfd=on,mac=00:AA:BB:CC:DD:01,netdev=mynet -netdev 
netmap,ifname=vale0:01,id=mynet -smp 2 -vga std -m 3G


 Changes against the previous version ***
  (1) make tap offloading callbacks static
  (2) tap-win32.c modified to initialize offloading callback pointers
  (3) fix a bug when calling qemu_peer_using_vnet_header

Vincenzo Maffione (6):
  net: change vnet-hdr TAP prototypes
  net: extend NetClientInfo for offloading
  net: TAP uses NetClientInfo offloading callbacks
  net: virtio-net and vmxnet3 use offloading API
  net: make tap offloading callbacks static
  net: add offloading support to netmap backend

 hw/net/vhost_net.c  |  4 +--
 hw/net/virtio-net.c | 16 --
 hw/net/vmxnet3.c| 12 +++
 include/net/net.h   | 19 +++
 include/net/tap.h   |  6 
 net/net.c   | 55 
 net/netmap.c| 68 ++-
 net/tap-win32.c | 92 -
 net/tap.c   | 20 
 9 files changed, 216 insertions(+), 76 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH v4 5/6] net: make tap offloading callbacks static

[Vincenzo Maffione]

Since TAP offloadings are manipulated through a new API, it's
not necessary to export them in include/net/tap.h anymore.

Signed-off-by: Vincenzo Maffione 
---
 include/net/tap.h |  6 --
 net/tap-win32.c   | 12 ++--
 net/tap.c | 12 ++--
 3 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/include/net/tap.h b/include/net/tap.h
index a3490a9..6daeb42 100644
--- a/include/net/tap.h
+++ b/include/net/tap.h
@@ -29,12 +29,6 @@
 #include "qemu-common.h"
 #include "qapi-types.h"
 
-bool tap_has_ufo(NetClientState *nc);
-bool tap_has_vnet_hdr(NetClientState *nc);
-bool tap_has_vnet_hdr_len(NetClientState *nc, int len);
-void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr);
-void tap_set_offload(NetClientState *nc, int csum, int tso4, int tso6, int 
ecn, int ufo);
-void tap_set_vnet_hdr_len(NetClientState *nc, int len);
 int tap_enable(NetClientState *nc);
 int tap_disable(NetClientState *nc);
 
diff --git a/net/tap-win32.c b/net/tap-win32.c
index 924ca55..8aee611 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -669,12 +669,12 @@ static void tap_win32_send(void *opaque)
 }
 }
 
-bool tap_has_ufo(NetClientState *nc)
+static bool tap_has_ufo(NetClientState *nc)
 {
 return false;
 }
 
-bool tap_has_vnet_hdr(NetClientState *nc)
+static bool tap_has_vnet_hdr(NetClientState *nc)
 {
 return false;
 }
@@ -688,11 +688,11 @@ void tap_fd_set_vnet_hdr_len(int fd, int len)
 {
 }
 
-void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr)
+static void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr)
 {
 }
 
-void tap_set_offload(NetClientState *nc, int csum, int tso4,
+static void tap_set_offload(NetClientState *nc, int csum, int tso4,
  int tso6, int ecn, int ufo)
 {
 }
@@ -702,12 +702,12 @@ struct vhost_net *tap_get_vhost_net(NetClientState *nc)
 return NULL;
 }
 
-bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
+static bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
 return false;
 }
 
-void tap_set_vnet_hdr_len(NetClientState *nc, int len)
+static void tap_set_vnet_hdr_len(NetClientState *nc, int len)
 {
 abort();
 }
diff --git a/net/tap.c b/net/tap.c
index d34ec88..2d5099b 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -210,7 +210,7 @@ static void tap_send(void *opaque)
 } while (size > 0 && qemu_can_send_packet(&s->nc));
 }
 
-bool tap_has_ufo(NetClientState *nc)
+static bool tap_has_ufo(NetClientState *nc)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
@@ -219,7 +219,7 @@ bool tap_has_ufo(NetClientState *nc)
 return s->has_ufo;
 }
 
-bool tap_has_vnet_hdr(NetClientState *nc)
+static bool tap_has_vnet_hdr(NetClientState *nc)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
@@ -228,7 +228,7 @@ bool tap_has_vnet_hdr(NetClientState *nc)
 return !!s->host_vnet_hdr_len;
 }
 
-bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
+static bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
@@ -237,7 +237,7 @@ bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
 return !!tap_probe_vnet_hdr_len(s->fd, len);
 }
 
-void tap_set_vnet_hdr_len(NetClientState *nc, int len)
+static void tap_set_vnet_hdr_len(NetClientState *nc, int len)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
@@ -249,7 +249,7 @@ void tap_set_vnet_hdr_len(NetClientState *nc, int len)
 s->host_vnet_hdr_len = len;
 }
 
-void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr)
+static void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
@@ -259,7 +259,7 @@ void tap_using_vnet_hdr(NetClientState *nc, bool 
using_vnet_hdr)
 s->using_vnet_hdr = using_vnet_hdr;
 }
 
-void tap_set_offload(NetClientState *nc, int csum, int tso4,
+static void tap_set_offload(NetClientState *nc, int csum, int tso4,
  int tso6, int ecn, int ufo)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 4/6] net: virtio-net and vmxnet3 use offloading API

[Vincenzo Maffione]

With this patch, virtio-net and vmxnet3 frontends make
use of the qemu_peer_* API for backend offloadings manipulations,
instead of calling TAP-specific functions directly.
We also remove the existing checks which prevent those frontends
from using offloadings with backends different from TAP (e.g. netmap).

Signed-off-by: Vincenzo Maffione 
---
 hw/net/vhost_net.c  |  4 ++--
 hw/net/virtio-net.c | 16 ++--
 hw/net/vmxnet3.c| 12 +---
 3 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 854997d..c90b9ec 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -106,7 +106,7 @@ struct vhost_net *vhost_net_init(NetClientState *backend, 
int devfd,
 goto fail;
 }
 net->nc = backend;
-net->dev.backend_features = tap_has_vnet_hdr(backend) ? 0 :
+net->dev.backend_features = backend->info->has_vnet_hdr(backend) ? 0 :
 (1 << VHOST_NET_F_VIRTIO_NET_HDR);
 net->backend = r;
 
@@ -117,7 +117,7 @@ struct vhost_net *vhost_net_init(NetClientState *backend, 
int devfd,
 if (r < 0) {
 goto fail;
 }
-if (!tap_has_vnet_hdr_len(backend,
+if (!backend->info->has_vnet_hdr_len(backend,
   sizeof(struct virtio_net_hdr_mrg_rxbuf))) {
 net->dev.features &= ~(1 << VIRTIO_NET_F_MRG_RXBUF);
 }
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3626608..cda8c75 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -325,11 +325,7 @@ static void peer_test_vnet_hdr(VirtIONet *n)
 return;
 }
 
-if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
-return;
-}
-
-n->has_vnet_hdr = tap_has_vnet_hdr(nc->peer);
+n->has_vnet_hdr = qemu_peer_has_vnet_hdr(nc);
 }
 
 static int peer_has_vnet_hdr(VirtIONet *n)
@@ -342,7 +338,7 @@ static int peer_has_ufo(VirtIONet *n)
 if (!peer_has_vnet_hdr(n))
 return 0;
 
-n->has_ufo = tap_has_ufo(qemu_get_queue(n->nic)->peer);
+n->has_ufo = qemu_peer_has_ufo(qemu_get_queue(n->nic));
 
 return n->has_ufo;
 }
@@ -361,8 +357,8 @@ static void virtio_net_set_mrg_rx_bufs(VirtIONet *n, int 
mergeable_rx_bufs)
 nc = qemu_get_subqueue(n->nic, i);
 
 if (peer_has_vnet_hdr(n) &&
-tap_has_vnet_hdr_len(nc->peer, n->guest_hdr_len)) {
-tap_set_vnet_hdr_len(nc->peer, n->guest_hdr_len);
+qemu_peer_has_vnet_hdr_len(nc, n->guest_hdr_len)) {
+qemu_peer_set_vnet_hdr_len(nc, n->guest_hdr_len);
 n->host_hdr_len = n->guest_hdr_len;
 }
 }
@@ -463,7 +459,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
 
 static void virtio_net_apply_guest_offloads(VirtIONet *n)
 {
-tap_set_offload(qemu_get_subqueue(n->nic, 0)->peer,
+qemu_peer_set_offload(qemu_get_subqueue(n->nic, 0),
 !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_CSUM)),
 !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO4)),
 !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO6)),
@@ -1544,7 +1540,7 @@ static void virtio_net_device_realize(DeviceState *dev, 
Error **errp)
 peer_test_vnet_hdr(n);
 if (peer_has_vnet_hdr(n)) {
 for (i = 0; i < n->max_queues; i++) {
-tap_using_vnet_hdr(qemu_get_subqueue(n->nic, i)->peer, true);
+qemu_peer_using_vnet_hdr(qemu_get_subqueue(n->nic, i), true);
 }
 n->host_hdr_len = sizeof(struct virtio_net_hdr);
 } else {
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 19687aa..0524684 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1290,7 +1290,7 @@ static void vmxnet3_update_features(VMXNET3State *s)
   s->lro_supported, rxcso_supported,
   s->rx_vlan_stripping);
 if (s->peer_has_vhdr) {
-tap_set_offload(qemu_get_queue(s->nic)->peer,
+qemu_peer_set_offload(qemu_get_queue(s->nic),
 rxcso_supported,
 s->lro_supported,
 s->lro_supported,
@@ -1883,11 +1883,9 @@ static NetClientInfo net_vmxnet3_info = {
 
 static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s)
 {
-NetClientState *peer = qemu_get_queue(s->nic)->peer;
+NetClientState *nc = qemu_get_queue(s->nic);
 
-if ((NULL != peer)  &&
-(peer->info->type == NET_CLIENT_OPTIONS_KIND_TAP)   &&
-tap_has_vnet_hdr(peer)) {
+if (qemu_peer_has_vnet_hdr(nc)) {
 return true;
 }
 
@@ -1935,10 +1933,10 @@ static void vmxnet3_net_init(VMXNET3State *s)
 s->lro_supported = false;
 
 if (s->peer_has_vhdr) {
-tap_set_vnet_hdr_len(qemu_get_queue

[Qemu-devel] [PATCH v4 3/6] net: TAP uses NetClientInfo offloading callbacks

[Vincenzo Maffione]

The TAP NetClientInfo structure is inizialized with the TAP-specific
functions that manipulates offloading features.

Signed-off-by: Vincenzo Maffione 
---
 net/tap-win32.c | 92 ++---
 net/tap.c   |  6 
 2 files changed, 55 insertions(+), 43 deletions(-)

diff --git a/net/tap-win32.c b/net/tap-win32.c
index edf26c4..924ca55 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -669,11 +669,60 @@ static void tap_win32_send(void *opaque)
 }
 }
 
+bool tap_has_ufo(NetClientState *nc)
+{
+return false;
+}
+
+bool tap_has_vnet_hdr(NetClientState *nc)
+{
+return false;
+}
+
+int tap_probe_vnet_hdr_len(int fd, int len)
+{
+return 0;
+}
+
+void tap_fd_set_vnet_hdr_len(int fd, int len)
+{
+}
+
+void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr)
+{
+}
+
+void tap_set_offload(NetClientState *nc, int csum, int tso4,
+ int tso6, int ecn, int ufo)
+{
+}
+
+struct vhost_net *tap_get_vhost_net(NetClientState *nc)
+{
+return NULL;
+}
+
+bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
+{
+return false;
+}
+
+void tap_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+abort();
+}
+
 static NetClientInfo net_tap_win32_info = {
 .type = NET_CLIENT_OPTIONS_KIND_TAP,
 .size = sizeof(TAPState),
 .receive = tap_receive,
 .cleanup = tap_cleanup,
+.has_ufo = tap_has_ufo,
+.has_vnet_hdr = tap_has_vnet_hdr,
+.has_vnet_hdr_len = tap_has_vnet_hdr_len,
+.using_vnet_hdr = tap_using_vnet_hdr,
+.set_offload = tap_set_offload,
+.set_vnet_hdr_len = tap_set_vnet_hdr_len,
 };
 
 static int tap_win32_init(NetClientState *peer, const char *model,
@@ -722,49 +771,6 @@ int net_init_tap(const NetClientOptions *opts, const char 
*name,
 return 0;
 }
 
-bool tap_has_ufo(NetClientState *nc)
-{
-return false;
-}
-
-bool tap_has_vnet_hdr(NetClientState *nc)
-{
-return false;
-}
-
-int tap_probe_vnet_hdr_len(int fd, int len)
-{
-return 0;
-}
-
-void tap_fd_set_vnet_hdr_len(int fd, int len)
-{
-}
-
-void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr)
-{
-}
-
-void tap_set_offload(NetClientState *nc, int csum, int tso4,
- int tso6, int ecn, int ufo)
-{
-}
-
-struct vhost_net *tap_get_vhost_net(NetClientState *nc)
-{
-return NULL;
-}
-
-bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
-{
-return false;
-}
-
-void tap_set_vnet_hdr_len(NetClientState *nc, int len)
-{
-abort();
-}
-
 int tap_enable(NetClientState *nc)
 {
 abort();
diff --git a/net/tap.c b/net/tap.c
index c805f3c..d34ec88 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -314,6 +314,12 @@ static NetClientInfo net_tap_info = {
 .receive_iov = tap_receive_iov,
 .poll = tap_poll,
 .cleanup = tap_cleanup,
+.has_ufo = tap_has_ufo,
+.has_vnet_hdr = tap_has_vnet_hdr,
+.has_vnet_hdr_len = tap_has_vnet_hdr_len,
+.using_vnet_hdr = tap_using_vnet_hdr,
+.set_offload = tap_set_offload,
+.set_vnet_hdr_len = tap_set_vnet_hdr_len,
 };
 
 static TAPState *net_tap_fd_init(NetClientState *peer,
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 6/6] net: add offloading support to netmap backend

[Vincenzo Maffione]

Whit this patch, the netmap backend supports TSO/UFO/CSUM
offloadings, and accepts the virtio-net header, similarly to what
happens with TAP. The offloading callbacks in the NetClientInfo
interface have been implemented.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 68 +++-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/net/netmap.c b/net/netmap.c
index 0ccc497..73f6d7a 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -31,6 +31,7 @@
 #include 
 
 #include "net/net.h"
+#include "net/tap.h"
 #include "clients.h"
 #include "sysemu/sysemu.h"
 #include "qemu/error-report.h"
@@ -54,6 +55,7 @@ typedef struct NetmapState {
 boolread_poll;
 boolwrite_poll;
 struct ioveciov[IOV_MAX];
+int vnet_hdr_len;  /* Current virtio-net header length. */
 } NetmapState;
 
 #define D(format, ...)  \
@@ -274,7 +276,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 return iov_size(iov, iovcnt);
 }
 
-i = ring->cur;
+last = i = ring->cur;
 avail = ring->avail;
 
 if (avail < iovcnt) {
@@ -394,6 +396,63 @@ static void netmap_cleanup(NetClientState *nc)
 s->me.fd = -1;
 }
 
+/* Offloading manipulation support callbacks. */
+static bool netmap_has_ufo(NetClientState *nc)
+{
+return true;
+}
+
+static bool netmap_has_vnet_hdr(NetClientState *nc)
+{
+return true;
+}
+
+static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
+{
+return len == 0 || len == sizeof(struct virtio_net_hdr) ||
+len == sizeof(struct virtio_net_hdr_mrg_rxbuf);
+}
+
+static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
+{
+}
+
+static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+int err;
+struct nmreq req;
+
+/* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
+ * length for the netmap adapter associated to 'me->ifname'.
+ */
+memset(&req, 0, sizeof(req));
+pstrcpy(req.nr_name, sizeof(req.nr_name), s->me.ifname);
+req.nr_version = NETMAP_API;
+req.nr_cmd = NETMAP_BDG_VNET_HDR;
+req.nr_arg1 = len;
+err = ioctl(s->me.fd, NIOCREGIF, &req);
+if (err) {
+error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
+ s->me.ifname, strerror(errno));
+} else {
+/* Keep track of the current length. */
+s->vnet_hdr_len = len;
+}
+}
+
+static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int 
tso6,
+   int ecn, int ufo)
+{
+NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+
+/* Setting a virtio-net header length greater than zero automatically
+ * enables the offloadings.
+ */
+if (!s->vnet_hdr_len) {
+netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
+}
+}
 
 /* NetClientInfo methods */
 static NetClientInfo net_netmap_info = {
@@ -403,6 +462,12 @@ static NetClientInfo net_netmap_info = {
 .receive_iov = netmap_receive_iov,
 .poll = netmap_poll,
 .cleanup = netmap_cleanup,
+.has_ufo = netmap_has_ufo,
+.has_vnet_hdr = netmap_has_vnet_hdr,
+.has_vnet_hdr_len = netmap_has_vnet_hdr_len,
+.using_vnet_hdr = netmap_using_vnet_hdr,
+.set_offload = netmap_set_offload,
+.set_vnet_hdr_len = netmap_set_vnet_hdr_len,
 };
 
 /* The exported init function
@@ -428,6 +493,7 @@ int net_init_netmap(const NetClientOptions *opts,
 nc = qemu_new_net_client(&net_netmap_info, peer, "netmap", name);
 s = DO_UPCAST(NetmapState, nc, nc);
 s->me = me;
+s->vnet_hdr_len = 0;
 netmap_read_poll(s, true); /* Initially only poll for reads. */
 
 return 0;
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 1/6] net: change vnet-hdr TAP prototypes

[Vincenzo Maffione]

The tap_has_vnet_hdr() and tap_has_vnet_hdr_len() functions used
to return int, even though they only return true/false values.
This patch changes the prototypes to return bool.

Signed-off-by: Vincenzo Maffione 
---
 include/net/tap.h | 4 ++--
 net/tap-win32.c   | 8 
 net/tap.c | 6 +++---
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/net/tap.h b/include/net/tap.h
index a994f20..a3490a9 100644
--- a/include/net/tap.h
+++ b/include/net/tap.h
@@ -30,8 +30,8 @@
 #include "qapi-types.h"
 
 bool tap_has_ufo(NetClientState *nc);
-int tap_has_vnet_hdr(NetClientState *nc);
-int tap_has_vnet_hdr_len(NetClientState *nc, int len);
+bool tap_has_vnet_hdr(NetClientState *nc);
+bool tap_has_vnet_hdr_len(NetClientState *nc, int len);
 void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr);
 void tap_set_offload(NetClientState *nc, int csum, int tso4, int tso6, int 
ecn, int ufo);
 void tap_set_vnet_hdr_len(NetClientState *nc, int len);
diff --git a/net/tap-win32.c b/net/tap-win32.c
index 91e9e84..edf26c4 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -727,9 +727,9 @@ bool tap_has_ufo(NetClientState *nc)
 return false;
 }
 
-int tap_has_vnet_hdr(NetClientState *nc)
+bool tap_has_vnet_hdr(NetClientState *nc)
 {
-return 0;
+return false;
 }
 
 int tap_probe_vnet_hdr_len(int fd, int len)
@@ -755,9 +755,9 @@ struct vhost_net *tap_get_vhost_net(NetClientState *nc)
 return NULL;
 }
 
-int tap_has_vnet_hdr_len(NetClientState *nc, int len)
+bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
-return 0;
+return false;
 }
 
 void tap_set_vnet_hdr_len(NetClientState *nc, int len)
diff --git a/net/tap.c b/net/tap.c
index 39c1cda..c805f3c 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -219,7 +219,7 @@ bool tap_has_ufo(NetClientState *nc)
 return s->has_ufo;
 }
 
-int tap_has_vnet_hdr(NetClientState *nc)
+bool tap_has_vnet_hdr(NetClientState *nc)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
@@ -228,13 +228,13 @@ int tap_has_vnet_hdr(NetClientState *nc)
 return !!s->host_vnet_hdr_len;
 }
 
-int tap_has_vnet_hdr_len(NetClientState *nc, int len)
+bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
 assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_TAP);
 
-return tap_probe_vnet_hdr_len(s->fd, len);
+return !!tap_probe_vnet_hdr_len(s->fd, len);
 }
 
 void tap_set_vnet_hdr_len(NetClientState *nc, int len)
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v3 3/5] net: TAP uses NetClientInfo offloading callbacks

[Vincenzo Maffione]

Ok, It could go as patch 5/6.

Thanks


2014-02-06 Stefan Hajnoczi :

> On Mon, Jan 20, 2014 at 06:07:03PM +0100, Vincenzo Maffione wrote:
> > The TAP NetClientInfo structure is inizialized with the TAP-specific
> > functions that manipulates offloading features.
> >
> > Signed-off-by: Vincenzo Maffione 
> > ---
> >  net/tap.c | 6 ++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/net/tap.c b/net/tap.c
> > index c805f3c..d34ec88 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -314,6 +314,12 @@ static NetClientInfo net_tap_info = {
> >  .receive_iov = tap_receive_iov,
> >  .poll = tap_poll,
> >  .cleanup = tap_cleanup,
> > +.has_ufo = tap_has_ufo,
> > +.has_vnet_hdr = tap_has_vnet_hdr,
> > +.has_vnet_hdr_len = tap_has_vnet_hdr_len,
> > +.using_vnet_hdr = tap_using_vnet_hdr,
> > +.set_offload = tap_set_offload,
> > +.set_vnet_hdr_len = tap_set_vnet_hdr_len,
> >  };
>
> Regarding making functions static: that has to be done after the next
> patch since vhost-net is still calling tap_*() directly here.  It can be
> a new patch later in this series.
>



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH v3 0/5] Add netmap backend offloadings support

[Vincenzo Maffione]

ping

Cheers
  Vincenzo


2014-01-20 Vincenzo Maffione :

> The purpose of this patch series is to add offloadings support
> (TSO/UFO/CSUM) to the netmap network backend, and make it possible
> for the paravirtual network frontends (virtio-net and vmxnet3) to
> use it.
> In order to achieve this, these patches extend the existing
> net.h interface to add abstract operations through which a network
> frontend can manipulate backend offloading features, instead of
> directly calling TAP-specific functions.
>
> Guest-to-guest performance before this patches for virtio-net + netmap:
>
> TCP_STREAM  5.0 Gbps
> TCP_RR  12.7 Gbps
> UDP_STREAM (64-bytes)   790 Kpps
>
> Guest-to-guest performance after this patches for virtio-net + netmap:
>
> TCP_STREAM  21.4 Gbps
> TCP_RR  12.7 Gbps
> UDP_STREAM (64-bytes)   790 Kpps
>
> Experiment details:
> - Processor: Intel i7-3770K CPU @ 3.50GHz (8 cores)
> - Memory @ 1333 MHz
> - Host O.S.: Archlinux with Linux 3.11
> - Guest O.S.: Archlinux with Linux 3.11
>
> - QEMU command line:
> qemu-system-x86_64 archdisk.qcow -snapshot -enable-kvm -device
> virtio-net-pci,ioeventfd=on,mac=00:AA:BB:CC:DD:01,netdev=mynet -netdev
> netmap,ifname=vale0:01,id=mynet -smp 2 -vga std -m 3G
>
>
>  Changes against the previous version ***
> NOTE: Now the VALE switch supports the offloadings, meaning that TCP/UDP
> will work
>   even between a virtio-net guest with offloadings enabled and another
>   guest with offloadings disabled. This has been achieved without
> performance loss
>   on the "fast" case (e.g. two virtio-net-guest communicating with
> offloadings on).
>
> (1) removed the second commit of the previous version, which was
> intended to remove tap_using_vnet_hdr()
> (2) the last commit has been revised: NETMAP_BDG_OFFSET becomes
> NETMAP_BDG_VNET_HDR, netmap_set_offload() implemented and
> netmap_has_vnet_hdr_len() rewritten.
>
>
> Vincenzo Maffione (5):
>   net: change vnet-hdr TAP prototypes
>   net: extend NetClientInfo for offloading
>   net: TAP uses NetClientInfo offloading callbacks
>   net: virtio-net and vmxnet3 use offloading API
>   net: add offloading support to netmap backend
>
>  hw/net/virtio-net.c | 16 +
>  hw/net/vmxnet3.c| 12 --
>  include/net/net.h   | 19 +++
>  include/net/tap.h   |  4 ++--
>  net/net.c   | 55 +++
>  net/netmap.c| 68
> ++++-
>  net/tap-win32.c |  8 +++
>  net/tap.c   | 12 +++---
>  8 files changed, 167 insertions(+), 27 deletions(-)
>
> --
> 1.8.5.2
>
>


-- 
Vincenzo Maffione

[Qemu-devel] [PATCH v3 5/5] net: add offloading support to netmap backend

[Vincenzo Maffione]

Whit this patch, the netmap backend supports TSO/UFO/CSUM
offloadings, and accepts the virtio-net header, similarly to what
happens with TAP. The offloading callbacks in the NetClientInfo
interface have been implemented.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 68 +++-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/net/netmap.c b/net/netmap.c
index 0ccc497..73f6d7a 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -31,6 +31,7 @@
 #include 
 
 #include "net/net.h"
+#include "net/tap.h"
 #include "clients.h"
 #include "sysemu/sysemu.h"
 #include "qemu/error-report.h"
@@ -54,6 +55,7 @@ typedef struct NetmapState {
 boolread_poll;
 boolwrite_poll;
 struct ioveciov[IOV_MAX];
+int vnet_hdr_len;  /* Current virtio-net header length. */
 } NetmapState;
 
 #define D(format, ...)  \
@@ -274,7 +276,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 return iov_size(iov, iovcnt);
 }
 
-i = ring->cur;
+last = i = ring->cur;
 avail = ring->avail;
 
 if (avail < iovcnt) {
@@ -394,6 +396,63 @@ static void netmap_cleanup(NetClientState *nc)
 s->me.fd = -1;
 }
 
+/* Offloading manipulation support callbacks. */
+static bool netmap_has_ufo(NetClientState *nc)
+{
+return true;
+}
+
+static bool netmap_has_vnet_hdr(NetClientState *nc)
+{
+return true;
+}
+
+static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
+{
+return len == 0 || len == sizeof(struct virtio_net_hdr) ||
+len == sizeof(struct virtio_net_hdr_mrg_rxbuf);
+}
+
+static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
+{
+}
+
+static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+int err;
+struct nmreq req;
+
+/* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
+ * length for the netmap adapter associated to 'me->ifname'.
+ */
+memset(&req, 0, sizeof(req));
+pstrcpy(req.nr_name, sizeof(req.nr_name), s->me.ifname);
+req.nr_version = NETMAP_API;
+req.nr_cmd = NETMAP_BDG_VNET_HDR;
+req.nr_arg1 = len;
+err = ioctl(s->me.fd, NIOCREGIF, &req);
+if (err) {
+error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
+ s->me.ifname, strerror(errno));
+} else {
+/* Keep track of the current length. */
+s->vnet_hdr_len = len;
+}
+}
+
+static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int 
tso6,
+   int ecn, int ufo)
+{
+NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+
+/* Setting a virtio-net header length greater than zero automatically
+ * enables the offloadings.
+ */
+if (!s->vnet_hdr_len) {
+netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
+}
+}
 
 /* NetClientInfo methods */
 static NetClientInfo net_netmap_info = {
@@ -403,6 +462,12 @@ static NetClientInfo net_netmap_info = {
 .receive_iov = netmap_receive_iov,
 .poll = netmap_poll,
 .cleanup = netmap_cleanup,
+.has_ufo = netmap_has_ufo,
+.has_vnet_hdr = netmap_has_vnet_hdr,
+.has_vnet_hdr_len = netmap_has_vnet_hdr_len,
+.using_vnet_hdr = netmap_using_vnet_hdr,
+.set_offload = netmap_set_offload,
+.set_vnet_hdr_len = netmap_set_vnet_hdr_len,
 };
 
 /* The exported init function
@@ -428,6 +493,7 @@ int net_init_netmap(const NetClientOptions *opts,
 nc = qemu_new_net_client(&net_netmap_info, peer, "netmap", name);
 s = DO_UPCAST(NetmapState, nc, nc);
 s->me = me;
+s->vnet_hdr_len = 0;
 netmap_read_poll(s, true); /* Initially only poll for reads. */
 
 return 0;
-- 
1.8.5.2

[Qemu-devel] [PATCH v3 3/5] net: TAP uses NetClientInfo offloading callbacks

[Vincenzo Maffione]

The TAP NetClientInfo structure is inizialized with the TAP-specific
functions that manipulates offloading features.

Signed-off-by: Vincenzo Maffione 
---
 net/tap.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/net/tap.c b/net/tap.c
index c805f3c..d34ec88 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -314,6 +314,12 @@ static NetClientInfo net_tap_info = {
 .receive_iov = tap_receive_iov,
 .poll = tap_poll,
 .cleanup = tap_cleanup,
+.has_ufo = tap_has_ufo,
+.has_vnet_hdr = tap_has_vnet_hdr,
+.has_vnet_hdr_len = tap_has_vnet_hdr_len,
+.using_vnet_hdr = tap_using_vnet_hdr,
+.set_offload = tap_set_offload,
+.set_vnet_hdr_len = tap_set_vnet_hdr_len,
 };
 
 static TAPState *net_tap_fd_init(NetClientState *peer,
-- 
1.8.5.2

[Qemu-devel] [PATCH v3 2/5] net: extend NetClientInfo for offloading

[Vincenzo Maffione]

Some new callbacks have been added to generalize the operations done
by virtio-net and vmxnet3 frontends to manipulate TAP offloadings.

Signed-off-by: Vincenzo Maffione 
---
 include/net/net.h | 19 +++
 net/net.c | 55 +++
 2 files changed, 74 insertions(+)

diff --git a/include/net/net.h b/include/net/net.h
index 11e1468..7b25394 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -50,6 +50,12 @@ typedef void (NetCleanup) (NetClientState *);
 typedef void (LinkStatusChanged)(NetClientState *);
 typedef void (NetClientDestructor)(NetClientState *);
 typedef RxFilterInfo *(QueryRxFilter)(NetClientState *);
+typedef bool (HasUfo)(NetClientState *);
+typedef bool (HasVnetHdr)(NetClientState *);
+typedef bool (HasVnetHdrLen)(NetClientState *, int);
+typedef void (UsingVnetHdr)(NetClientState *, bool);
+typedef void (SetOffload)(NetClientState *, int, int, int, int, int);
+typedef void (SetVnetHdrLen)(NetClientState *, int);
 
 typedef struct NetClientInfo {
 NetClientOptionsKind type;
@@ -62,6 +68,12 @@ typedef struct NetClientInfo {
 LinkStatusChanged *link_status_changed;
 QueryRxFilter *query_rx_filter;
 NetPoll *poll;
+HasUfo *has_ufo;
+HasVnetHdr *has_vnet_hdr;
+HasVnetHdrLen *has_vnet_hdr_len;
+UsingVnetHdr *using_vnet_hdr;
+SetOffload *set_offload;
+SetVnetHdrLen *set_vnet_hdr_len;
 } NetClientInfo;
 
 struct NetClientState {
@@ -120,6 +132,13 @@ ssize_t qemu_send_packet_async(NetClientState *nc, const 
uint8_t *buf,
 void qemu_purge_queued_packets(NetClientState *nc);
 void qemu_flush_queued_packets(NetClientState *nc);
 void qemu_format_nic_info_str(NetClientState *nc, uint8_t macaddr[6]);
+bool qemu_peer_has_ufo(NetClientState *nc);
+bool qemu_peer_has_vnet_hdr(NetClientState *nc);
+bool qemu_peer_has_vnet_hdr_len(NetClientState *nc, int len);
+void qemu_peer_using_vnet_hdr(NetClientState *nc, bool enable);
+void qemu_peer_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
+   int ecn, int ufo);
+void qemu_peer_set_vnet_hdr_len(NetClientState *nc, int len);
 void qemu_macaddr_default_if_unset(MACAddr *macaddr);
 int qemu_show_nic_models(const char *arg, const char *const *models);
 void qemu_check_nic_model(NICInfo *nd, const char *model);
diff --git a/net/net.c b/net/net.c
index f8db85f..0a31c95 100644
--- a/net/net.c
+++ b/net/net.c
@@ -381,6 +381,61 @@ void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
 }
 }
 
+bool qemu_peer_has_ufo(NetClientState *nc)
+{
+if (!nc->peer || !nc->peer->info->has_ufo) {
+return false;
+}
+
+return nc->peer->info->has_ufo(nc->peer);
+}
+
+bool qemu_peer_has_vnet_hdr(NetClientState *nc)
+{
+if (!nc->peer || !nc->peer->info->has_vnet_hdr) {
+return false;
+}
+
+return nc->peer->info->has_vnet_hdr(nc->peer);
+}
+
+bool qemu_peer_has_vnet_hdr_len(NetClientState *nc, int len)
+{
+if (!nc->peer || !nc->peer->info->has_vnet_hdr_len) {
+return false;
+}
+
+return nc->peer->info->has_vnet_hdr_len(nc->peer, len);
+}
+
+void qemu_peer_using_vnet_hdr(NetClientState *nc, bool enable)
+{
+if (!nc->peer || !nc->peer->info->using_vnet_hdr) {
+return;
+}
+
+nc->peer->info->using_vnet_hdr(nc->peer, enable);
+}
+
+void qemu_peer_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
+  int ecn, int ufo)
+{
+if (!nc->peer || !nc->peer->info->set_offload) {
+return;
+}
+
+nc->peer->info->set_offload(nc->peer, csum, tso4, tso6, ecn, ufo);
+}
+
+void qemu_peer_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+if (!nc->peer || !nc->peer->info->set_vnet_hdr_len) {
+return;
+}
+
+nc->peer->info->set_vnet_hdr_len(nc->peer, len);
+}
+
 int qemu_can_send_packet(NetClientState *sender)
 {
 if (!sender->peer) {
-- 
1.8.5.2

[Qemu-devel] [PATCH v3 1/5] net: change vnet-hdr TAP prototypes

[Vincenzo Maffione]

The tap_has_vnet_hdr() and tap_has_vnet_hdr_len() functions used
to return int, even though they only return true/false values.
This patch changes the prototypes to return bool.

Signed-off-by: Vincenzo Maffione 
---
 include/net/tap.h | 4 ++--
 net/tap-win32.c   | 8 
 net/tap.c | 6 +++---
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/net/tap.h b/include/net/tap.h
index a994f20..a3490a9 100644
--- a/include/net/tap.h
+++ b/include/net/tap.h
@@ -30,8 +30,8 @@
 #include "qapi-types.h"
 
 bool tap_has_ufo(NetClientState *nc);
-int tap_has_vnet_hdr(NetClientState *nc);
-int tap_has_vnet_hdr_len(NetClientState *nc, int len);
+bool tap_has_vnet_hdr(NetClientState *nc);
+bool tap_has_vnet_hdr_len(NetClientState *nc, int len);
 void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr);
 void tap_set_offload(NetClientState *nc, int csum, int tso4, int tso6, int 
ecn, int ufo);
 void tap_set_vnet_hdr_len(NetClientState *nc, int len);
diff --git a/net/tap-win32.c b/net/tap-win32.c
index 91e9e84..edf26c4 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -727,9 +727,9 @@ bool tap_has_ufo(NetClientState *nc)
 return false;
 }
 
-int tap_has_vnet_hdr(NetClientState *nc)
+bool tap_has_vnet_hdr(NetClientState *nc)
 {
-return 0;
+return false;
 }
 
 int tap_probe_vnet_hdr_len(int fd, int len)
@@ -755,9 +755,9 @@ struct vhost_net *tap_get_vhost_net(NetClientState *nc)
 return NULL;
 }
 
-int tap_has_vnet_hdr_len(NetClientState *nc, int len)
+bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
-return 0;
+return false;
 }
 
 void tap_set_vnet_hdr_len(NetClientState *nc, int len)
diff --git a/net/tap.c b/net/tap.c
index 39c1cda..c805f3c 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -219,7 +219,7 @@ bool tap_has_ufo(NetClientState *nc)
 return s->has_ufo;
 }
 
-int tap_has_vnet_hdr(NetClientState *nc)
+bool tap_has_vnet_hdr(NetClientState *nc)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
@@ -228,13 +228,13 @@ int tap_has_vnet_hdr(NetClientState *nc)
 return !!s->host_vnet_hdr_len;
 }
 
-int tap_has_vnet_hdr_len(NetClientState *nc, int len)
+bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
 assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_TAP);
 
-return tap_probe_vnet_hdr_len(s->fd, len);
+return !!tap_probe_vnet_hdr_len(s->fd, len);
 }
 
 void tap_set_vnet_hdr_len(NetClientState *nc, int len)
-- 
1.8.5.2

[Qemu-devel] [PATCH v3 4/5] net: virtio-net and vmxnet3 use offloading API

[Vincenzo Maffione]

With this patch, virtio-net and vmxnet3 frontends make
use of the qemu_peer_* API for backend offloadings manipulations,
instead of calling TAP-specific functions directly.
We also remove the existing checks which prevent those frontends
from using offloadings with backends different from TAP (e.g. netmap).

Signed-off-by: Vincenzo Maffione 
---
 hw/net/virtio-net.c | 16 ++--
 hw/net/vmxnet3.c| 12 +---
 2 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3626608..00c149b 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -325,11 +325,7 @@ static void peer_test_vnet_hdr(VirtIONet *n)
 return;
 }
 
-if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
-return;
-}
-
-n->has_vnet_hdr = tap_has_vnet_hdr(nc->peer);
+n->has_vnet_hdr = qemu_peer_has_vnet_hdr(nc);
 }
 
 static int peer_has_vnet_hdr(VirtIONet *n)
@@ -342,7 +338,7 @@ static int peer_has_ufo(VirtIONet *n)
 if (!peer_has_vnet_hdr(n))
 return 0;
 
-n->has_ufo = tap_has_ufo(qemu_get_queue(n->nic)->peer);
+n->has_ufo = qemu_peer_has_ufo(qemu_get_queue(n->nic));
 
 return n->has_ufo;
 }
@@ -361,8 +357,8 @@ static void virtio_net_set_mrg_rx_bufs(VirtIONet *n, int 
mergeable_rx_bufs)
 nc = qemu_get_subqueue(n->nic, i);
 
 if (peer_has_vnet_hdr(n) &&
-tap_has_vnet_hdr_len(nc->peer, n->guest_hdr_len)) {
-tap_set_vnet_hdr_len(nc->peer, n->guest_hdr_len);
+qemu_peer_has_vnet_hdr_len(nc, n->guest_hdr_len)) {
+qemu_peer_set_vnet_hdr_len(nc, n->guest_hdr_len);
 n->host_hdr_len = n->guest_hdr_len;
 }
 }
@@ -463,7 +459,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
 
 static void virtio_net_apply_guest_offloads(VirtIONet *n)
 {
-tap_set_offload(qemu_get_subqueue(n->nic, 0)->peer,
+qemu_peer_set_offload(qemu_get_subqueue(n->nic, 0),
 !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_CSUM)),
 !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO4)),
 !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO6)),
@@ -1544,7 +1540,7 @@ static void virtio_net_device_realize(DeviceState *dev, 
Error **errp)
 peer_test_vnet_hdr(n);
 if (peer_has_vnet_hdr(n)) {
 for (i = 0; i < n->max_queues; i++) {
-tap_using_vnet_hdr(qemu_get_subqueue(n->nic, i)->peer, true);
+qemu_peer_using_vnet_hdr(qemu_get_subqueue(n->nic, i)->peer, true);
 }
 n->host_hdr_len = sizeof(struct virtio_net_hdr);
 } else {
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 19687aa..4ad3bf0 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1290,7 +1290,7 @@ static void vmxnet3_update_features(VMXNET3State *s)
   s->lro_supported, rxcso_supported,
   s->rx_vlan_stripping);
 if (s->peer_has_vhdr) {
-tap_set_offload(qemu_get_queue(s->nic)->peer,
+qemu_peer_set_offload(qemu_get_queue(s->nic),
 rxcso_supported,
 s->lro_supported,
 s->lro_supported,
@@ -1883,11 +1883,9 @@ static NetClientInfo net_vmxnet3_info = {
 
 static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s)
 {
-NetClientState *peer = qemu_get_queue(s->nic)->peer;
+NetClientState *nc = qemu_get_queue(s->nic);
 
-if ((NULL != peer)  &&
-(peer->info->type == NET_CLIENT_OPTIONS_KIND_TAP)   &&
-tap_has_vnet_hdr(peer)) {
+if (qemu_peer_has_vnet_hdr(nc)) {
 return true;
 }
 
@@ -1935,10 +1933,10 @@ static void vmxnet3_net_init(VMXNET3State *s)
 s->lro_supported = false;
 
 if (s->peer_has_vhdr) {
-tap_set_vnet_hdr_len(qemu_get_queue(s->nic)->peer,
+qemu_peer_set_vnet_hdr_len(qemu_get_queue(s->nic),
 sizeof(struct virtio_net_hdr));
 
-tap_using_vnet_hdr(qemu_get_queue(s->nic)->peer, 1);
+qemu_peer_using_vnet_hdr(qemu_get_queue(s->nic)->peer, 1);
 }
 
 qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
-- 
1.8.5.2

[Qemu-devel] [PATCH v3 0/5] Add netmap backend offloadings support

[Vincenzo Maffione]

The purpose of this patch series is to add offloadings support
(TSO/UFO/CSUM) to the netmap network backend, and make it possible
for the paravirtual network frontends (virtio-net and vmxnet3) to
use it.
In order to achieve this, these patches extend the existing
net.h interface to add abstract operations through which a network
frontend can manipulate backend offloading features, instead of
directly calling TAP-specific functions.

Guest-to-guest performance before this patches for virtio-net + netmap:

TCP_STREAM  5.0 Gbps
TCP_RR  12.7 Gbps
UDP_STREAM (64-bytes)   790 Kpps

Guest-to-guest performance after this patches for virtio-net + netmap:

TCP_STREAM  21.4 Gbps
TCP_RR  12.7 Gbps
UDP_STREAM (64-bytes)   790 Kpps

Experiment details:
- Processor: Intel i7-3770K CPU @ 3.50GHz (8 cores)
- Memory @ 1333 MHz
- Host O.S.: Archlinux with Linux 3.11
- Guest O.S.: Archlinux with Linux 3.11

- QEMU command line:
qemu-system-x86_64 archdisk.qcow -snapshot -enable-kvm -device 
virtio-net-pci,ioeventfd=on,mac=00:AA:BB:CC:DD:01,netdev=mynet -netdev 
netmap,ifname=vale0:01,id=mynet -smp 2 -vga std -m 3G


 Changes against the previous version ***
NOTE: Now the VALE switch supports the offloadings, meaning that TCP/UDP will 
work
  even between a virtio-net guest with offloadings enabled and another
  guest with offloadings disabled. This has been achieved without 
performance loss
  on the "fast" case (e.g. two virtio-net-guest communicating with 
offloadings on).

(1) removed the second commit of the previous version, which was
intended to remove tap_using_vnet_hdr()
(2) the last commit has been revised: NETMAP_BDG_OFFSET becomes
NETMAP_BDG_VNET_HDR, netmap_set_offload() implemented and
netmap_has_vnet_hdr_len() rewritten.


Vincenzo Maffione (5):
  net: change vnet-hdr TAP prototypes
  net: extend NetClientInfo for offloading
  net: TAP uses NetClientInfo offloading callbacks
  net: virtio-net and vmxnet3 use offloading API
  net: add offloading support to netmap backend

 hw/net/virtio-net.c | 16 +
 hw/net/vmxnet3.c| 12 --
 include/net/net.h   | 19 +++
 include/net/tap.h   |  4 ++--
 net/net.c   | 55 +++
 net/netmap.c| 68 -
 net/tap-win32.c |  8 +++
 net/tap.c   | 12 +++---
 8 files changed, 167 insertions(+), 27 deletions(-)

-- 
1.8.5.2

Re: [Qemu-devel] [PATCH v2 0/6] Add netmap backend offloadings support

[Vincenzo Maffione]

2014/1/16 Stefan Hajnoczi 

> On Tue, Jan 14, 2014 at 11:59:44AM +0100, Vincenzo Maffione wrote:
> > (3) There is actually an important problem. In the previous patch
> version, TCP/UDP traffic was
> > supported between two guests attached to a VALE switch if and only
> if both guests use (or
> > don't) the same offloadings: e.g. two virtio-net guests or two e1000
> guests. This is why
> > in this version I put the commit which adds the support for netmap
> as the last one and
> > I temporarily disable the offloading feature (e.g.
> netmap_has_vnet_hdr() returns false).
> > We are working at adding proper support also in the general case in
> which there is
> > a mismatch between the NIC offloadings capabilities. As soon as we
> have proper support,
> > we can enable it in net/netmap.c.
> > What do you think about that? What's the best way to manage this
> temporary lack of
> > support?
>
> What you described is known problem in QEMU.  You cannot unplug a
> vnet_hdr-enabled tap netdev and plug in a non-vnet_hdr socket netdev
> since the socket netdev and its remote side don't understand vnet_hdr.
> This has stopped us from supporting changing NetClient peers at runtime.
>
> When this issue was raised we figured we'll have to add code to QEMU to
> emulate the offload in software (i.e. TSO, checksums, etc).  But no one
> has implemented that yet (although vmxnet3 has VMware offload software
> emulation IIRC).
>
> So maybe the answer is to finally implement vnet_hdr offload emulation
> inside QEMU?  Then netmap can negotiate with its remote side and fall
> back to offload emulation if the remote side doesn't understand
> vnet_hdr.
>
> Keep in mind that virtio-net does not allow the host to disable an
> offload feature that was already enabled, except after the device has
> been reset.  This precludes a simple solution where we just tell the
> guest to stop using vnet_hdr.
>

So what you are saying is that once you turn on an offload feature, it's
not possible to turn it off (apart from device reset).
By the way, let me ask a "sidechannel" question.
It's not clear to me how the virtio-net device decides which features to
enable and which features not. I guess there is a negotiation between the
guest virtio-net driver and the QEMU virtio-net frontend, and this is good.
But if I am not mistaken there is not a negotiation between the virtio-net
frontend and the netdev (the backend), apart from the UFO feature (I see a
tap_has_ufo() function). This means that the virtio-frontend will enable
the feature negotiated with the guest driver regardless of what the backend
is able to do. This is ok as long as only tap provides those offloadings,
but don't you see the need for a more general negotiation also between
netdev and the frontend?e.g. extend tap_has_ufo() to all the other
offloadings?


>
> I don't want to merge the tap -> net API changes and netmap offload
> enablement until there is a solution to this.
>
>
Ok, you pointed out a possible solution, but at the moment I don't think is
easy/convenient to add negotiation support to netmap (for example, in the
VALE switch there are more than two ports, so many possible pairs to keep
track...).

I'm currently working on adding offloading support inside netmap (so, no
QEMU code) and this will be enough to fix the problem for netmap, meaning
that two arbitrary netmap clients will be able to communicate regardless of
wether they understand or not the virtio-net-header, exactly how the tap
(and the associated in-kernel bridge) is currently able to do. Will you
accept the patches provided I complete the offloading support?

As a further step, I could try to convert the offloading code for QEMU, so
that you can solve the socket problem you described, regardless of netmap.

Does this sound good to you?





> > Vincenzo Maffione (6):
> >   net: change vnet-hdr TAP prototypes
> >   net: removing tap_using_vnet_hdr() function
>
> Happy with these two patches, we can merge them regardless of the rest
> of this series.  Still giving Michael Tsirkin a chance to review since
> the special tap vnet_hdr interface is used closely by vhost_net.
>



-- 
Vincenzo Maffione

[Qemu-devel] [PATCH v2 5/6] net: virtio-net and vmxnet3 use offloading API

[Vincenzo Maffione]

With this patch, virtio-net and vmxnet3 frontends make
use of the qemu_peer_* API for backend offloadings manipulations,
instead of calling TAP-specific functions directly.
We also remove the existing checks which prevent those frontends
from using offloadings with backends different from TAP (e.g. netmap).

Signed-off-by: Vincenzo Maffione 
---
 hw/net/virtio-net.c | 14 +-
 hw/net/vmxnet3.c| 10 --
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index ac8322d..c850d5b 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -325,11 +325,7 @@ static void peer_test_vnet_hdr(VirtIONet *n)
 return;
 }
 
-if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
-return;
-}
-
-n->has_vnet_hdr = tap_has_vnet_hdr(nc->peer);
+n->has_vnet_hdr = qemu_peer_has_vnet_hdr(nc);
 }
 
 static int peer_has_vnet_hdr(VirtIONet *n)
@@ -342,7 +338,7 @@ static int peer_has_ufo(VirtIONet *n)
 if (!peer_has_vnet_hdr(n))
 return 0;
 
-n->has_ufo = tap_has_ufo(qemu_get_queue(n->nic)->peer);
+n->has_ufo = qemu_peer_has_ufo(qemu_get_queue(n->nic));
 
 return n->has_ufo;
 }
@@ -361,8 +357,8 @@ static void virtio_net_set_mrg_rx_bufs(VirtIONet *n, int 
mergeable_rx_bufs)
 nc = qemu_get_subqueue(n->nic, i);
 
 if (peer_has_vnet_hdr(n) &&
-tap_has_vnet_hdr_len(nc->peer, n->guest_hdr_len)) {
-tap_set_vnet_hdr_len(nc->peer, n->guest_hdr_len);
+qemu_peer_has_vnet_hdr_len(nc, n->guest_hdr_len)) {
+qemu_peer_set_vnet_hdr_len(nc, n->guest_hdr_len);
 n->host_hdr_len = n->guest_hdr_len;
 }
 }
@@ -463,7 +459,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
 
 static void virtio_net_apply_guest_offloads(VirtIONet *n)
 {
-tap_set_offload(qemu_get_subqueue(n->nic, 0)->peer,
+qemu_peer_set_offload(qemu_get_subqueue(n->nic, 0),
 !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_CSUM)),
 !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO4)),
 !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO6)),
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 096b598..ded53a4 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1290,7 +1290,7 @@ static void vmxnet3_update_features(VMXNET3State *s)
   s->lro_supported, rxcso_supported,
   s->rx_vlan_stripping);
 if (s->peer_has_vhdr) {
-tap_set_offload(qemu_get_queue(s->nic)->peer,
+qemu_peer_set_offload(qemu_get_queue(s->nic),
 rxcso_supported,
 s->lro_supported,
 s->lro_supported,
@@ -1883,11 +1883,9 @@ static NetClientInfo net_vmxnet3_info = {
 
 static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s)
 {
-NetClientState *peer = qemu_get_queue(s->nic)->peer;
+NetClientState *nc = qemu_get_queue(s->nic);
 
-if ((NULL != peer)  &&
-(peer->info->type == NET_CLIENT_OPTIONS_KIND_TAP)   &&
-tap_has_vnet_hdr(peer)) {
+if (qemu_peer_has_vnet_hdr(nc)) {
 return true;
 }
 
@@ -1935,7 +1933,7 @@ static void vmxnet3_net_init(VMXNET3State *s)
 s->lro_supported = false;
 
 if (s->peer_has_vhdr) {
-tap_set_vnet_hdr_len(qemu_get_queue(s->nic)->peer,
+qemu_peer_set_vnet_hdr_len(qemu_get_queue(s->nic),
 sizeof(struct virtio_net_hdr));
 }
 
-- 
1.8.5.2

[Qemu-devel] [PATCH v2 6/6] net: add offloadings support to netmap backend

[Vincenzo Maffione]

Whit this patch, the netmap backend supports TSO/UFO/CSUM
offloadings, and accepts the virtio-net header, similarly to what
happens with TAP. The offloading callbacks in the NetClientInfo
interface have been implemented.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 59 ++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/net/netmap.c b/net/netmap.c
index 0ccc497..d99bb29 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -54,6 +54,7 @@ typedef struct NetmapState {
 boolread_poll;
 boolwrite_poll;
 struct ioveciov[IOV_MAX];
+int vnet_hdr_len;  /* Current virtio-net header length. */
 } NetmapState;
 
 #define D(format, ...)  \
@@ -274,7 +275,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 return iov_size(iov, iovcnt);
 }
 
-i = ring->cur;
+last = i = ring->cur;
 avail = ring->avail;
 
 if (avail < iovcnt) {
@@ -394,6 +395,56 @@ static void netmap_cleanup(NetClientState *nc)
 s->me.fd = -1;
 }
 
+/* Offloading manipulation support callbacks.
+ *
+ * Currently we are simply able to pass the virtio-net header
+ * throughout the VALE switch, without modifying it or interpreting it.
+ * For this reason we are always able to support TSO/UFO/CSUM and
+ * different header lengths as long as two virtio-net-header-capable
+ * VMs are attached to the bridge.
+ */
+static bool netmap_has_ufo(NetClientState *nc)
+{
+return false; /* Temporarily disabled. */
+}
+
+static bool netmap_has_vnet_hdr(NetClientState *nc)
+{
+return false; /* Temporarily disabled. */
+}
+
+static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
+{
+return false; /* Temporarily disabled. */
+}
+
+static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int 
tso6,
+   int ecn, int ufo)
+{
+}
+
+static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+int err;
+struct nmreq req;
+
+/* Issue a NETMAP_BDG_OFFSET command to change the netmap adapter
+   offset of 'me->ifname'. */
+memset(&req, 0, sizeof(req));
+pstrcpy(req.nr_name, sizeof(req.nr_name), s->me.ifname);
+req.nr_version = NETMAP_API;
+req.nr_cmd = NETMAP_BDG_OFFSET;
+req.nr_arg1 = len;
+err = ioctl(s->me.fd, NIOCREGIF, &req);
+if (err) {
+error_report("Unable to execute NETMAP_BDG_OFFSET on %s: %s",
+ s->me.ifname, strerror(errno));
+} else {
+/* Keep track of the current length, may be useful in the future. */
+s->vnet_hdr_len = len;
+}
+}
 
 /* NetClientInfo methods */
 static NetClientInfo net_netmap_info = {
@@ -403,6 +454,11 @@ static NetClientInfo net_netmap_info = {
 .receive_iov = netmap_receive_iov,
 .poll = netmap_poll,
 .cleanup = netmap_cleanup,
+.has_ufo = netmap_has_ufo,
+.has_vnet_hdr = netmap_has_vnet_hdr,
+.has_vnet_hdr_len = netmap_has_vnet_hdr_len,
+.set_offload = netmap_set_offload,
+.set_vnet_hdr_len = netmap_set_vnet_hdr_len,
 };
 
 /* The exported init function
@@ -428,6 +484,7 @@ int net_init_netmap(const NetClientOptions *opts,
 nc = qemu_new_net_client(&net_netmap_info, peer, "netmap", name);
 s = DO_UPCAST(NetmapState, nc, nc);
 s->me = me;
+s->vnet_hdr_len = 0;
 netmap_read_poll(s, true); /* Initially only poll for reads. */
 
 return 0;
-- 
1.8.5.2

[Qemu-devel] [PATCH v2 4/6] net: TAP uses NetClientInfo offloading callbacks

[Vincenzo Maffione]

The TAP NetClientInfo structure is inizialized with the TAP-specific
callbacks that manipulates backend offloading features.

Signed-off-by: Vincenzo Maffione 
---
 net/tap.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/net/tap.c b/net/tap.c
index 42f768c..e6c598d 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -305,6 +305,11 @@ static NetClientInfo net_tap_info = {
 .receive_iov = tap_receive_iov,
 .poll = tap_poll,
 .cleanup = tap_cleanup,
+.has_ufo = tap_has_ufo,
+.has_vnet_hdr = tap_has_vnet_hdr,
+.has_vnet_hdr_len = tap_has_vnet_hdr_len,
+.set_offload = tap_set_offload,
+.set_vnet_hdr_len = tap_set_vnet_hdr_len,
 };
 
 static TAPState *net_tap_fd_init(NetClientState *peer,
-- 
1.8.5.2

[Qemu-devel] [PATCH v2 0/6] Add netmap backend offloadings support

[Vincenzo Maffione]

The purpose of this patch series is to add offloadings support
(TSO/UFO/CSUM) to the netmap network backend, and make it possible
for the paravirtual network frontends (virtio-net and vmxnet3) to
use it.
In order to achieve this, these patches extend the existing
net.h interface to add abstract operations through which a network
frontend can manipulate backend offloading features, instead of
directly calling TAP-specific functions.

Guest-to-guest performance before this patches for virtio-net + netmap:

TCP_STREAM  5.0 Gbps
TCP_RR  12.7 Gbps
UDP_STREAM (64-bytes)   790 Kpps

Guest-to-guest performance after this patches for virtio-net + netmap:

TCP_STREAM  21.4 Gbps
TCP_RR  12.7 Gbps
UDP_STREAM (64-bytes)   790 Kpps

Experiment details:
- Processor: Intel i7-3770K CPU @ 3.50GHz (8 cores)
- Memory @ 1333 MHz
- Host O.S.: Archlinux with Linux 3.11
- Guest O.S.: Archlinux with Linux 3.11

- QEMU command line:
qemu-system-x86_64 archdisk.qcow -snapshot -enable-kvm -device 
virtio-net-pci,ioeventfd=on,mac=00:AA:BB:CC:DD:01,netdev=mynet -netdev 
netmap,ifname=vale0:01,id=mynet -smp 2 -vga std -m 3G


 Changes against the previous version ***
(1) The first two commits were not included into the previous version. They
are used to clean the TAP interface before the offloadings manipulation
API is introduced.

(2) The fifth commit merges two commits from the previous version: 3/5 
"virtio-net and
vmxnet3 use offloading API" and 5/5 "virtio-net and vmxnet3 can use netmap 
offloadings".
This because of (3).

(3) There is actually an important problem. In the previous patch version, 
TCP/UDP traffic was
supported between two guests attached to a VALE switch if and only if both 
guests use (or
don't) the same offloadings: e.g. two virtio-net guests or two e1000 
guests. This is why
in this version I put the commit which adds the support for netmap as the 
last one and
I temporarily disable the offloading feature (e.g. netmap_has_vnet_hdr() 
returns false).
We are working at adding proper support also in the general case in which 
there is
a mismatch between the NIC offloadings capabilities. As soon as we have 
proper support,
we can enable it in net/netmap.c.
What do you think about that? What's the best way to manage this temporary 
lack of
    support?


Vincenzo Maffione (6):
  net: change vnet-hdr TAP prototypes
  net: removing tap_using_vnet_hdr() function
  net: extend NetClientInfo for offloading manipulations
  net: TAP uses NetClientInfo offloading callbacks
  net: virtio-net and vmxnet3 use offloading API
  net: add offloadings support to netmap backend

 hw/net/virtio-net.c | 18 +---
 hw/net/vmxnet3.c| 12 ---
 include/net/net.h   | 17 +++
 include/net/tap.h   |  5 ++---
 net/net.c   | 46 +
 net/netmap.c| 59 -
 net/tap-win32.c | 12 ---
 net/tap.c   | 22 
 8 files changed, 145 insertions(+), 46 deletions(-)

-- 
1.8.5.2

[Qemu-devel] [PATCH v2 1/6] net: change vnet-hdr TAP prototypes

[Vincenzo Maffione]

The tap_has_vnet_hdr() and tap_has_vnet_hdr_len() functions used
to return int, even though they only return true/false values.
This patch changes the prototypes to return bool.

Signed-off-by: Vincenzo Maffione 
---
 include/net/tap.h | 4 ++--
 net/tap-win32.c   | 8 
 net/tap.c | 6 +++---
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/net/tap.h b/include/net/tap.h
index a994f20..a3490a9 100644
--- a/include/net/tap.h
+++ b/include/net/tap.h
@@ -30,8 +30,8 @@
 #include "qapi-types.h"
 
 bool tap_has_ufo(NetClientState *nc);
-int tap_has_vnet_hdr(NetClientState *nc);
-int tap_has_vnet_hdr_len(NetClientState *nc, int len);
+bool tap_has_vnet_hdr(NetClientState *nc);
+bool tap_has_vnet_hdr_len(NetClientState *nc, int len);
 void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr);
 void tap_set_offload(NetClientState *nc, int csum, int tso4, int tso6, int 
ecn, int ufo);
 void tap_set_vnet_hdr_len(NetClientState *nc, int len);
diff --git a/net/tap-win32.c b/net/tap-win32.c
index 91e9e84..edf26c4 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -727,9 +727,9 @@ bool tap_has_ufo(NetClientState *nc)
 return false;
 }
 
-int tap_has_vnet_hdr(NetClientState *nc)
+bool tap_has_vnet_hdr(NetClientState *nc)
 {
-return 0;
+return false;
 }
 
 int tap_probe_vnet_hdr_len(int fd, int len)
@@ -755,9 +755,9 @@ struct vhost_net *tap_get_vhost_net(NetClientState *nc)
 return NULL;
 }
 
-int tap_has_vnet_hdr_len(NetClientState *nc, int len)
+bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
-return 0;
+return false;
 }
 
 void tap_set_vnet_hdr_len(NetClientState *nc, int len)
diff --git a/net/tap.c b/net/tap.c
index 39c1cda..c805f3c 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -219,7 +219,7 @@ bool tap_has_ufo(NetClientState *nc)
 return s->has_ufo;
 }
 
-int tap_has_vnet_hdr(NetClientState *nc)
+bool tap_has_vnet_hdr(NetClientState *nc)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
@@ -228,13 +228,13 @@ int tap_has_vnet_hdr(NetClientState *nc)
 return !!s->host_vnet_hdr_len;
 }
 
-int tap_has_vnet_hdr_len(NetClientState *nc, int len)
+bool tap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
 TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
 assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_TAP);
 
-return tap_probe_vnet_hdr_len(s->fd, len);
+return !!tap_probe_vnet_hdr_len(s->fd, len);
 }
 
 void tap_set_vnet_hdr_len(NetClientState *nc, int len)
-- 
1.8.5.2

[Qemu-devel] [PATCH v2 2/6] net: removing tap_using_vnet_hdr() function

[Vincenzo Maffione]

This function was used to set the using_vnet_hdr field into the
TAPState struct. However, it is always called immediately before
(see virtio-net.c) or immediately after (see vmxnet3.c) the function
tap_set_vnet_hdr_len(). It's therefore possible to set the
using_vnet_hdr field directly in tap_set_vnet_hdr_len() and remove
tap_using_vnet_hdr(), making the code simpler.

Signed-off-by: Vincenzo Maffione 
---
 hw/net/virtio-net.c |  4 
 hw/net/vmxnet3.c|  2 --
 include/net/tap.h   |  1 -
 net/tap-win32.c |  4 
 net/tap.c   | 11 +--
 5 files changed, 1 insertion(+), 21 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3626608..ac8322d 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1496,7 +1496,6 @@ static void virtio_net_device_realize(DeviceState *dev, 
Error **errp)
 VirtIODevice *vdev = VIRTIO_DEVICE(dev);
 VirtIONet *n = VIRTIO_NET(dev);
 NetClientState *nc;
-int i;
 
 virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
 
@@ -1543,9 +1542,6 @@ static void virtio_net_device_realize(DeviceState *dev, 
Error **errp)
 
 peer_test_vnet_hdr(n);
 if (peer_has_vnet_hdr(n)) {
-for (i = 0; i < n->max_queues; i++) {
-tap_using_vnet_hdr(qemu_get_subqueue(n->nic, i)->peer, true);
-}
 n->host_hdr_len = sizeof(struct virtio_net_hdr);
 } else {
 n->host_hdr_len = 0;
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 19687aa..096b598 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1937,8 +1937,6 @@ static void vmxnet3_net_init(VMXNET3State *s)
 if (s->peer_has_vhdr) {
 tap_set_vnet_hdr_len(qemu_get_queue(s->nic)->peer,
 sizeof(struct virtio_net_hdr));
-
-tap_using_vnet_hdr(qemu_get_queue(s->nic)->peer, 1);
 }
 
 qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
diff --git a/include/net/tap.h b/include/net/tap.h
index a3490a9..54974dc 100644
--- a/include/net/tap.h
+++ b/include/net/tap.h
@@ -32,7 +32,6 @@
 bool tap_has_ufo(NetClientState *nc);
 bool tap_has_vnet_hdr(NetClientState *nc);
 bool tap_has_vnet_hdr_len(NetClientState *nc, int len);
-void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr);
 void tap_set_offload(NetClientState *nc, int csum, int tso4, int tso6, int 
ecn, int ufo);
 void tap_set_vnet_hdr_len(NetClientState *nc, int len);
 int tap_enable(NetClientState *nc);
diff --git a/net/tap-win32.c b/net/tap-win32.c
index edf26c4..f4cd002 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -741,10 +741,6 @@ void tap_fd_set_vnet_hdr_len(int fd, int len)
 {
 }
 
-void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr)
-{
-}
-
 void tap_set_offload(NetClientState *nc, int csum, int tso4,
  int tso6, int ecn, int ufo)
 {
diff --git a/net/tap.c b/net/tap.c
index c805f3c..42f768c 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -247,16 +247,7 @@ void tap_set_vnet_hdr_len(NetClientState *nc, int len)
 
 tap_fd_set_vnet_hdr_len(s->fd, len);
 s->host_vnet_hdr_len = len;
-}
-
-void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr)
-{
-TAPState *s = DO_UPCAST(TAPState, nc, nc);
-
-assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_TAP);
-assert(!!s->host_vnet_hdr_len == using_vnet_hdr);
-
-s->using_vnet_hdr = using_vnet_hdr;
+s->using_vnet_hdr = true;
 }
 
 void tap_set_offload(NetClientState *nc, int csum, int tso4,
-- 
1.8.5.2

[Qemu-devel] [PATCH v2 3/6] net: extend NetClientInfo for offloading manipulations

[Vincenzo Maffione]

A set of new callbacks has been added to the NetClientInfo struct in
order to abstract the operations done by virtio-net and vmxnet3
frontends to manipulate TAP offloadings.

The net.h API has been extended with functions that access those
abstract operations, providing frontends with a way to manipulate
backend offloadings.

Signed-off-by: Vincenzo Maffione 
---
 include/net/net.h | 17 +
 net/net.c | 46 ++
 2 files changed, 63 insertions(+)

diff --git a/include/net/net.h b/include/net/net.h
index 11e1468..29a0698 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -50,6 +50,11 @@ typedef void (NetCleanup) (NetClientState *);
 typedef void (LinkStatusChanged)(NetClientState *);
 typedef void (NetClientDestructor)(NetClientState *);
 typedef RxFilterInfo *(QueryRxFilter)(NetClientState *);
+typedef bool (HasUfo)(NetClientState *);
+typedef bool (HasVnetHdr)(NetClientState *);
+typedef bool (HasVnetHdrLen)(NetClientState *, int);
+typedef void (SetOffload)(NetClientState *, int, int, int, int, int);
+typedef void (SetVnetHdrLen)(NetClientState *, int);
 
 typedef struct NetClientInfo {
 NetClientOptionsKind type;
@@ -62,6 +67,11 @@ typedef struct NetClientInfo {
 LinkStatusChanged *link_status_changed;
 QueryRxFilter *query_rx_filter;
 NetPoll *poll;
+HasUfo *has_ufo;
+HasVnetHdr *has_vnet_hdr;
+HasVnetHdrLen *has_vnet_hdr_len;
+SetOffload *set_offload;
+SetVnetHdrLen *set_vnet_hdr_len;
 } NetClientInfo;
 
 struct NetClientState {
@@ -120,6 +130,13 @@ ssize_t qemu_send_packet_async(NetClientState *nc, const 
uint8_t *buf,
 void qemu_purge_queued_packets(NetClientState *nc);
 void qemu_flush_queued_packets(NetClientState *nc);
 void qemu_format_nic_info_str(NetClientState *nc, uint8_t macaddr[6]);
+bool qemu_peer_has_ufo(NetClientState *nc);
+bool qemu_peer_has_vnet_hdr(NetClientState *nc);
+bool qemu_peer_has_vnet_hdr_len(NetClientState *nc, int len);
+void qemu_peer_using_vnet_hdr(NetClientState *nc, bool enable);
+void qemu_peer_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
+   int ecn, int ufo);
+void qemu_peer_set_vnet_hdr_len(NetClientState *nc, int len);
 void qemu_macaddr_default_if_unset(MACAddr *macaddr);
 int qemu_show_nic_models(const char *arg, const char *const *models);
 void qemu_check_nic_model(NICInfo *nd, const char *model);
diff --git a/net/net.c b/net/net.c
index f8db85f..bc90a27 100644
--- a/net/net.c
+++ b/net/net.c
@@ -381,6 +381,52 @@ void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
 }
 }
 
+bool qemu_peer_has_ufo(NetClientState *nc)
+{
+if (!nc->peer || !nc->peer->info->has_ufo) {
+return false;
+}
+
+return nc->peer->info->has_ufo(nc->peer);
+}
+
+bool qemu_peer_has_vnet_hdr(NetClientState *nc)
+{
+if (!nc->peer || !nc->peer->info->has_vnet_hdr) {
+return false;
+}
+
+return nc->peer->info->has_vnet_hdr(nc->peer);
+}
+
+bool qemu_peer_has_vnet_hdr_len(NetClientState *nc, int len)
+{
+if (!nc->peer || !nc->peer->info->has_vnet_hdr_len) {
+return false;
+}
+
+return nc->peer->info->has_vnet_hdr_len(nc->peer, len);
+}
+
+void qemu_peer_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
+  int ecn, int ufo)
+{
+if (!nc->peer || !nc->peer->info->set_offload) {
+return;
+}
+
+nc->peer->info->set_offload(nc->peer, csum, tso4, tso6, ecn, ufo);
+}
+
+void qemu_peer_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+if (!nc->peer || !nc->peer->info->set_vnet_hdr_len) {
+return;
+}
+
+nc->peer->info->set_vnet_hdr_len(nc->peer, len);
+}
+
 int qemu_can_send_packet(NetClientState *sender)
 {
 if (!sender->peer) {
-- 
1.8.5.2

Re: [Qemu-devel] [PATCH 4/5] net: add offloadings support to netmap backend

[Vincenzo Maffione]

2014/1/13 Stefan Hajnoczi 

> On Fri, Dec 13, 2013 at 01:05:02PM +0100, Vincenzo Maffione wrote:
> > +static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
> > +{
> > +}
>
> I was trying to figure out whether it's okay for this function to be a
> nop.  I've come to the conclusion that it's okay:
>
> If the netdev supports vnet_hdr then we enable vnet_hdr.  If not, it
> will not enable it.
>
> Other NICs never enable vnet_hdr even if the netdev supports it.
>
> This interface is basically useless because set_vnet_hdr_len() is
> also needed and provides the same information, i.e. that we are
> transitioning from vnet_hdr off -> on.
>
> The bool argument is misleading since we never use this function to
> disable vnet_hdr.  It's impossible to transition on -> off.
>
> I suggest removing using_vnet_hdr() and instead relying solely on
> set_vnet_hdr_len().  Do this as the first patch in the series before
> introducing the function pointers, that way all your following patches
> become simpler.
>
> Completely agree. As usual, I didn't want to change too much the existing
code.
This means that I have to remove the tap_using_vnet_hdr() calls from
virtio-net and vmxnet3 NICs before this patches, haven't I?



> > +static void netmap_set_offload(NetClientState *nc, int csum, int tso4,
> int tso6,
> > +   int ecn, int ufo)
> > +{
> > +}
>
> This interface is necessary for toggling offload features at runtime,
> e.g. because ethtool was used inside the guest.  Offloads can impact
> performance and sometimes expose bugs.  Therefore users may wish to
> disable certain offloads.
>
> Please consider supporting set_offload()!
>

Yes, we are considering to do that, but at the moment there is no such a
support.


>
> > +static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
> > +{
> > +NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
> > +int err;
> > +struct nmreq req;
> > +
> > +/* Issue a NETMAP_BDG_OFFSET command to change the netmap adapter
> > +   offset of 'me->ifname'. */
> > +memset(&req, 0, sizeof(req));
> > +pstrcpy(req.nr_name, sizeof(req.nr_name), s->me.ifname);
> > +req.nr_version = NETMAP_API;
> > +req.nr_cmd = NETMAP_BDG_OFFSET;
> > +req.nr_arg1 = len;
> > +err = ioctl(s->me.fd, NIOCREGIF, &req);
> > +if (err) {
> > +error_report("Unable to execute NETMAP_BDG_OFFSET on %s: %s",
> > + s->me.ifname, strerror(errno));
> > +} else {
> > +/* Keep track of the current length, may be usefule in the
> future. */
>
> s/usefule/useful/
>



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH 1/5] net: extend NetClientInfo for offloading manipulations

[Vincenzo Maffione]

Ok.
I did not include those modifications into the patch in order to avoid
modifications to the TAP netdev.


2014/1/13 Stefan Hajnoczi 

> On Fri, Dec 13, 2013 at 01:04:59PM +0100, Vincenzo Maffione wrote:
> > diff --git a/include/net/net.h b/include/net/net.h
> > index 11e1468..f5b5bae 100644
> > --- a/include/net/net.h
> > +++ b/include/net/net.h
> > @@ -50,6 +50,12 @@ typedef void (NetCleanup) (NetClientState *);
> >  typedef void (LinkStatusChanged)(NetClientState *);
> >  typedef void (NetClientDestructor)(NetClientState *);
> >  typedef RxFilterInfo *(QueryRxFilter)(NetClientState *);
> > +typedef bool (HasUfo)(NetClientState *);
> > +typedef int (HasVnetHdr)(NetClientState *);
>
> Please change the return type to bool.  bool is easier to understand
> since the reader can be sure the variable only takes true/false values.
>
> > +typedef int (HasVnetHdrLen)(NetClientState *, int);
>
> Please change the return type to bool.
>



-- 
Vincenzo Maffione

[Qemu-devel] [PATCH 4/5] net: add offloadings support to netmap backend

[Vincenzo Maffione]

Whit this patch, the netmap backend supports TSO/UFO/CSUM
offloadings, and accepts the virtio-net header, similarly to what
happens with TAP. The offloading callbacks in the NetClientInfo
interface have been implemented.

Signed-off-by: Vincenzo Maffione 
---
 net/netmap.c | 64 +++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/net/netmap.c b/net/netmap.c
index 0ccc497..f02a574 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -54,6 +54,7 @@ typedef struct NetmapState {
 boolread_poll;
 boolwrite_poll;
 struct ioveciov[IOV_MAX];
+int vnet_hdr_len;  /* Current virtio-net header length. */
 } NetmapState;
 
 #define D(format, ...)  \
@@ -274,7 +275,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
 return iov_size(iov, iovcnt);
 }
 
-i = ring->cur;
+last = i = ring->cur;
 avail = ring->avail;
 
 if (avail < iovcnt) {
@@ -394,6 +395,60 @@ static void netmap_cleanup(NetClientState *nc)
 s->me.fd = -1;
 }
 
+/* Offloading manipulation support callbacks.
+ *
+ * Currently we are simply able to pass the virtio-net header
+ * throughout the VALE switch, without modifying it or interpreting it.
+ * For this reason we are always able to support TSO/UFO/CSUM and
+ * different header lengths as long as two virtio-net-header-capable
+ * VMs are attached to the bridge.
+ */
+static bool netmap_has_ufo(NetClientState *nc)
+{
+return true;
+}
+
+static int netmap_has_vnet_hdr(NetClientState *nc)
+{
+return true;
+}
+
+static int netmap_has_vnet_hdr_len(NetClientState *nc, int len)
+{
+return len >= 0 && len <= NETMAP_BDG_MAX_OFFSET;
+}
+
+static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
+{
+}
+
+static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int 
tso6,
+   int ecn, int ufo)
+{
+}
+
+static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+int err;
+struct nmreq req;
+
+/* Issue a NETMAP_BDG_OFFSET command to change the netmap adapter
+   offset of 'me->ifname'. */
+memset(&req, 0, sizeof(req));
+pstrcpy(req.nr_name, sizeof(req.nr_name), s->me.ifname);
+req.nr_version = NETMAP_API;
+req.nr_cmd = NETMAP_BDG_OFFSET;
+req.nr_arg1 = len;
+err = ioctl(s->me.fd, NIOCREGIF, &req);
+if (err) {
+error_report("Unable to execute NETMAP_BDG_OFFSET on %s: %s",
+ s->me.ifname, strerror(errno));
+} else {
+/* Keep track of the current length, may be usefule in the future. */
+s->vnet_hdr_len = len;
+}
+}
 
 /* NetClientInfo methods */
 static NetClientInfo net_netmap_info = {
@@ -403,6 +458,12 @@ static NetClientInfo net_netmap_info = {
 .receive_iov = netmap_receive_iov,
 .poll = netmap_poll,
 .cleanup = netmap_cleanup,
+.has_ufo = netmap_has_ufo,
+.has_vnet_hdr = netmap_has_vnet_hdr,
+.has_vnet_hdr_len = netmap_has_vnet_hdr_len,
+.using_vnet_hdr = netmap_using_vnet_hdr,
+.set_offload = netmap_set_offload,
+.set_vnet_hdr_len = netmap_set_vnet_hdr_len,
 };
 
 /* The exported init function
@@ -428,6 +489,7 @@ int net_init_netmap(const NetClientOptions *opts,
 nc = qemu_new_net_client(&net_netmap_info, peer, "netmap", name);
 s = DO_UPCAST(NetmapState, nc, nc);
 s->me = me;
+s->vnet_hdr_len = 0;
 netmap_read_poll(s, true); /* Initially only poll for reads. */
 
 return 0;
-- 
1.8.5.1

[Qemu-devel] [PATCH 3/5] net: virtio-net and vmxnet3 use offloading API

[Vincenzo Maffione]

With this patch, virtio-net and vmxnet3 frontends make
use of the qemu_peer_* API for backend offloadings manipulations,
instead of calling TAP-specific functions directly.

Signed-off-by: Vincenzo Maffione 
---
 hw/net/virtio-net.c | 12 ++--
 hw/net/vmxnet3.c| 14 +++---
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index d312b9c..c8ee2fa 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -329,7 +329,7 @@ static void peer_test_vnet_hdr(VirtIONet *n)
 return;
 }
 
-n->has_vnet_hdr = tap_has_vnet_hdr(nc->peer);
+n->has_vnet_hdr = qemu_peer_has_vnet_hdr(nc);
 }
 
 static int peer_has_vnet_hdr(VirtIONet *n)
@@ -342,7 +342,7 @@ static int peer_has_ufo(VirtIONet *n)
 if (!peer_has_vnet_hdr(n))
 return 0;
 
-n->has_ufo = tap_has_ufo(qemu_get_queue(n->nic)->peer);
+n->has_ufo = qemu_peer_has_ufo(qemu_get_queue(n->nic));
 
 return n->has_ufo;
 }
@@ -361,8 +361,8 @@ static void virtio_net_set_mrg_rx_bufs(VirtIONet *n, int 
mergeable_rx_bufs)
 nc = qemu_get_subqueue(n->nic, i);
 
 if (peer_has_vnet_hdr(n) &&
-tap_has_vnet_hdr_len(nc->peer, n->guest_hdr_len)) {
-tap_set_vnet_hdr_len(nc->peer, n->guest_hdr_len);
+qemu_peer_has_vnet_hdr_len(nc, n->guest_hdr_len)) {
+qemu_peer_set_vnet_hdr_len(nc, n->guest_hdr_len);
 n->host_hdr_len = n->guest_hdr_len;
 }
 }
@@ -463,7 +463,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
 
 static void virtio_net_apply_guest_offloads(VirtIONet *n)
 {
-tap_set_offload(qemu_get_subqueue(n->nic, 0)->peer,
+qemu_peer_set_offload(qemu_get_subqueue(n->nic, 0),
 !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_CSUM)),
 !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO4)),
 !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO6)),
@@ -1546,7 +1546,7 @@ static int virtio_net_device_init(VirtIODevice *vdev)
 peer_test_vnet_hdr(n);
 if (peer_has_vnet_hdr(n)) {
 for (i = 0; i < n->max_queues; i++) {
-tap_using_vnet_hdr(qemu_get_subqueue(n->nic, i)->peer, true);
+qemu_peer_using_vnet_hdr(qemu_get_subqueue(n->nic, i), true);
 }
 n->host_hdr_len = sizeof(struct virtio_net_hdr);
 } else {
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 19687aa..f00c649 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1290,7 +1290,7 @@ static void vmxnet3_update_features(VMXNET3State *s)
   s->lro_supported, rxcso_supported,
   s->rx_vlan_stripping);
 if (s->peer_has_vhdr) {
-tap_set_offload(qemu_get_queue(s->nic)->peer,
+qemu_peer_set_offload(qemu_get_queue(s->nic),
 rxcso_supported,
 s->lro_supported,
 s->lro_supported,
@@ -1883,11 +1883,11 @@ static NetClientInfo net_vmxnet3_info = {
 
 static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s)
 {
-NetClientState *peer = qemu_get_queue(s->nic)->peer;
+NetClientState *nc = qemu_get_queue(s->nic);
 
-if ((NULL != peer)  &&
-(peer->info->type == NET_CLIENT_OPTIONS_KIND_TAP)   &&
-tap_has_vnet_hdr(peer)) {
+if ((NULL != nc->peer)  &&
+(nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_TAP)   &&
+qemu_peer_has_vnet_hdr(nc)) {
 return true;
 }
 
@@ -1935,10 +1935,10 @@ static void vmxnet3_net_init(VMXNET3State *s)
 s->lro_supported = false;
 
 if (s->peer_has_vhdr) {
-tap_set_vnet_hdr_len(qemu_get_queue(s->nic)->peer,
+qemu_peer_set_vnet_hdr_len(qemu_get_queue(s->nic),
 sizeof(struct virtio_net_hdr));
 
-tap_using_vnet_hdr(qemu_get_queue(s->nic)->peer, 1);
+qemu_peer_using_vnet_hdr(qemu_get_queue(s->nic), 1);
 }
 
 qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
-- 
1.8.5.1

[Qemu-devel] [PATCH 1/5] net: extend NetClientInfo for offloading manipulations

[Vincenzo Maffione]

A set of new callbacks has been added to the NetClientInfo struct in
order to abstract the operations done by virtio-net and vmxnet3
frontends to manipulate TAP offloadings.

The net.h API has been extended with functions that access those
abstract operations, providing frontends with a way to manipulate
backend offloadings.

Signed-off-by: Vincenzo Maffione 
---
 include/net/net.h | 19 +++
 net/net.c | 55 +++
 2 files changed, 74 insertions(+)

diff --git a/include/net/net.h b/include/net/net.h
index 11e1468..f5b5bae 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -50,6 +50,12 @@ typedef void (NetCleanup) (NetClientState *);
 typedef void (LinkStatusChanged)(NetClientState *);
 typedef void (NetClientDestructor)(NetClientState *);
 typedef RxFilterInfo *(QueryRxFilter)(NetClientState *);
+typedef bool (HasUfo)(NetClientState *);
+typedef int (HasVnetHdr)(NetClientState *);
+typedef int (HasVnetHdrLen)(NetClientState *, int);
+typedef void (UsingVnetHdr)(NetClientState *, bool);
+typedef void (SetOffload)(NetClientState *, int, int, int, int, int);
+typedef void (SetVnetHdrLen)(NetClientState *, int);
 
 typedef struct NetClientInfo {
 NetClientOptionsKind type;
@@ -62,6 +68,12 @@ typedef struct NetClientInfo {
 LinkStatusChanged *link_status_changed;
 QueryRxFilter *query_rx_filter;
 NetPoll *poll;
+HasUfo *has_ufo;
+HasVnetHdr *has_vnet_hdr;
+HasVnetHdrLen *has_vnet_hdr_len;
+UsingVnetHdr *using_vnet_hdr;
+SetOffload *set_offload;
+SetVnetHdrLen *set_vnet_hdr_len;
 } NetClientInfo;
 
 struct NetClientState {
@@ -120,6 +132,13 @@ ssize_t qemu_send_packet_async(NetClientState *nc, const 
uint8_t *buf,
 void qemu_purge_queued_packets(NetClientState *nc);
 void qemu_flush_queued_packets(NetClientState *nc);
 void qemu_format_nic_info_str(NetClientState *nc, uint8_t macaddr[6]);
+bool qemu_peer_has_ufo(NetClientState *nc);
+int qemu_peer_has_vnet_hdr(NetClientState *nc);
+int qemu_peer_has_vnet_hdr_len(NetClientState *nc, int len);
+void qemu_peer_using_vnet_hdr(NetClientState *nc, bool enable);
+void qemu_peer_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
+   int ecn, int ufo);
+void qemu_peer_set_vnet_hdr_len(NetClientState *nc, int len);
 void qemu_macaddr_default_if_unset(MACAddr *macaddr);
 int qemu_show_nic_models(const char *arg, const char *const *models);
 void qemu_check_nic_model(NICInfo *nd, const char *model);
diff --git a/net/net.c b/net/net.c
index 9db88cc..96f05d9 100644
--- a/net/net.c
+++ b/net/net.c
@@ -381,6 +381,61 @@ void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
 }
 }
 
+bool qemu_peer_has_ufo(NetClientState *nc)
+{
+if (!nc->peer || !nc->peer->info->has_ufo) {
+return false;
+}
+
+return nc->peer->info->has_ufo(nc->peer);
+}
+
+int qemu_peer_has_vnet_hdr(NetClientState *nc)
+{
+if (!nc->peer || !nc->peer->info->has_vnet_hdr) {
+return false;
+}
+
+return nc->peer->info->has_vnet_hdr(nc->peer);
+}
+
+int qemu_peer_has_vnet_hdr_len(NetClientState *nc, int len)
+{
+if (!nc->peer || !nc->peer->info->has_vnet_hdr_len) {
+return false;
+}
+
+return nc->peer->info->has_vnet_hdr_len(nc->peer, len);
+}
+
+void qemu_peer_using_vnet_hdr(NetClientState *nc, bool enable)
+{
+if (!nc->peer || !nc->peer->info->using_vnet_hdr) {
+return;
+}
+
+nc->peer->info->using_vnet_hdr(nc->peer, enable);
+}
+
+void qemu_peer_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
+  int ecn, int ufo)
+{
+if (!nc->peer || !nc->peer->info->set_offload) {
+return;
+}
+
+nc->peer->info->set_offload(nc->peer, csum, tso4, tso6, ecn, ufo);
+}
+
+void qemu_peer_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+if (!nc->peer || !nc->peer->info->set_vnet_hdr_len) {
+return;
+}
+
+nc->peer->info->set_vnet_hdr_len(nc->peer, len);
+}
+
 int qemu_can_send_packet(NetClientState *sender)
 {
 if (!sender->peer) {
-- 
1.8.5.1

[Qemu-devel] [PATCH 5/5] net: virtio-net and vmxnet3 can use netmap offloadings

[Vincenzo Maffione]

With this patch we remove the existing checks in the virtio-net
and vmxnet3 frontends that prevents them from using
offloadings with backends different from TAP (e.g. netmap).

Signed-off-by: Vincenzo Maffione 
---
 hw/net/virtio-net.c | 4 
 hw/net/vmxnet3.c| 4 +---
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index c8ee2fa..8a94539 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -325,10 +325,6 @@ static void peer_test_vnet_hdr(VirtIONet *n)
 return;
 }
 
-if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
-return;
-}
-
 n->has_vnet_hdr = qemu_peer_has_vnet_hdr(nc);
 }
 
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index f00c649..0524684 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1885,9 +1885,7 @@ static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s)
 {
 NetClientState *nc = qemu_get_queue(s->nic);
 
-if ((NULL != nc->peer)  &&
-(nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_TAP)   &&
-qemu_peer_has_vnet_hdr(nc)) {
+if (qemu_peer_has_vnet_hdr(nc)) {
 return true;
 }
 
-- 
1.8.5.1

[Qemu-devel] [PATCH 2/5] net: TAP uses NetClientInfo offloading callbacks

[Vincenzo Maffione]

The TAP NetClientInfo structure is inizialized with the TAP-specific
callbacks that manipulates backend offloading features.

Signed-off-by: Vincenzo Maffione 
---
 net/tap.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/net/tap.c b/net/tap.c
index 39c1cda..175fcb3 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -314,6 +314,12 @@ static NetClientInfo net_tap_info = {
 .receive_iov = tap_receive_iov,
 .poll = tap_poll,
 .cleanup = tap_cleanup,
+.has_ufo = tap_has_ufo,
+.has_vnet_hdr = tap_has_vnet_hdr,
+.has_vnet_hdr_len = tap_has_vnet_hdr_len,
+.using_vnet_hdr = tap_using_vnet_hdr,
+.set_offload = tap_set_offload,
+.set_vnet_hdr_len = tap_set_vnet_hdr_len,
 };
 
 static TAPState *net_tap_fd_init(NetClientState *peer,
-- 
1.8.5.1

[Qemu-devel] [PATCH 0/5] Add netmap backend offloadings support

[Vincenzo Maffione]

The purpose of this patch series is to add offloadings support
(TSO/UFO/CSUM) to the netmap network backend, and make it possible
for the paravirtual network frontends (virtio-net and vmxnet3) to
use it.
In order to achieve this, these patches extend the existing
net.h interface to add abstract operations through which a network
frontend can manipulate backend offloading features, instead of
directly calling TAP-specific functions.

Guest-to-guest performance before this patches for virtio-net + netmap:

TCP_STREAM  5.0 Gbps
TCP_RR  12.7 Gbps
UDP_STREAM (64-bytes)   790 Kpps

Guest-to-guest performance after this patches for virtio-net + netmap:

TCP_STREAM  21.4 Gbps
TCP_RR  12.7 Gbps
UDP_STREAM (64-bytes)   790 Kpps

Experiment details:
- Processor: Intel i7-3770K CPU @ 3.50GHz (8 cores)
- Memory @ 1333 MHz
- Host O.S.: Archlinux with Linux 3.11
- Guest O.S.: Archlinux with Linux 3.11

- QEMU command line:
qemu-system-x86_64 archdisk.qcow -snapshot -enable-kvm -device 
virtio-net-pci,ioeventfd=on,mac=00:AA:BB:CC:DD:01,netdev=mynet -netdev 
netmap,ifname=vale0:01,id=mynet -smp 2 -vga std -m 3G


Vincenzo Maffione (5):
  net: extend NetClientInfo for offloading manipulations
  net: TAP uses NetClientInfo offloading callbacks
  net: virtio-net and vmxnet3 use offloading API
  net: add offloadings support to netmap backend
  net: virtio-net and vmxnet3 can use netmap offloadings

 hw/net/virtio-net.c | 16 +-
 hw/net/vmxnet3.c| 12 +-
 include/net/net.h   | 19 
 net/net.c   | 55 +
 net/netmap.c| 64 -
 net/tap.c   |  6 +
 6 files changed, 154 insertions(+), 18 deletions(-)

-- 
1.8.5.1

Re: [Qemu-devel] [PULL 1/4] net: Adding netmap network backend

[Vincenzo Maffione]

Do you want me to send the patch?

Thanks,
  Vincenzo


2013/12/11 Stefan Hajnoczi 

> On Tue, Dec 10, 2013 at 10:49:00AM -0700, Eric Blake wrote:
> > On 12/09/2013 06:11 AM, Stefan Hajnoczi wrote:
> > > +++ b/qapi-schema.json
> > > @@ -3009,6 +3009,27 @@
> > >  'hubid': 'int32' } }
> > >
> > >  ##
> > > +# @NetdevNetmapOptions
> > > +#
> > > +# Connect a client to a netmap-enabled NIC or to a VALE switch port
> > > +#
> > > +# @ifname: Either the name of an existing network interface supported
> by
> > > +#  netmap, or the name of a VALE port (created on the fly).
> > > +#  A VALE port name is in the form 'valeXXX:YYY', where XXX
> and
> > > +#  YYY are non-negative integers. XXX identifies a switch and
> > > +#  YYY identifies a port of the switch. VALE ports having the
> > > +#  same XXX are therefore connected to the same switch.
> > > +#
> > > +# @devname: #optional path of the netmap device (default:
> '/dev/netmap').
> > > +#
> > > +# Since 1.8
> >
> > s/1.8/2.0/
>
> Thank you for spotting this.  Sending a separate patch to fix this since
> the pull request has been merged.
>
> Stefan
>



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH] net: QEMU_NET_PACKET_FLAG_MORE introduced

[Vincenzo Maffione]

Ok,
   We will prepare the support for the vnet-header and offloadings.

However, this requires some API extensions because AFAIK only the TAP
backend currently supports TSO/UFO/CSUM, and consequently virtio-net code
directly calls TAP-specific functions like tap_set_offload(),
tap_using_vnet_hdr(), tap_has_vnet_hdr(),  These should become
something like qemu_peer_set_offload(), qemu_peer_using_vnet_hdr(), ...
adding the proper callbacks into the NetClientInfo struct.

Does this sound good to you?


Regards,
  Vincenzo


2013/12/9 Michael S. Tsirkin 

> On Mon, Dec 09, 2013 at 02:25:46PM +0100, Vincenzo Maffione wrote:
> >
> >
> >
> > 2013/12/9 Stefan Hajnoczi 
> >
> > On Mon, Dec 09, 2013 at 01:14:31PM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Dec 09, 2013 at 11:55:57AM +0100, Vincenzo Maffione wrote:
> > > > If you don't think adding the new flag support for virtio-net is
> a good
> > idea
> > > > (though TAP performance is not affected in every case) we could
> also
> > make it
> > > > optional.
> > > >
> > > >
> > > > Cheers
> > > >   Vincenzo
> > > >
> > >
> > > I think it's too early to say whether this patch is benefitial for
> > > netmap, too.  It looks like something that trades off latency
> > > for throughput, and this is a decision the endpoint (VM) should
> > > make, not the network (host).
> > > So you should measure with offloads on before you make conclusions
> about
> > it.
> >
> > Just to check my understanding, we're talking about the following
> kind
> > of batching:
> >
> >   int num_packets = peek_available_packets(device);
> >   while (num_packets-- > 0) {
> >   int flags = MORE;
> >   if (num_packets == 0) {
> >   flags = NONE;
> >   }
> >   qemu_net_send_packet(..., flags);
> >   }
> >
> > In other words, this only batches up a single burst of packets.  It
> > doesn't introduce timers or blocking calls.
> >
> > So the effect of batching should be relatively small on latency.  In
> > fact, it's almost like sendmmsg(2)/recvmmsg(2) but using a
> > one-packet-at-a-time interface.
> >
> > Does this sound right?
> >
> > Stefan
> >
> >
> > Totally correct.
> >
> > In reply to Michael:
> >- what you say is right with netmap used as a backend with typical TCP
> > applications in the guests, and we have already an implementation that
> supports
> > those offloadings
> >
> >- however, consider that the main use of netmap is fast packet
> processing in
> > middleboxes, where packet aggregation is not always possible.
> Applications that
> > use netmap **in the guest** typically use "packet batching" (i.e. send
> multiple
> > packets with one system call), so batches originate in the guest.
> Without the
> > MORE flag, those batches are split at the frontend-backend interface.
> This is
> > just a different workload.
> >
> >
> > Regards,
> > --
> > Vincenzo Maffione
>
> Considering that you have measured performance regression under
> netperf, I don't understand why do we keep arguing
> about theory. Increasing latency is a problem and if it can already be
> seen with netperf it will only get worse with real life workloads.
>
> So my advice is, start by merging offload support for netmap, then check
> whether this optimization adds enough performance to be worth it, if yes
> it needs more heuristics to avoid hurting latency.
>
> --
> MST
>



-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH] net: QEMU_NET_PACKET_FLAG_MORE introduced

[Vincenzo Maffione]

2013/12/9 Stefan Hajnoczi 

> On Mon, Dec 09, 2013 at 01:14:31PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Dec 09, 2013 at 11:55:57AM +0100, Vincenzo Maffione wrote:
> > > If you don't think adding the new flag support for virtio-net is a
> good idea
> > > (though TAP performance is not affected in every case) we could also
> make it
> > > optional.
> > >
> > >
> > > Cheers
> > >   Vincenzo
> > >
> >
> > I think it's too early to say whether this patch is benefitial for
> > netmap, too.  It looks like something that trades off latency
> > for throughput, and this is a decision the endpoint (VM) should
> > make, not the network (host).
> > So you should measure with offloads on before you make conclusions about
> it.
>
> Just to check my understanding, we're talking about the following kind
> of batching:
>
>   int num_packets = peek_available_packets(device);
>   while (num_packets-- > 0) {
>   int flags = MORE;
>   if (num_packets == 0) {
>   flags = NONE;
>   }
>   qemu_net_send_packet(..., flags);
>   }
>
> In other words, this only batches up a single burst of packets.  It
> doesn't introduce timers or blocking calls.
>
> So the effect of batching should be relatively small on latency.  In
> fact, it's almost like sendmmsg(2)/recvmmsg(2) but using a
> one-packet-at-a-time interface.
>
> Does this sound right?
>
> Stefan
>

Totally correct.

In reply to Michael:
   - what you say is right with netmap used as a backend with typical TCP
applications in the guests, and we have already an implementation that
supports those offloadings

   - however, consider that the main use of netmap is fast packet
processing in middleboxes, where packet aggregation is not always possible.
Applications that use netmap **in the guest** typically use "packet
batching" (i.e. send multiple packets with one system call), so batches
originate in the guest. Without the MORE flag, those batches are split at
the frontend-backend interface. This is just a different workload.


Regards,
-- 
Vincenzo Maffione

Re: [Qemu-devel] [PATCH] net: QEMU_NET_PACKET_FLAG_MORE introduced

[Vincenzo Maffione]

I totally agree with you, and we will propose a patch to make this possible.

However, none of the offloadings you mentioned helps with packet rate
throughput (checksum offload doesn't really help with short packets), which
is the main purpose of this patch. High packet rates (say 1-5 Mpps) are
interesting for people who want to use VMs as middleboxes. These packet
rates (and up to 20+ Mpps) are possible with netmap if proper batching is
supported.

If you don't think adding the new flag support for virtio-net is a good
idea (though TAP performance is not affected in every case) we could also
make it optional.


Cheers
  Vincenzo


2013/12/9 Michael S. Tsirkin 

> On Mon, Dec 09, 2013 at 11:20:29AM +0100, Vincenzo Maffione wrote:
> > Hello,
> >I've done some netperf TCP_STREAM and TCP_RR virtio-net tests, using
> the
> > same configuration.
> > Here are the results
> >
> > ## netperf TCP_STREAM ###
> > NO BATCHING BATCHING
> > 1  5.5 Gbps 3.8 Gbps
> > 2  5.4 Gbps 5.5 Gbps
> > 3  5.2 Gbps 5.2 Gbps
> > 4  5.1 Gbps 5.0 Gbps
> > 10 5.4 Gbps 5.2 Gbps
> > 20 5.4 Gbps 5.4 Gbps
> >
> >
> >  netperf TCP_RR #
> > NO BATCHING BATCHING
> > 1 13.0 Ktts 12.8 Ktts
> > 2 23.8 Ktts 23.0 Ktts
> > 3 34.0 Ktts 32.5 Ktts
> > 4 44.5 Ktts 41.0 Ktts
> > 1097.0 Ktts 93.0 Ktts
> > 15   122.0 Ktts120.0 Ktts
> > 20   125.0 Ktts128.0 Ktts
> > 25   128.0 Ktts130.0 Ktts
> >
> >
> > There is some negative effects introduced by batching.
> > Also consider that
> >- Since TAP backend doesn't use the new flag, this patch doesn't
> change the
> > performance when the TAP backend is used.
> >- I've not submitted yet the patch for virtio_net_header support, and
> > therefore the TCP_STREAM performance with NETMAP backend is now not
> >  comparable to the performance with TAP backend, because we are
> limited to
> > 1.5KB packets.
> >
> > Cheers,
> >   Vincenzo
>
> Ah, so no GSO/UFO/checksum offload then?
> In that case maybe it's a good idea to start with supporting
> that in your backend. This does batching within the guest so
> extra host side batching with all the tradeoffs it involves
> might not be necessary.
>
> Guest network stack behaviour with and without offloads is
> different to such a degree that it's not clear optimizing
> one is not pessimizing the other.
>
> --
> MST
>



-- 
Vincenzo Maffione

[Qemu-devel] [PATCH] net: QEMU_NET_PACKET_FLAG_MORE introduced

[Vincenzo Maffione]

This patch extends the frontend-backend interface so that it is possible
to pass a new flag (QEMU_NET_PACKET_FLAG_MORE) when sending a packet to the
other peer. The new flag acts as a hint for the receiving peer, which can
accumulate a batch of packets before forwarding those packets (to the host
if the receiving peer is the backend or to the guest if the receiving peer
is the frontend).

The patch also implements a batching mechanism for the netmap backend (on the
backend receive side) and for the e1000 and virtio frontends (on the frontend
transmit side).

Measured improvement of a guest-to-guest UDP_STREAM netperf test (64 bytes
packets) with virtio-net frontends:
820 Kpps ==> 1000 Kpps (+22%).

Measured improvement of a guest-to-guest UDP test (64 bytes packets) with
e1000 frontends and netmap clients on the guests:
1.8 Mpps ==> 3.1 Mpps (+72%).

Signed-off-by: Vincenzo Maffione 
---
Experiment details:
- Processor: Intel i7-3770K CPU @ 3.50GHz (8 cores)
- Memory @ 1333 MHz
- Host O.S.: Archlinux with Linux 3.11
- Guest O.S.: Archlinux with Linux 3.11

QEMU command line for the virtio experiment:
qemu-system-x86_64 archdisk.qcow -snapshot -enable-kvm -device 
virtio-net-pci,ioeventfd=on,mac=00:AA:BB:CC:DD:01,netdev=mynet -netdev 
netmap,ifname=vale0:01,id=mynet -smp 2 -vga std -m 3G

QEMU command line for the e1000 experiment:
qemu-system-x86_64 archdisk.qcow -snapshot -enable-kvm -device 
e1000,mitigation=off,mac=00:AA:BB:CC:DD:01,netdev=mynet -netdev 
netmap,ifname=vale0:01,id=mynet -smp 2 -vga std -m 3G

With the e1000 experiments, we don't use netperf on the guests, but netmap 
clients (pkt-gen)
that run directly on the e1000 adapter, bypassing the O.S. stack.

Other things:
- This patch is against the net-next tree 
(https://github.com/stefanha/qemu.git)
  because the first netmap patch is not in the qemu master (AFAIK).
- The batching can also be implemented on the backend transmit side and 
frontend
  receive side. We could do it in the future.

 hw/net/cadence_gem.c|  3 ++-
 hw/net/dp8393x.c|  5 +++--
 hw/net/e1000.c  | 21 -
 hw/net/eepro100.c   |  5 +++--
 hw/net/etraxfs_eth.c|  5 +++--
 hw/net/lan9118.c|  2 +-
 hw/net/mcf_fec.c|  5 +++--
 hw/net/mipsnet.c|  6 --
 hw/net/ne2000.c |  5 +++--
 hw/net/ne2000.h |  3 ++-
 hw/net/opencores_eth.c  |  2 +-
 hw/net/pcnet.c  |  8 +---
 hw/net/pcnet.h  |  3 ++-
 hw/net/rtl8139.c|  7 ---
 hw/net/smc91c111.c  |  5 +++--
 hw/net/spapr_llan.c |  2 +-
 hw/net/stellaris_enet.c |  3 ++-
 hw/net/virtio-net.c | 10 --
 hw/net/vmxnet3.c|  3 ++-
 hw/net/vmxnet_tx_pkt.c  |  4 ++--
 hw/net/xgmac.c  |  2 +-
 hw/net/xilinx_axienet.c |  2 +-
 hw/usb/dev-network.c|  8 +---
 include/net/net.h   | 20 +---
 include/net/queue.h |  1 +
 net/dump.c  |  3 ++-
 net/hub.c   | 10 ++
 net/net.c   | 39 +++
 net/netmap.c| 17 -
 net/slirp.c |  5 +++--
 net/socket.c| 10 ++
 net/tap-win32.c |  2 +-
 net/tap.c   | 12 +++-
 net/vde.c   |  5 +++--
 savevm.c|  2 +-
 35 files changed, 155 insertions(+), 90 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 4a355bb..432687a 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -583,7 +583,8 @@ static int gem_mac_address_filter(GemState *s, const 
uint8_t *packet)
  * gem_receive:
  * Fit a packet handed to us by QEMU into the receive descriptor ring.
  */
-static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
+static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size,
+   unsigned flags)
 {
 unsigneddesc[2];
 hwaddr packet_desc_addr, last_desc_addr;
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 789d385..d8c7da8 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -415,7 +415,7 @@ static void do_transmit_packets(dp8393xState *s)
 }
 } else {
 /* Transmit packet */
-qemu_send_packet(nc, s->tx_buffer, tx_len);
+qemu_send_packet(nc, s->tx_buffer, tx_len, 0);
 }
 s->regs[SONIC_TCR] |= SONIC_TCR_PTX;
 
@@ -723,7 +723,8 @@ static int receive_filter(dp8393xState *s, const uint8_t * 
buf, int size)
 return -1;
 }
 
-static ssize_t nic_receive(NetClientState *nc, const uint8_t * buf, size_t 
size)
+static ssize_t nic_receive(NetClientState *nc, const uint8_t * buf,
+   size_t size, unsigned flags)
 {
 dp8393xState *s = qemu_get_nic_opaque(nc);
 uint16_t data[10];
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ae63591..5294ec5 100644
--- a/hw/net/e1000.c
+++ b/hw/ne

[Qemu-devel] [PATCH] MAINTAINERS: Add netmap maintainers

[Vincenzo Maffione]

Signed-off-by: Vincenzo Maffione 
---
 MAINTAINERS | 8 
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 77edacf..7295f33 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -707,6 +707,14 @@ S: Maintained
 F: net/
 T: git git://github.com/stefanha/qemu.git net
 
+Netmap network backend
+M: Luigi Rizzo 
+M: Giuseppe Lettieri 
+M: Vincenzo Maffione 
+W: http://info.iet.unipi.it/~luigi/netmap/
+S: Maintained
+F: net/netmap.c
+
 Network Block Device (NBD)
 M: Paolo Bonzini 
 S: Odd Fixes
-- 
1.8.4.2