Re: [PATCHv7] add mergeable buffers support to vhost_net

2010-05-10 Thread Michael S. Tsirkin 
On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
 @@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
   use_mm(net-dev.mm);
   mutex_lock(vq-mutex);
   vhost_disable_notify(vq);
 - hdr_size = vq-hdr_size;
 + vhost_hlen = vq-vhost_hlen;
  
   vq_log = unlikely(vhost_has_feature(net-dev, VHOST_F_LOG_ALL)) ?
   vq-log : NULL;
  
 - for (;;) {
 - head = vhost_get_vq_desc(net-dev, vq, vq-iov,
 -  ARRAY_SIZE(vq-iov),
 -  out, in,
 -  vq_log, log);
 + while ((datalen = vhost_head_len(vq, sock-sk))) {
 + headcount = vhost_get_desc_n(vq, vq-heads,
 +  datalen + vhost_hlen,
 +  in, vq_log, log);
 + if (headcount  0)
 + break;
   /* OK, now we need to know about added descriptors. */
 - if (head == vq-num) {
 + if (!headcount) {
   if (unlikely(vhost_enable_notify(vq))) {
   /* They have slipped one in as we were
* doing that: check again. */
 @@ -241,46 +272,53 @@ static void handle_rx(struct vhost_net *
   break;
   }
   /* We don't need to be notified again. */
 - if (out) {
 - vq_err(vq, Unexpected descriptor format for RX: 
 -out %d, int %d\n,
 -out, in);
 - break;
 - }
 - /* Skip header. TODO: support TSO/mergeable rx buffers. */
 - s = move_iovec_hdr(vq-iov, vq-hdr, hdr_size, in);
 + if (vhost_hlen)
 + /* Skip header. TODO: support TSO. */
 + s = move_iovec_hdr(vq-iov, vq-hdr, vhost_hlen, in);
 + else
 + s = copy_iovec_hdr(vq-iov, vq-hdr, vq-sock_hlen, in);
   msg.msg_iovlen = in;
   len = iov_length(vq-iov, in);
   /* Sanity check */
   if (!len) {
   vq_err(vq, Unexpected header len for RX: 
  %zd expected %zd\n,
 -iov_length(vq-hdr, s), hdr_size);
 +iov_length(vq-hdr, s), vhost_hlen);
   break;
   }
   err = sock-ops-recvmsg(NULL, sock, msg,
len, MSG_DONTWAIT | MSG_TRUNC);
   /* TODO: Check specific error and bomb out unless EAGAIN? */
   if (err  0) {
 - vhost_discard_vq_desc(vq);
 + vhost_discard_desc(vq, headcount);
   break;
   }
 - /* TODO: Should check and handle checksum. */
 - if (err  len) {
 - pr_err(Discarded truncated rx packet: 
 - len %d  %zd\n, err, len);
 - vhost_discard_vq_desc(vq);
 + if (err != datalen) {
 + pr_err(Discarded rx packet: 
 + len %d, expected %zd\n, err, datalen);
 + vhost_discard_desc(vq, headcount);
   continue;
   }
   len = err;
 - err = memcpy_toiovec(vq-hdr, (unsigned char *)hdr, hdr_size);
 - if (err) {
 - vq_err(vq, Unable to write vnet_hdr at addr %p: %d\n,
 -vq-iov-iov_base, err);
 + if (vhost_hlen 
 + memcpy_toiovecend(vq-hdr, (unsigned char *)hdr, 0,
 +   vhost_hlen)) {
 + vq_err(vq, Unable to write vnet_hdr at addr %p\n,
 +vq-iov-iov_base);
   break;
   }
 - len += hdr_size;
 - vhost_add_used_and_signal(net-dev, vq, head, len);
 + /* TODO: Should check and handle checksum. */
 + if (vhost_has_feature(net-dev, VIRTIO_NET_F_MRG_RXBUF) 
 + memcpy_toiovecend(vq-hdr, (unsigned char *)headcount,
 +   offsetof(typeof(hdr), num_buffers),
 +   sizeof(hdr.num_buffers))) {
 + vq_err(vq, Failed num_buffers write);
 + vhost_discard_desc(vq, headcount);
 + break;
 + }
 + len += vhost_hlen;
 + vhost_add_used_and_signal_n(net-dev, vq, vq-heads,
 + headcount);
   if (unlikely(vq_log))
   vhost_log_write(vq, vq_log, log, len);
   total_len += len;

OK I think I see the bug here: vhost_add_used_and_signal_n
does not get the actual length, it

Re: [PATCHv7] add mergeable buffers support to vhost_net

2010-05-10 Thread David Stevens 
Since datalen carries the difference and will be negative by that amount
from the original loop, what about just adding something like:

}
if (headcount)
heads[headcount-1].len += datalen;
[and really, headcount 0 since datalen  0, so just:

heads[headcount-1].len += datalen;

+-DLS


kvm-ow...@vger.kernel.org wrote on 05/10/2010 09:43:03 AM:

 On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
  @@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
  use_mm(net-dev.mm);
  mutex_lock(vq-mutex);
  vhost_disable_notify(vq);
  -   hdr_size = vq-hdr_size;
  +   vhost_hlen = vq-vhost_hlen;
  
  vq_log = unlikely(vhost_has_feature(net-dev, VHOST_F_LOG_ALL)) ?
 vq-log : NULL;
  
  -   for (;;) {
  -  head = vhost_get_vq_desc(net-dev, vq, vq-iov,
  -ARRAY_SIZE(vq-iov),
  -out, in,
  -vq_log, log);
  +   while ((datalen = vhost_head_len(vq, sock-sk))) {
  +  headcount = vhost_get_desc_n(vq, vq-heads,
  +datalen + vhost_hlen,
  +in, vq_log, log);
  +  if (headcount  0)
  + break;
 /* OK, now we need to know about added descriptors. */
  -  if (head == vq-num) {
  +  if (!headcount) {
if (unlikely(vhost_enable_notify(vq))) {
   /* They have slipped one in as we were
* doing that: check again. */
  @@ -241,46 +272,53 @@ static void handle_rx(struct vhost_net *
break;
 }
 /* We don't need to be notified again. */
  -  if (out) {
  - vq_err(vq, Unexpected descriptor format for RX: 
  -out %d, int %d\n,
  -out, in);
  - break;
  -  }
  -  /* Skip header. TODO: support TSO/mergeable rx buffers. */
  -  s = move_iovec_hdr(vq-iov, vq-hdr, hdr_size, in);
  +  if (vhost_hlen)
  + /* Skip header. TODO: support TSO. */
  + s = move_iovec_hdr(vq-iov, vq-hdr, vhost_hlen, in);
  +  else
  + s = copy_iovec_hdr(vq-iov, vq-hdr, vq-sock_hlen, in);
 msg.msg_iovlen = in;
 len = iov_length(vq-iov, in);
 /* Sanity check */
 if (!len) {
vq_err(vq, Unexpected header len for RX: 
   %zd expected %zd\n,
  -iov_length(vq-hdr, s), hdr_size);
  +iov_length(vq-hdr, s), vhost_hlen);
break;
 }
 err = sock-ops-recvmsg(NULL, sock, msg,
   len, MSG_DONTWAIT | MSG_TRUNC);
 /* TODO: Check specific error and bomb out unless EAGAIN? */
 if (err  0) {
  - vhost_discard_vq_desc(vq);
  + vhost_discard_desc(vq, headcount);
break;
 }
  -  /* TODO: Should check and handle checksum. */
  -  if (err  len) {
  - pr_err(Discarded truncated rx packet: 
  - len %d  %zd\n, err, len);
  - vhost_discard_vq_desc(vq);
  +  if (err != datalen) {
  + pr_err(Discarded rx packet: 
  + len %d, expected %zd\n, err, datalen);
  + vhost_discard_desc(vq, headcount);
continue;
 }
 len = err;
  -  err = memcpy_toiovec(vq-hdr, (unsigned char *)hdr, hdr_size);
  -  if (err) {
  - vq_err(vq, Unable to write vnet_hdr at addr %p: %d\n,
  -vq-iov-iov_base, err);
  +  if (vhost_hlen 
  +  memcpy_toiovecend(vq-hdr, (unsigned char *)hdr, 0,
  +  vhost_hlen)) {
  + vq_err(vq, Unable to write vnet_hdr at addr %p\n,
  +vq-iov-iov_base);
break;
 }
  -  len += hdr_size;
  -  vhost_add_used_and_signal(net-dev, vq, head, len);
  +  /* TODO: Should check and handle checksum. */
  +  if (vhost_has_feature(net-dev, VIRTIO_NET_F_MRG_RXBUF) 
  +  memcpy_toiovecend(vq-hdr, (unsigned char *)headcount,
  +  offsetof(typeof(hdr), num_buffers),
  +  sizeof(hdr.num_buffers))) {
  + vq_err(vq, Failed num_buffers write);
  + vhost_discard_desc(vq, headcount);
  + break;
  +  }
  +  len += vhost_hlen;
  +  vhost_add_used_and_signal_n(net-dev, vq, vq-heads,
  +   headcount);
 if (unlikely(vq_log))
vhost_log_write(vq, vq_log, log, len);
 total_len += len;
 
 OK I think I see the bug here: vhost_add_used_and_signal_n
 does not get the actual length, it gets the iovec length from vhost.
 Guest virtio uses this as packet length, with bad results.
 
 So I have applied the follows and it seems to have fixed the problem:
 
 diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
 index c16db02..9d7496d 100644
 --- a/drivers/vhost/net.c
 +++ b/drivers/vhost/net.c
 @@ -219,7 +219,7 @@ static int peek_head_len(struct vhost_virtqueue *vq, 

 struct sock *sk)
  /* This is a multi-buffer

Re: [PATCHv7] add mergeable buffers support to vhost_net

2010-05-10 Thread Michael S. Tsirkin 
On Mon, May 10, 2010 at 10:09:03AM -0700, David Stevens wrote:
 Since datalen carries the difference and will be negative by that amount
 from the original loop, what about just adding something like:
 
 }
 if (headcount)
 heads[headcount-1].len += datalen;
 [and really, headcount 0 since datalen  0, so just:
 
 heads[headcount-1].len += datalen;
 
 +-DLS

This works too, just does more checks and comparisons.
I am still surprised that you were unable to reproduce the problem.

 
 kvm-ow...@vger.kernel.org wrote on 05/10/2010 09:43:03 AM:
 
  On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
   @@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
   use_mm(net-dev.mm);
   mutex_lock(vq-mutex);
   vhost_disable_notify(vq);
   -   hdr_size = vq-hdr_size;
   +   vhost_hlen = vq-vhost_hlen;
   
   vq_log = unlikely(vhost_has_feature(net-dev, VHOST_F_LOG_ALL)) ?
  vq-log : NULL;
   
   -   for (;;) {
   -  head = vhost_get_vq_desc(net-dev, vq, vq-iov,
   -ARRAY_SIZE(vq-iov),
   -out, in,
   -vq_log, log);
   +   while ((datalen = vhost_head_len(vq, sock-sk))) {
   +  headcount = vhost_get_desc_n(vq, vq-heads,
   +datalen + vhost_hlen,
   +in, vq_log, log);
   +  if (headcount  0)
   + break;
  /* OK, now we need to know about added descriptors. */
   -  if (head == vq-num) {
   +  if (!headcount) {
 if (unlikely(vhost_enable_notify(vq))) {
/* They have slipped one in as we were
 * doing that: check again. */
   @@ -241,46 +272,53 @@ static void handle_rx(struct vhost_net *
 break;
  }
  /* We don't need to be notified again. */
   -  if (out) {
   - vq_err(vq, Unexpected descriptor format for RX: 
   -out %d, int %d\n,
   -out, in);
   - break;
   -  }
   -  /* Skip header. TODO: support TSO/mergeable rx buffers. */
   -  s = move_iovec_hdr(vq-iov, vq-hdr, hdr_size, in);
   +  if (vhost_hlen)
   + /* Skip header. TODO: support TSO. */
   + s = move_iovec_hdr(vq-iov, vq-hdr, vhost_hlen, in);
   +  else
   + s = copy_iovec_hdr(vq-iov, vq-hdr, vq-sock_hlen, in);
  msg.msg_iovlen = in;
  len = iov_length(vq-iov, in);
  /* Sanity check */
  if (!len) {
 vq_err(vq, Unexpected header len for RX: 
%zd expected %zd\n,
   -iov_length(vq-hdr, s), hdr_size);
   +iov_length(vq-hdr, s), vhost_hlen);
 break;
  }
  err = sock-ops-recvmsg(NULL, sock, msg,
len, MSG_DONTWAIT | MSG_TRUNC);
  /* TODO: Check specific error and bomb out unless EAGAIN? */
  if (err  0) {
   - vhost_discard_vq_desc(vq);
   + vhost_discard_desc(vq, headcount);
 break;
  }
   -  /* TODO: Should check and handle checksum. */
   -  if (err  len) {
   - pr_err(Discarded truncated rx packet: 
   - len %d  %zd\n, err, len);
   - vhost_discard_vq_desc(vq);
   +  if (err != datalen) {
   + pr_err(Discarded rx packet: 
   + len %d, expected %zd\n, err, datalen);
   + vhost_discard_desc(vq, headcount);
 continue;
  }
  len = err;
   -  err = memcpy_toiovec(vq-hdr, (unsigned char *)hdr, hdr_size);
   -  if (err) {
   - vq_err(vq, Unable to write vnet_hdr at addr %p: %d\n,
   -vq-iov-iov_base, err);
   +  if (vhost_hlen 
   +  memcpy_toiovecend(vq-hdr, (unsigned char *)hdr, 0,
   +  vhost_hlen)) {
   + vq_err(vq, Unable to write vnet_hdr at addr %p\n,
   +vq-iov-iov_base);
 break;
  }
   -  len += hdr_size;
   -  vhost_add_used_and_signal(net-dev, vq, head, len);
   +  /* TODO: Should check and handle checksum. */
   +  if (vhost_has_feature(net-dev, VIRTIO_NET_F_MRG_RXBUF) 
   +  memcpy_toiovecend(vq-hdr, (unsigned char *)headcount,
   +  offsetof(typeof(hdr), num_buffers),
   +  sizeof(hdr.num_buffers))) {
   + vq_err(vq, Failed num_buffers write);
   + vhost_discard_desc(vq, headcount);
   + break;
   +  }
   +  len += vhost_hlen;
   +  vhost_add_used_and_signal_n(net-dev, vq, vq-heads,
   +   headcount);
  if (unlikely(vq_log))
 vhost_log_write(vq, vq_log, log, len);
  total_len += len;
  
  OK I think I see the bug here: vhost_add_used_and_signal_n
  does not get the actual length, it gets the iovec length from vhost.
  Guest virtio uses this as packet length, with bad results.
  
  So I have applied the

Re: [PATCHv7] add mergeable buffers support to vhost_net

2010-05-10 Thread Michael S. Tsirkin 
On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
 @@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
   use_mm(net-dev.mm);
   mutex_lock(vq-mutex);
   vhost_disable_notify(vq);
 - hdr_size = vq-hdr_size;
 + vhost_hlen = vq-vhost_hlen;
  
   vq_log = unlikely(vhost_has_feature(net-dev, VHOST_F_LOG_ALL)) ?
   vq-log : NULL;
  
 - for (;;) {
 - head = vhost_get_vq_desc(net-dev, vq, vq-iov,
 -  ARRAY_SIZE(vq-iov),
 -  out, in,
 -  vq_log, log);
 + while ((datalen = vhost_head_len(vq, sock-sk))) {
 + headcount = vhost_get_desc_n(vq, vq-heads,
 +  datalen + vhost_hlen,
 +  in, vq_log, log);
 + if (headcount  0)
 + break;
   /* OK, now we need to know about added descriptors. */
 - if (head == vq-num) {
 + if (!headcount) {
   if (unlikely(vhost_enable_notify(vq))) {
   /* They have slipped one in as we were
* doing that: check again. */


So I think this breaks handling for a failure mode where
we get an skb that is larger than the max packet guest
can get. The right thing to do in this case is to
drop the skb, we currently do this by passing
truncate flag to recvmsg.

In particular, with mergeable buffers off, if we get an skb
that does not fit in a single packet, this code will
spread it over multiple buffers.

You should be able to reproduce this fairly easily
by disabling both indirect buffers and mergeable buffers
on qemu command line. With current code TCP still
works by falling back on small packets. I think
with your code it will get stuck forever once
we get an skb that is too large for us to handle.

-- 
MST


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7] add mergeable buffers support to vhost_net

2010-05-10 Thread David Stevens 
netdev-ow...@vger.kernel.org wrote on 05/10/2010 10:25:57 AM:

 On Mon, May 10, 2010 at 10:09:03AM -0700, David Stevens wrote:
  Since datalen carries the difference and will be negative by that 
amount
  from the original loop, what about just adding something like:
  
  }
  if (headcount)
  heads[headcount-1].len += datalen;
  [and really, headcount 0 since datalen  0, so just:
  
  heads[headcount-1].len += datalen;
  
  +-DLS
 
 This works too, just does more checks and comparisons.
 I am still surprised that you were unable to reproduce the problem.
 

I'm sure it happened, and probably had a performance
penalty on my systems too, but not as much as yours.
I didn't see any obvious performance difference running
with the patch, though; not sure why. I'll instrument to
see how often it's happening, I think.
But fixed now, good catch!

+-DLS

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7] add mergeable buffers support to vhost_net

2010-05-03 Thread Michael S. Tsirkin 
On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
 This patch adds mergeable receive buffer support to vhost_net.
 
 Signed-off-by: David L Stevens dlstev...@us.ibm.com

I've been doing some more testing before sending out a pull
request, and I see a drastic performance degradation in guest to host
traffic when this is applied but mergeable buffers are not in used
by userspace (existing qemu-kvm userspace).

This is both with and without my patch on top.

Without patch:
[...@tuck ~]$ sh runtest  21 | tee 
ser-meregeable-disabled-kernel-only-tun-only.log
Starting netserver at port 12865
set_up_server could not establish a listen endpoint for  port 12865 with family 
AF_UNSPEC
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 11.0.0.4 (11.0.0.4) 
port 0 AF_INET : demo
Recv   SendSend  Utilization   Service Demand
Socket Socket  Message  Elapsed  Send Recv SendRecv
Size   SizeSize Time Throughput  localremote   local   remote
bytes  bytes   bytessecs.10^6bits/s  % S  % S  us/KB   us/KB

 87380  16384  1638410.00  9107.26   89.2033.850.802   2.436  

With patch:
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 11.0.0.4 (11.0.0.4) 
port 0 AF_INET : demo
Recv   SendSend  Utilization   Service Demand
Socket Socket  Message  Elapsed  Send Recv SendRecv
Size   SizeSize Time Throughput  localremote   local   remote
bytes  bytes   bytessecs.10^6bits/s  % S  % S  us/KB   us/KB

 87380  16384  1638410.0035.00   2.21 0.62 5.181   11.575 


For ease of testing, I put this on my tree
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-broken

Please take a look.
Thanks!

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7] add mergeable buffers support to vhost_net

2010-05-03 Thread David Stevens 
Michael S. Tsirkin m...@redhat.com wrote on 05/03/2010 03:34:11 AM:

 On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
  This patch adds mergeable receive buffer support to vhost_net.
  
  Signed-off-by: David L Stevens dlstev...@us.ibm.com
 
 I've been doing some more testing before sending out a pull
 request, and I see a drastic performance degradation in guest to host
 traffic when this is applied but mergeable buffers are not in used
 by userspace (existing qemu-kvm userspace).

Actually, I wouldn't expect it to work at all; the qemu-kvm
patch (particularly the feature bit setting bug fix) is required.
Without it, I think the existing code will tell the guest to use
mergeable buffers while turning it off in vhost. That was completely
non-functional for me -- what version of qemu-kvm are you using?
What I did to test w/o mergeable buffers is turn off the
bit in VHOST_FEATURES. I'll recheck these, but qemu-kvm definitely
must be updated; the original doesn't correctly handle feature bits.

+-DLS

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7] add mergeable buffers support to vhost_net

2010-05-03 Thread Michael S. Tsirkin 
On Mon, May 03, 2010 at 08:39:08AM -0700, David Stevens wrote:
 Michael S. Tsirkin m...@redhat.com wrote on 05/03/2010 03:34:11 AM:
 
  On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
   This patch adds mergeable receive buffer support to vhost_net.
   
   Signed-off-by: David L Stevens dlstev...@us.ibm.com
  
  I've been doing some more testing before sending out a pull
  request, and I see a drastic performance degradation in guest to host
  traffic when this is applied but mergeable buffers are not in used
  by userspace (existing qemu-kvm userspace).
 
 Actually, I wouldn't expect it to work at all;
 the qemu-kvm
 patch (particularly the feature bit setting bug fix) is required.

Which bugfix is that?

 Without it, I think the existing code will tell the guest to use
 mergeable buffers while turning it off in vhost.

It should not do that, specifically
vhost_net_get_features does:
features = ~(1  VIRTIO_NET_F_MRG_RXBUF);
unconditionally. This was added with:
5751995a20e77cd9d61d00f7390401895fa172a6

I forced mergeable buffers off with -global virtio-net-pci.mrg_rxbuf=off
and it did not seem to help either.

 That was completely
 non-functional for me -- what version of qemu-kvm are you using?

992cc816c42f2e93db033919a9ddbfcd1da4 or later should work well
AFAIK.

 What I did to test w/o mergeable buffers is turn off the
 bit in VHOST_FEATURES.

It should be enough to force mergeable buffers to off by qemu command
line: -global virtio-net-pci.mrg_rxbuf=off 

 I'll recheck these, but qemu-kvm definitely
 must be updated; the original doesn't correctly handle feature bits.
 
 +-DLS

Hmm, I don't see the bug.

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7] add mergeable buffers support to vhost_net

2010-05-03 Thread David Stevens 
Michael S. Tsirkin m...@redhat.com wrote on 05/03/2010 08:56:14 AM:

 On Mon, May 03, 2010 at 08:39:08AM -0700, David Stevens wrote:
  Michael S. Tsirkin m...@redhat.com wrote on 05/03/2010 03:34:11 AM:
  
   On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
This patch adds mergeable receive buffer support to vhost_net.

Signed-off-by: David L Stevens dlstev...@us.ibm.com
   
   I've been doing some more testing before sending out a pull
   request, and I see a drastic performance degradation in guest to 
host
   traffic when this is applied but mergeable buffers are not in used
   by userspace (existing qemu-kvm userspace).
  
  Actually, I wouldn't expect it to work at all;
  the qemu-kvm
  patch (particularly the feature bit setting bug fix) is required.
 
 Which bugfix is that?

Actually, I see you put that upstream already--
commit dc14a397812b91dd0d48b03d1b8f66a251542369  in
Avi's tree is the one I was talking about.
I'll look further.

+-DLS

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7] add mergeable buffers support to vhost_net

2010-04-30 Thread David Stevens 
Michael S. Tsirkin m...@redhat.com wrote on 04/29/2010 06:45:15 AM:

 On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
  This patch adds mergeable receive buffer support to vhost_net.
  
  Signed-off-by: David L Stevens dlstev...@us.ibm.com
 
 I have applied this, thanks very much!
 I have also applied some tweaks on top,
 please take a look.
 
 Thanks,
 MSt
 

Looks fine to me.

Acked-by: David L Stevens dlstev...@us.ibm.com

 commit 2809e94f5f26d89dc5232aaec753ffda95c4d95e
 Author: Michael S. Tsirkin m...@redhat.com
 Date:   Thu Apr 29 16:18:08 2010 +0300
 
 vhost-net: minor tweaks in mergeable buffer code
 
 Applies the following tweaks on top of mergeable buffers patch:
 1. vhost_get_desc_n assumes that all desriptors are 'in' only.
It's also unlikely to be useful for any vhost frontend
besides vhost_net, so just move it to net.c, and rename
get_rx_bufs for brevity.
 
 2. for rx, we called iov_length within vhost_get_desc_n
(now get_rx_bufs) already, so we don't
need an extra call to iov_length to avoid overflow anymore.
Accordingly, copy_iovec_hdr can return void now.
 
 3. for rx, do some further code tweaks:
do not assign len = err as we check that err == len
handle data length in a way similar to how we handle
header length: datalen - sock_len, len - vhost_len.
add sock_hlen as a local variable, for symmetry with vhost_hlen.
 
 4. add some likely/unlikely annotations
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 
 diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
 index d61d945..85519b4 100644
 --- a/drivers/vhost/net.c
 +++ b/drivers/vhost/net.c
 @@ -74,9 +74,9 @@ static int move_iovec_hdr(struct iovec *from, struct 
iovec *to,
 }
 return seg;
  }
 -/* Copy iovec entries for len bytes from iovec. Return segments used. 
*/
 -static int copy_iovec_hdr(const struct iovec *from, struct iovec *to,
 -   size_t len, int iovcount)
 +/* Copy iovec entries for len bytes from iovec. */
 +static void copy_iovec_hdr(const struct iovec *from, struct iovec *to,
 +size_t len, int iovcount)
  {
 int seg = 0;
 size_t size;
 @@ -89,7 +89,6 @@ static int copy_iovec_hdr(const struct iovec *from, 
struct iovec *to,
++to;
++seg;
 }
 -   return seg;
  }
 
  /* Caller must have TX VQ lock */
 @@ -204,7 +203,7 @@ static void handle_tx(struct vhost_net *net)
 unuse_mm(net-dev.mm);
  }
 
 -static int vhost_head_len(struct vhost_virtqueue *vq, struct sock *sk)
 +static int peek_head_len(struct vhost_virtqueue *vq, struct sock *sk)
  {
 struct sk_buff *head;
 int len = 0;
 @@ -212,17 +211,70 @@ static int vhost_head_len(struct vhost_virtqueue 
*vq, 
 struct sock *sk)
 lock_sock(sk);
 head = skb_peek(sk-sk_receive_queue);
 if (head)
 -  len = head-len + vq-sock_hlen;
 +  len = head-len;
 release_sock(sk);
 return len;
  }
 
 +/* This is a multi-buffer version of vhost_get_desc, that works if
 + *   vq has read descriptors only.
 + * @vq  - the relevant virtqueue
 + * @datalen   - data length we'll be reading
 + * @iovcount   - returned count of io vectors we fill
 + * @log  - vhost log
 + * @log_num   - log offset
 + *   returns number of buffer heads allocated, negative on error
 + */
 +static int get_rx_bufs(struct vhost_virtqueue *vq,
 + struct vring_used_elem *heads,
 + int datalen,
 + unsigned *iovcount,
 + struct vhost_log *log,
 + unsigned *log_num)
 +{
 +   unsigned int out, in;
 +   int seg = 0;
 +   int headcount = 0;
 +   unsigned d;
 +   int r;
 +
 +   while (datalen  0) {
 +  if (unlikely(headcount = VHOST_NET_MAX_SG)) {
 + r = -ENOBUFS;
 + goto err;
 +  }
 +  d = vhost_get_desc(vq-dev, vq, vq-iov + seg,
 +   ARRAY_SIZE(vq-iov) - seg, out,
 +   in, log, log_num);
 +  if (d == vq-num) {
 + r = 0;
 + goto err;
 +  }
 +  if (unlikely(out || in = 0)) {
 + vq_err(vq, unexpected descriptor format for RX: 
 +out %d, in %d\n, out, in);
 + r = -EINVAL;
 + goto err;
 +  }
 +  heads[headcount].id = d;
 +  heads[headcount].len = iov_length(vq-iov + seg, in);
 +  datalen -= heads[headcount].len;
 +  ++headcount;
 +  seg += in;
 +   }
 +   *iovcount = seg;
 +   return headcount;
 +err:
 +   vhost_discard_desc(vq, headcount);
 +   return r;
 +}
 +
  /* Expects to be always run from workqueue - which acts as
   * read-size critical section for our kind of RCU. */
  static void handle_rx(struct vhost_net *net)
  {
 struct vhost_virtqueue *vq = net-dev.vqs[VHOST_NET_VQ_RX];
 -   unsigned in, log, s;
 +   unsigned uninitialized_var(in), log;
 struct vhost_log *vq_log;
 struct msghdr msg = {
.msg_name = NULL,
 @@ -238,9 +290,10 @@ static void handle_rx(struct vhost_net *net)

Re: [PATCHv7] add mergeable buffers support to vhost_net

2010-04-30 Thread Michael S. Tsirkin 
On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
 This patch adds mergeable receive buffer support to vhost_net.
 
 Signed-off-by: David L Stevens dlstev...@us.ibm.com

I have applied this, thanks very much!
I have also applied some tweaks on top,
please take a look.

Thanks,
MSt

commit 2809e94f5f26d89dc5232aaec753ffda95c4d95e
Author: Michael S. Tsirkin m...@redhat.com
Date:   Thu Apr 29 16:18:08 2010 +0300

vhost-net: minor tweaks in mergeable buffer code

Applies the following tweaks on top of mergeable buffers patch:
1. vhost_get_desc_n assumes that all desriptors are 'in' only.
   It's also unlikely to be useful for any vhost frontend
   besides vhost_net, so just move it to net.c, and rename
   get_rx_bufs for brevity.

2. for rx, we called iov_length within vhost_get_desc_n
   (now get_rx_bufs) already, so we don't
   need an extra call to iov_length to avoid overflow anymore.
   Accordingly, copy_iovec_hdr can return void now.

3. for rx, do some further code tweaks:
   do not assign len = err as we check that err == len
   handle data length in a way similar to how we handle
   header length: datalen - sock_len, len - vhost_len.
   add sock_hlen as a local variable, for symmetry with vhost_hlen.

4. add some likely/unlikely annotations

Signed-off-by: Michael S. Tsirkin m...@redhat.com

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index d61d945..85519b4 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -74,9 +74,9 @@ static int move_iovec_hdr(struct iovec *from, struct iovec 
*to,
}
return seg;
 }
-/* Copy iovec entries for len bytes from iovec. Return segments used. */
-static int copy_iovec_hdr(const struct iovec *from, struct iovec *to,
- size_t len, int iovcount)
+/* Copy iovec entries for len bytes from iovec. */
+static void copy_iovec_hdr(const struct iovec *from, struct iovec *to,
+  size_t len, int iovcount)
 {
int seg = 0;
size_t size;
@@ -89,7 +89,6 @@ static int copy_iovec_hdr(const struct iovec *from, struct 
iovec *to,
++to;
++seg;
}
-   return seg;
 }
 
 /* Caller must have TX VQ lock */
@@ -204,7 +203,7 @@ static void handle_tx(struct vhost_net *net)
unuse_mm(net-dev.mm);
 }
 
-static int vhost_head_len(struct vhost_virtqueue *vq, struct sock *sk)
+static int peek_head_len(struct vhost_virtqueue *vq, struct sock *sk)
 {
struct sk_buff *head;
int len = 0;
@@ -212,17 +211,70 @@ static int vhost_head_len(struct vhost_virtqueue *vq, 
struct sock *sk)
lock_sock(sk);
head = skb_peek(sk-sk_receive_queue);
if (head)
-   len = head-len + vq-sock_hlen;
+   len = head-len;
release_sock(sk);
return len;
 }
 
+/* This is a multi-buffer version of vhost_get_desc, that works if
+ * vq has read descriptors only.
+ * @vq - the relevant virtqueue
+ * @datalen- data length we'll be reading
+ * @iovcount   - returned count of io vectors we fill
+ * @log- vhost log
+ * @log_num- log offset
+ * returns number of buffer heads allocated, negative on error
+ */
+static int get_rx_bufs(struct vhost_virtqueue *vq,
+  struct vring_used_elem *heads,
+  int datalen,
+  unsigned *iovcount,
+  struct vhost_log *log,
+  unsigned *log_num)
+{
+   unsigned int out, in;
+   int seg = 0;
+   int headcount = 0;
+   unsigned d;
+   int r;
+
+   while (datalen  0) {
+   if (unlikely(headcount = VHOST_NET_MAX_SG)) {
+   r = -ENOBUFS;
+   goto err;
+   }
+   d = vhost_get_desc(vq-dev, vq, vq-iov + seg,
+  ARRAY_SIZE(vq-iov) - seg, out,
+  in, log, log_num);
+   if (d == vq-num) {
+   r = 0;
+   goto err;
+   }
+   if (unlikely(out || in = 0)) {
+   vq_err(vq, unexpected descriptor format for RX: 
+   out %d, in %d\n, out, in);
+   r = -EINVAL;
+   goto err;
+   }
+   heads[headcount].id = d;
+   heads[headcount].len = iov_length(vq-iov + seg, in);
+   datalen -= heads[headcount].len;
+   ++headcount;
+   seg += in;
+   }
+   *iovcount = seg;
+   return headcount;
+err:
+   vhost_discard_desc(vq, headcount);
+   return r;
+}
+
 /* Expects to be always run from workqueue - which acts as
  * read-size critical section for our kind of RCU. */
 static void handle_rx(struct vhost_net *net)
 {
struct vhost_virtqueue *vq =

Re: [PATCHv7] add mergeable buffers support to vhost_net

2010-04-30 Thread Michael S. Tsirkin 
On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
 This patch adds mergeable receive buffer support to vhost_net.
 
 Signed-off-by: David L Stevens dlstev...@us.ibm.com

You can find the latest version on the following net-next based tree:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv7] add mergeable buffers support to vhost_net

2010-04-28 Thread David L Stevens 
This patch adds mergeable receive buffer support to vhost_net.

Signed-off-by: David L Stevens dlstev...@us.ibm.com

diff -ruNp net-next-v0/drivers/vhost/net.c net-next-v7/drivers/vhost/net.c
--- net-next-v0/drivers/vhost/net.c 2010-04-24 21:36:54.0 -0700
+++ net-next-v7/drivers/vhost/net.c 2010-04-28 12:26:18.0 -0700
@@ -74,6 +74,23 @@ static int move_iovec_hdr(struct iovec *
}
return seg;
 }
+/* Copy iovec entries for len bytes from iovec. Return segments used. */
+static int copy_iovec_hdr(const struct iovec *from, struct iovec *to,
+ size_t len, int iovcount)
+{
+   int seg = 0;
+   size_t size;
+   while (len  seg  iovcount) {
+   size = min(from-iov_len, len);
+   to-iov_base = from-iov_base;
+   to-iov_len = size;
+   len -= size;
+   ++from;
+   ++to;
+   ++seg;
+   }
+   return seg;
+}
 
 /* Caller must have TX VQ lock */
 static void tx_poll_stop(struct vhost_net *net)
@@ -109,7 +126,7 @@ static void handle_tx(struct vhost_net *
};
size_t len, total_len = 0;
int err, wmem;
-   size_t hdr_size;
+   size_t vhost_hlen;
struct socket *sock = rcu_dereference(vq-private_data);
if (!sock)
return;
@@ -128,13 +145,13 @@ static void handle_tx(struct vhost_net *
 
if (wmem  sock-sk-sk_sndbuf / 2)
tx_poll_stop(net);
-   hdr_size = vq-hdr_size;
+   vhost_hlen = vq-vhost_hlen;
 
for (;;) {
-   head = vhost_get_vq_desc(net-dev, vq, vq-iov,
-ARRAY_SIZE(vq-iov),
-out, in,
-NULL, NULL);
+   head = vhost_get_desc(net-dev, vq, vq-iov,
+ ARRAY_SIZE(vq-iov),
+ out, in,
+ NULL, NULL);
/* Nothing new?  Wait for eventfd to tell us they refilled. */
if (head == vq-num) {
wmem = atomic_read(sock-sk-sk_wmem_alloc);
@@ -155,20 +172,20 @@ static void handle_tx(struct vhost_net *
break;
}
/* Skip header. TODO: support TSO. */
-   s = move_iovec_hdr(vq-iov, vq-hdr, hdr_size, out);
+   s = move_iovec_hdr(vq-iov, vq-hdr, vhost_hlen, out);
msg.msg_iovlen = out;
len = iov_length(vq-iov, out);
/* Sanity check */
if (!len) {
vq_err(vq, Unexpected header len for TX: 
   %zd expected %zd\n,
-  iov_length(vq-hdr, s), hdr_size);
+  iov_length(vq-hdr, s), vhost_hlen);
break;
}
/* TODO: Check specific error and bomb out unless ENOBUFS? */
err = sock-ops-sendmsg(NULL, sock, msg, len);
if (unlikely(err  0)) {
-   vhost_discard_vq_desc(vq);
+   vhost_discard_desc(vq, 1);
tx_poll_start(net, sock);
break;
}
@@ -187,12 +204,25 @@ static void handle_tx(struct vhost_net *
unuse_mm(net-dev.mm);
 }
 
+static int vhost_head_len(struct vhost_virtqueue *vq, struct sock *sk)
+{
+   struct sk_buff *head;
+   int len = 0;
+
+   lock_sock(sk);
+   head = skb_peek(sk-sk_receive_queue);
+   if (head)
+   len = head-len + vq-sock_hlen;
+   release_sock(sk);
+   return len;
+}
+
 /* Expects to be always run from workqueue - which acts as
  * read-size critical section for our kind of RCU. */
 static void handle_rx(struct vhost_net *net)
 {
struct vhost_virtqueue *vq = net-dev.vqs[VHOST_NET_VQ_RX];
-   unsigned head, out, in, log, s;
+   unsigned in, log, s;
struct vhost_log *vq_log;
struct msghdr msg = {
.msg_name = NULL,
@@ -203,14 +233,14 @@ static void handle_rx(struct vhost_net *
.msg_flags = MSG_DONTWAIT,
};
 
-   struct virtio_net_hdr hdr = {
-   .flags = 0,
-   .gso_type = VIRTIO_NET_HDR_GSO_NONE
+   struct virtio_net_hdr_mrg_rxbuf hdr = {
+   .hdr.flags = 0,
+   .hdr.gso_type = VIRTIO_NET_HDR_GSO_NONE
};
 
size_t len, total_len = 0;
-   int err;
-   size_t hdr_size;
+   int err, headcount, datalen;
+   size_t vhost_hlen;
struct socket *sock = rcu_dereference(vq-private_data);
if (!sock || skb_queue_empty(sock-sk-sk_receive_queue))
return;
@@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
use_mm(net-dev.mm);
mutex_lock(vq-mutex);
vhost_disable_notify(vq);
-   hdr_size =