* Re: [RFC] virtio: Support releasing lock during kick
From: Stefan Hajnoczi @ 2010-06-25 6:17 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, Stefan Hajnoczi, Anthony Liguori,
virtualization
In-Reply-To: <201006251239.23224.rusty@rustcorp.com.au>
On Fri, Jun 25, 2010 at 4:09 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Thu, 24 Jun 2010 03:00:30 pm Stefan Hajnoczi wrote:
>> On Wed, Jun 23, 2010 at 11:12 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> > Shouldn't it be possible to just drop the lock before invoking
>> > virtqueue_kick() and reacquire it afterwards? There's nothing in that
>> > virtqueue_kick() path that the lock is protecting AFAICT.
>>
>> No, that would lead to a race condition because vq->num_added is
>> modified by both virtqueue_add_buf_gfp() and virtqueue_kick().
>> Without a lock held during virtqueue_kick() another vcpu could add
>> bufs while vq->num_added is used and cleared by virtqueue_kick():
>
> Right, this dovetails with another proposed change (was it Michael?)
> where we would update the avail idx inside add_buf, rather than waiting
> until kick. This means a barrier inside add_buf, but that's probably
> fine.
>
> If we do that, then we don't need a lock on virtqueue_kick.
That would be nice, we could push the change up into just virtio-blk.
I did wonder if virtio-net can take advantage of unlocked kick, too,
but haven't investigated yet. The virtio-net kick in start_xmit()
happens with the netdev _xmit_lock held. Any ideas?
Stefan
^ permalink raw reply
* Re: [RFC] virtio: Support releasing lock during kick
From: Michael S. Tsirkin @ 2010-06-25 10:43 UTC (permalink / raw)
To: Rusty Russell; +Cc: kvm, Stefan Hajnoczi, Anthony Liguori, virtualization
In-Reply-To: <201006251239.23224.rusty@rustcorp.com.au>
On Fri, Jun 25, 2010 at 12:39:21PM +0930, Rusty Russell wrote:
> On Thu, 24 Jun 2010 03:00:30 pm Stefan Hajnoczi wrote:
> > On Wed, Jun 23, 2010 at 11:12 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> > > Shouldn't it be possible to just drop the lock before invoking
> > > virtqueue_kick() and reacquire it afterwards? There's nothing in that
> > > virtqueue_kick() path that the lock is protecting AFAICT.
> >
> > No, that would lead to a race condition because vq->num_added is
> > modified by both virtqueue_add_buf_gfp() and virtqueue_kick().
> > Without a lock held during virtqueue_kick() another vcpu could add
> > bufs while vq->num_added is used and cleared by virtqueue_kick():
>
> Right, this dovetails with another proposed change (was it Michael?)
> where we would update the avail idx inside add_buf, rather than waiting
> until kick. This means a barrier inside add_buf, but that's probably
> fine.
>
> If we do that, then we don't need a lock on virtqueue_kick.
>
> Michael, thoughts?
Maybe not even that: I think we could just do virtio_wmb()
in add, and keep the mb() in kick.
What I'm a bit worried about is contention on the cacheline
including index and flags: the more we write to that line,
the worse it gets.
So need to test performance impact of this change:
I didn't find time to do this yet, as I am trying
to finalize the used index publishing patches.
Any takers?
Do we see performance improvement after making kick lockless?
> Thanks,
> Rusty.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Call for Demos and Exhibition: The 7th International Conference on Autonomic and Trusted Computing (ATC 2010)
From: Ming Zhao @ 2010-06-25 13:21 UTC (permalink / raw)
Call for Demos and Exhibition
The 7th International Conference on Autonomic and Trusted Computing (ATC
2010)
Xi'an China, October 26-29, 2010
http://www.nwpu.edu.cn/atc2010/
The ATC 2010 demo/exhibition program provides researchers and engineers
with opportunities to show their cutting-edge work presented in an
interactive fashion. The live demonstrations and exhibitions may include
implementations of techniques presented in the main conference,
workshops, other conferences and venues, as well as systems and video.
We encourage academic researchers as well as industry lab researchers
and companies to participate in showcasing their products and research.
Prospective applicants should submit a 4-page abstract of the
demonstration/exhibition in LNCS-format via email (atc10demo@gmail.com).
Video demonstrations are also welcome.
The main selection criteria will be the expected general interest of the
demonstration to the Autonomic and Trusted Computing community. The
accepted demo/exhibition will be included in the conference proceedings.
As a condition for acceptance, at least one of the people must be
registered for the main conference, and present the demo in the
conference. All demos will compete for the Best Demo Award.
At the conference, you will at minimum be provided with a space for your
demo, a poster stand, a table and chairs, and a power connection (220V).
Any other special equipment should be provided by yourself, including
power converters, projectors, audio amplification, wireless LAN, etc.
Please specify this with a separate page so that we can plan the demo
area accordingly.
Demo/Exhibition Chairs:
• Alvin Chin, Nokia Research Center, China
• Ming Zhao, Florida International University, USA
Important Dates:
• July 9, 2010 — Submission deadline
• July 15, 2010 — Notification of acceptance
• July 30, 2010 — Final version due
Further Information:
• atc10demo@gmail.com
^ permalink raw reply
* Re: [RFC] virtio: Support releasing lock during kick
From: Stefan Hajnoczi @ 2010-06-25 15:31 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, Anthony Liguori, virtualization
In-Reply-To: <20100625104317.GC16321@redhat.com>
On Fri, Jun 25, 2010 at 01:43:17PM +0300, Michael S. Tsirkin wrote:
> On Fri, Jun 25, 2010 at 12:39:21PM +0930, Rusty Russell wrote:
> > On Thu, 24 Jun 2010 03:00:30 pm Stefan Hajnoczi wrote:
> > > On Wed, Jun 23, 2010 at 11:12 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> > > > Shouldn't it be possible to just drop the lock before invoking
> > > > virtqueue_kick() and reacquire it afterwards? There's nothing in that
> > > > virtqueue_kick() path that the lock is protecting AFAICT.
> > >
> > > No, that would lead to a race condition because vq->num_added is
> > > modified by both virtqueue_add_buf_gfp() and virtqueue_kick().
> > > Without a lock held during virtqueue_kick() another vcpu could add
> > > bufs while vq->num_added is used and cleared by virtqueue_kick():
> >
> > Right, this dovetails with another proposed change (was it Michael?)
> > where we would update the avail idx inside add_buf, rather than waiting
> > until kick. This means a barrier inside add_buf, but that's probably
> > fine.
> >
> > If we do that, then we don't need a lock on virtqueue_kick.
> >
> > Michael, thoughts?
>
> Maybe not even that: I think we could just do virtio_wmb()
> in add, and keep the mb() in kick.
>
> What I'm a bit worried about is contention on the cacheline
> including index and flags: the more we write to that line,
> the worse it gets.
>
> So need to test performance impact of this change:
> I didn't find time to do this yet, as I am trying
> to finalize the used index publishing patches.
> Any takers?
>
> Do we see performance improvement after making kick lockless?
There was no guest CPU reduction or I/O throughput increase with my
patch when running 4 dd iflag=direct bs=4k if=/dev/vdb of=/dev/null
processes. However, the lock_stat numbers above show clear improvement
of the lock hold/wait times.
I was hoping to see guest CPU utilization go down and I/O throughput go
up, so there is still investigation to do with my patch in isolation.
Although I'd like to try it later, putting my patch on top of your avail
idx work is too early because it will be harder to reason about the
performance with both patches present at the same time.
Stefan
^ permalink raw reply
* Re: [RFC] virtio: Support releasing lock during kick
From: Michael S. Tsirkin @ 2010-06-25 15:32 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kvm, Anthony Liguori, virtualization
In-Reply-To: <20100625153143.GA12784@stefan-thinkpad.transitives.com>
On Fri, Jun 25, 2010 at 04:31:44PM +0100, Stefan Hajnoczi wrote:
> On Fri, Jun 25, 2010 at 01:43:17PM +0300, Michael S. Tsirkin wrote:
> > On Fri, Jun 25, 2010 at 12:39:21PM +0930, Rusty Russell wrote:
> > > On Thu, 24 Jun 2010 03:00:30 pm Stefan Hajnoczi wrote:
> > > > On Wed, Jun 23, 2010 at 11:12 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> > > > > Shouldn't it be possible to just drop the lock before invoking
> > > > > virtqueue_kick() and reacquire it afterwards? There's nothing in that
> > > > > virtqueue_kick() path that the lock is protecting AFAICT.
> > > >
> > > > No, that would lead to a race condition because vq->num_added is
> > > > modified by both virtqueue_add_buf_gfp() and virtqueue_kick().
> > > > Without a lock held during virtqueue_kick() another vcpu could add
> > > > bufs while vq->num_added is used and cleared by virtqueue_kick():
> > >
> > > Right, this dovetails with another proposed change (was it Michael?)
> > > where we would update the avail idx inside add_buf, rather than waiting
> > > until kick. This means a barrier inside add_buf, but that's probably
> > > fine.
> > >
> > > If we do that, then we don't need a lock on virtqueue_kick.
> > >
> > > Michael, thoughts?
> >
> > Maybe not even that: I think we could just do virtio_wmb()
> > in add, and keep the mb() in kick.
> >
> > What I'm a bit worried about is contention on the cacheline
> > including index and flags: the more we write to that line,
> > the worse it gets.
> >
> > So need to test performance impact of this change:
> > I didn't find time to do this yet, as I am trying
> > to finalize the used index publishing patches.
> > Any takers?
> >
> > Do we see performance improvement after making kick lockless?
>
> There was no guest CPU reduction or I/O throughput increase with my
> patch when running 4 dd iflag=direct bs=4k if=/dev/vdb of=/dev/null
> processes. However, the lock_stat numbers above show clear improvement
> of the lock hold/wait times.
>
> I was hoping to see guest CPU utilization go down and I/O throughput go
> up, so there is still investigation to do with my patch in isolation.
> Although I'd like to try it later, putting my patch on top of your avail
> idx work is too early because it will be harder to reason about the
> performance with both patches present at the same time.
>
> Stefan
What about host CPU utilization?
Also, are you using PARAVIRT_SPINLOCKS?
--
MST
^ permalink raw reply
* Re: [RFC] virtio: Support releasing lock during kick
From: Stefan Hajnoczi @ 2010-06-25 16:05 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, Anthony Liguori, virtualization
In-Reply-To: <20100625153220.GB17911@redhat.com>
On Fri, Jun 25, 2010 at 06:32:20PM +0300, Michael S. Tsirkin wrote:
> On Fri, Jun 25, 2010 at 04:31:44PM +0100, Stefan Hajnoczi wrote:
> > On Fri, Jun 25, 2010 at 01:43:17PM +0300, Michael S. Tsirkin wrote:
> > > On Fri, Jun 25, 2010 at 12:39:21PM +0930, Rusty Russell wrote:
> > > > On Thu, 24 Jun 2010 03:00:30 pm Stefan Hajnoczi wrote:
> > > > > On Wed, Jun 23, 2010 at 11:12 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> > > > > > Shouldn't it be possible to just drop the lock before invoking
> > > > > > virtqueue_kick() and reacquire it afterwards? There's nothing in that
> > > > > > virtqueue_kick() path that the lock is protecting AFAICT.
> > > > >
> > > > > No, that would lead to a race condition because vq->num_added is
> > > > > modified by both virtqueue_add_buf_gfp() and virtqueue_kick().
> > > > > Without a lock held during virtqueue_kick() another vcpu could add
> > > > > bufs while vq->num_added is used and cleared by virtqueue_kick():
> > > >
> > > > Right, this dovetails with another proposed change (was it Michael?)
> > > > where we would update the avail idx inside add_buf, rather than waiting
> > > > until kick. This means a barrier inside add_buf, but that's probably
> > > > fine.
> > > >
> > > > If we do that, then we don't need a lock on virtqueue_kick.
> > > >
> > > > Michael, thoughts?
> > >
> > > Maybe not even that: I think we could just do virtio_wmb()
> > > in add, and keep the mb() in kick.
> > >
> > > What I'm a bit worried about is contention on the cacheline
> > > including index and flags: the more we write to that line,
> > > the worse it gets.
> > >
> > > So need to test performance impact of this change:
> > > I didn't find time to do this yet, as I am trying
> > > to finalize the used index publishing patches.
> > > Any takers?
> > >
> > > Do we see performance improvement after making kick lockless?
> >
> > There was no guest CPU reduction or I/O throughput increase with my
> > patch when running 4 dd iflag=direct bs=4k if=/dev/vdb of=/dev/null
> > processes. However, the lock_stat numbers above show clear improvement
> > of the lock hold/wait times.
> >
> > I was hoping to see guest CPU utilization go down and I/O throughput go
> > up, so there is still investigation to do with my patch in isolation.
> > Although I'd like to try it later, putting my patch on top of your avail
> > idx work is too early because it will be harder to reason about the
> > performance with both patches present at the same time.
> >
> > Stefan
>
> What about host CPU utilization?
There is data available for host CPU utilization, I need to dig it up.
> Also, are you using PARAVIRT_SPINLOCKS?
No. I haven't found much documentation on paravirt spinlocks other than
the commit that introduced them:
commit 8efcbab674de2bee45a2e4cdf97de16b8e609ac8
Author: Jeremy Fitzhardinge <jeremy@goop.org>
Date: Mon Jul 7 12:07:51 2008 -0700
paravirt: introduce a "lock-byte" spinlock implementation
PARAVIRT_SPINLOCKS is not set in the config I use, probably because of
the associated performance issue that causes distros to build without
them:
commit b4ecc126991b30fe5f9a59dfacda046aeac124b2
Author: Jeremy Fitzhardinge <jeremy@goop.org>
Date: Wed May 13 17:16:55 2009 -0700
x86: Fix performance regression caused by paravirt_ops on native
kernels
I would expect performance results to be smoother with
PARAVIRT_SPINLOCKS for the guest kernel. I will add it for future runs,
thanks for pointing it out.
Stefan
^ permalink raw reply
* [PATCH] vhost: break out of polling loop on error
From: Michael S. Tsirkin @ 2010-06-27 8:59 UTC (permalink / raw)
To: Sridhar Samudrala, Arnd Bergmann, Paul E. McKenney, Juan Quintela,
Rus
When ring parsing fails, we currently handle this
as ring empty condition. This means that we enable
kicks and recheck ring empty: if this not empty,
we re-start polling which of course will fail again.
Instead, let's return a negative error code and stop polling.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Dave, I'm sending this out so it can get reviewed.
I'll put this on my vhost tree
so no need for you to pick this patch directly.
drivers/vhost/net.c | 12 ++++++++++--
drivers/vhost/vhost.c | 33 +++++++++++++++++----------------
drivers/vhost/vhost.h | 8 ++++----
3 files changed, 31 insertions(+), 22 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 0f41c91..54096ee 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -98,7 +98,8 @@ static void tx_poll_start(struct vhost_net *net, struct socket *sock)
static void handle_tx(struct vhost_net *net)
{
struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
- unsigned head, out, in, s;
+ unsigned out, in, s;
+ int head;
struct msghdr msg = {
.msg_name = NULL,
.msg_namelen = 0,
@@ -135,6 +136,9 @@ static void handle_tx(struct vhost_net *net)
ARRAY_SIZE(vq->iov),
&out, &in,
NULL, NULL);
+ /* On error, stop handling until the next kick. */
+ if (head < 0)
+ break;
/* Nothing new? Wait for eventfd to tell us they refilled. */
if (head == vq->num) {
wmem = atomic_read(&sock->sk->sk_wmem_alloc);
@@ -192,7 +196,8 @@ static void handle_tx(struct vhost_net *net)
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 out, in, log, s;
+ int head;
struct vhost_log *vq_log;
struct msghdr msg = {
.msg_name = NULL,
@@ -228,6 +233,9 @@ static void handle_rx(struct vhost_net *net)
ARRAY_SIZE(vq->iov),
&out, &in,
vq_log, &log);
+ /* On error, stop handling until the next kick. */
+ if (head < 0)
+ break;
/* OK, now we need to know about added descriptors. */
if (head == vq->num) {
if (unlikely(vhost_enable_notify(vq))) {
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 3b83382..5ccd384 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -873,12 +873,13 @@ static unsigned get_indirect(struct vhost_dev *dev, struct vhost_virtqueue *vq,
* number of output then some number of input descriptors, it's actually two
* iovecs, but we pack them into one and note how many of each there were.
*
- * This function returns the descriptor number found, or vq->num (which
- * is never a valid descriptor number) if none was found. */
-unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
- struct iovec iov[], unsigned int iov_size,
- unsigned int *out_num, unsigned int *in_num,
- struct vhost_log *log, unsigned int *log_num)
+ * This function returns the descriptor number found, or vq->num (which is
+ * never a valid descriptor number) if none was found. A negative code is
+ * returned on error. */
+int vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
+ struct iovec iov[], unsigned int iov_size,
+ unsigned int *out_num, unsigned int *in_num,
+ struct vhost_log *log, unsigned int *log_num)
{
struct vring_desc desc;
unsigned int i, head, found = 0;
@@ -890,13 +891,13 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
if (get_user(vq->avail_idx, &vq->avail->idx)) {
vq_err(vq, "Failed to access avail idx at %p\n",
&vq->avail->idx);
- return vq->num;
+ return -EFAULT;
}
if ((u16)(vq->avail_idx - last_avail_idx) > vq->num) {
vq_err(vq, "Guest moved used index from %u to %u",
last_avail_idx, vq->avail_idx);
- return vq->num;
+ return -EFAULT;
}
/* If there's nothing new since last we looked, return invalid. */
@@ -912,14 +913,14 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
vq_err(vq, "Failed to read head: idx %d address %p\n",
last_avail_idx,
&vq->avail->ring[last_avail_idx % vq->num]);
- return vq->num;
+ return -EFAULT;
}
/* If their number is silly, that's an error. */
if (head >= vq->num) {
vq_err(vq, "Guest says index %u > %u is available",
head, vq->num);
- return vq->num;
+ return -EINVAL;
}
/* When we start there are none of either input nor output. */
@@ -933,19 +934,19 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
if (i >= vq->num) {
vq_err(vq, "Desc index is %u > %u, head = %u",
i, vq->num, head);
- return vq->num;
+ return -EINVAL;
}
if (++found > vq->num) {
vq_err(vq, "Loop detected: last one at %u "
"vq size %u head %u\n",
i, vq->num, head);
- return vq->num;
+ return -EINVAL;
}
ret = copy_from_user(&desc, vq->desc + i, sizeof desc);
if (ret) {
vq_err(vq, "Failed to get descriptor: idx %d addr %p\n",
i, vq->desc + i);
- return vq->num;
+ return -EFAULT;
}
if (desc.flags & VRING_DESC_F_INDIRECT) {
ret = get_indirect(dev, vq, iov, iov_size,
@@ -954,7 +955,7 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
if (ret < 0) {
vq_err(vq, "Failure detected "
"in indirect descriptor at idx %d\n", i);
- return vq->num;
+ return ret;
}
continue;
}
@@ -964,7 +965,7 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
if (ret < 0) {
vq_err(vq, "Translation failure %d descriptor idx %d\n",
ret, i);
- return vq->num;
+ return ret;
}
if (desc.flags & VRING_DESC_F_WRITE) {
/* If this is an input descriptor,
@@ -981,7 +982,7 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
if (*in_num) {
vq_err(vq, "Descriptor has out after in: "
"idx %d\n", i);
- return vq->num;
+ return -EINVAL;
}
*out_num += ret;
}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 44591ba..11ee13d 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -120,10 +120,10 @@ long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, unsigned long arg);
int vhost_vq_access_ok(struct vhost_virtqueue *vq);
int vhost_log_access_ok(struct vhost_dev *);
-unsigned vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
- struct iovec iov[], unsigned int iov_count,
- unsigned int *out_num, unsigned int *in_num,
- struct vhost_log *log, unsigned int *log_num);
+int vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
+ struct iovec iov[], unsigned int iov_count,
+ unsigned int *out_num, unsigned int *in_num,
+ struct vhost_log *log, unsigned int *log_num);
void vhost_discard_vq_desc(struct vhost_virtqueue *);
int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len);
--
1.7.1.12.g42b7f
^ permalink raw reply related
* [PATCH RFC] vhost-net: add dhclient work-around from userspace
From: Michael S. Tsirkin @ 2010-06-27 15:46 UTC (permalink / raw)
To: Sridhar Samudrala, David S. Miller, Arnd Bergmann,
Paul E. McKenney, kvm
In-Reply-To: <20100626.200320.43025947.davem@davemloft.net>
Userspace virtio server has the following hack
so guests rely on it, and we have to replicate it, too:
use source port to detect incoming IPv4 DHCP response packets,
and fill in the checksum for these.
The issue we are solving is that on linux guests, some apps
that use recvmsg with AF_PACKET sockets, don't know how to
handle CHECKSUM_PARTIAL;
The interface to return the relevant information was added
in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
and older userspace does not use it.
One important user of recvmsg with AF_PACKET is dhclient,
so we add a work-around just for DHCP.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
So here's what I came up with: I basically copied
the work-around from userspace virtio.
As suggested by Dave (assuming I understood the suggestion
correctly) this implements the workaround in vhost-net, so
other tun users don't start relying on it.
Untested.
drivers/vhost/net.c | 42 +++++++++++++++++++++++++++++++++++++++++-
1 files changed, 41 insertions(+), 1 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 54096ee..9ed4051 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -25,6 +25,10 @@
#include <linux/if_tun.h>
#include <linux/if_macvlan.h>
+#include <linux/ip.h>
+#include <linux/udp.h>
+#include <linux/netdevice.h>
+
#include <net/sock.h>
#include "vhost.h"
@@ -191,6 +195,42 @@ static void handle_tx(struct vhost_net *net)
unuse_mm(net->dev.mm);
}
+static int peek_head(struct sock *sk)
+{
+ struct sk_buff *head;
+ int ret;
+
+ lock_sock(sk);
+ head = skb_peek(&sk->sk_receive_queue);
+ if (likely(head)) {
+ ret = 1;
+ /* Userspace virtio server has the following hack so
+ * guests rely on it, and we have to replicate it, too: */
+ /* On linux guests, some apps that use recvmsg with AF_PACKET
+ * sockets, don't know how to handle CHECKSUM_PARTIAL;
+ * The interface to return the relevant information was added in
+ * 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
+ * and older userspace does not use it.
+ * One important user of recvmsg with AF_PACKET is dhclient,
+ * so we add a work-around just for DHCP. */
+ /* We use source port to detect DHCP packets. */
+ if (skb->ip_summed == CHECKSUM_PARTIAL &&
+ skb->protocol == htons(ETH_P_IP) &&
+ skb_network_header_len(skb) >= sizeof(struct iphdr) &&
+ ip_hdr(skb)->protocol == IPPRODO_UDP &&
+ skb_headlen(skb) >= skb_transport_offset(skb) + sizeof(struct udphdr) &&
+ udp_hdr(skb)->source == htons(0x67)) {
+ skb_checksum_help(skb);
+ /* Restore ip_summed value: tun passes it to user. */
+ skb->ip_summed = CHECKSUM_PARTIAL;
+ }
+ } else {
+ ret = 0;
+ }
+ 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)
@@ -228,7 +268,7 @@ static void handle_rx(struct vhost_net *net)
vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
vq->log : NULL;
- for (;;) {
+ while (peek_head(sock)) {
head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
ARRAY_SIZE(vq->iov),
&out, &in,
--
1.7.1.12.g42b7f
^ permalink raw reply related
* [PATCHv2] vhost-net: add dhclient work-around from userspace
From: Michael S. Tsirkin @ 2010-06-28 10:08 UTC (permalink / raw)
To: Michael S. Tsirkin, Aristeu Rozanski, Herbert Xu, Juan Quintela,
D
Userspace virtio server has the following hack
so guests rely on it, and we have to replicate it, too:
Use port number to detect incoming IPv4 DHCP response packets,
and fill in the checksum for these.
The issue we are solving is that on linux guests, some apps
that use recvmsg with AF_PACKET sockets, don't know how to
handle CHECKSUM_PARTIAL;
The interface to return the relevant information was added
in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
and older userspace does not use it.
One important user of recvmsg with AF_PACKET is dhclient,
so we add a work-around just for DHCP.
Don't bother applying the hack to IPv6 as userspace virtio does not
have a work-around for that - let's hope guests will do the right
thing wrt IPv6.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Dave, I'm going to put this patch on the vhost tree,
no need for you to bother merging it - you'll get
it with a pull request.
drivers/vhost/net.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 43 insertions(+), 1 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index cc19595..03bba6a 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -24,6 +24,10 @@
#include <linux/if_tun.h>
#include <linux/if_macvlan.h>
+#include <linux/ip.h>
+#include <linux/udp.h>
+#include <linux/netdevice.h>
+
#include <net/sock.h>
#include "vhost.h"
@@ -186,6 +190,44 @@ static void handle_tx(struct vhost_net *net)
unuse_mm(net->dev.mm);
}
+static int peek_head(struct sock *sk)
+{
+ struct sk_buff *skb;
+
+ lock_sock(sk);
+ skb = skb_peek(&sk->sk_receive_queue);
+ if (unlikely(!skb)) {
+ release_sock(sk);
+ return 0;
+ }
+ /* Userspace virtio server has the following hack so
+ * guests rely on it, and we have to replicate it, too: */
+ /* Use port number to detect incoming IPv4 DHCP response packets,
+ * and fill in the checksum. */
+
+ /* The issue we are solving is that on linux guests, some apps
+ * that use recvmsg with AF_PACKET sockets, don't know how to
+ * handle CHECKSUM_PARTIAL;
+ * The interface to return the relevant information was added in
+ * 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
+ * and older userspace does not use it.
+ * One important user of recvmsg with AF_PACKET is dhclient,
+ * so we add a work-around just for DHCP. */
+ if (skb->ip_summed == CHECKSUM_PARTIAL &&
+ skb_headlen(skb) >= skb_transport_offset(skb) +
+ sizeof(struct udphdr) &&
+ udp_hdr(skb)->dest == htons(68) &&
+ skb_network_header_len(skb) >= sizeof(struct iphdr) &&
+ ip_hdr(skb)->protocol == IPPROTO_UDP &&
+ skb->protocol == htons(ETH_P_IP)) {
+ skb_checksum_help(skb);
+ /* Restore ip_summed value: tun passes it to user. */
+ skb->ip_summed = CHECKSUM_PARTIAL;
+ }
+ release_sock(sk);
+ return 1;
+}
+
/* 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)
@@ -222,7 +264,7 @@ static void handle_rx(struct vhost_net *net)
vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
vq->log : NULL;
- for (;;) {
+ while (peek_head(sock->sk)) {
head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
ARRAY_SIZE(vq->iov),
&out, &in,
--
1.7.1.12.g42b7f
^ permalink raw reply related
* Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
From: Michael S. Tsirkin @ 2010-06-28 15:30 UTC (permalink / raw)
To: kvm, virtualization, netdev, linux-kernel, ykaul, markmc
In-Reply-To: <20100628100807.GA30685@redhat.com>
On Mon, Jun 28, 2010 at 01:08:07PM +0300, Michael S. Tsirkin wrote:
> Userspace virtio server has the following hack
> so guests rely on it, and we have to replicate it, too:
>
> Use port number to detect incoming IPv4 DHCP response packets,
> and fill in the checksum for these.
>
> The issue we are solving is that on linux guests, some apps
> that use recvmsg with AF_PACKET sockets, don't know how to
> handle CHECKSUM_PARTIAL;
> The interface to return the relevant information was added
> in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> and older userspace does not use it.
> One important user of recvmsg with AF_PACKET is dhclient,
> so we add a work-around just for DHCP.
>
> Don't bother applying the hack to IPv6 as userspace virtio does not
> have a work-around for that - let's hope guests will do the right
> thing wrt IPv6.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Laine Stump <laine@redhat.com>
^ permalink raw reply
* Re: [RFC] virtio: Support releasing lock during kick
From: Marcelo Tosatti @ 2010-06-28 15:55 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kvm, virtualization
In-Reply-To: <1277328242-10685-1-git-send-email-stefanha@linux.vnet.ibm.com>
On Wed, Jun 23, 2010 at 10:24:02PM +0100, Stefan Hajnoczi wrote:
> The virtio block device holds a lock during I/O request processing.
> Kicking the virtqueue while the lock is held results in long lock hold
> times and increases contention for the lock.
>
> This patch modifies virtqueue_kick() to optionally release a lock while
> notifying the host. Virtio block is modified to pass in its lock. This
> allows other vcpus to queue I/O requests during the time spent servicing
> the virtqueue notify in the host.
>
> The virtqueue_kick() function is modified to know about locking because
> it changes the state of the virtqueue and should execute with the lock
> held (it would not be correct for virtio block to release the lock
> before calling virtqueue_kick()).
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
> I am not yet 100% happy with this patch which aims to reduce guest CPU
> consumption related to vblk->lock contention. Although this patch reduces
> wait/hold times it does not affect I/O throughput or guest CPU utilization.
> More investigation is required to get to the bottom of why guest CPU
> utilization does not decrease when a lock bottleneck has been removed.
>
> Performance figures:
>
> Host: 2.6.34 upstream kernel, qemu-kvm-0.12.4 if=virtio,cache=none
> Guest: 2.6.35-rc3-kvm.git upstream kernel
> Storage: 12 disks as striped LVM volume
> Benchmark: 4 concurrent dd bs=4k iflag=direct
>
> Lockstat data for &vblk->lock:
>
> test con-bounces contentions waittime-min waittime-max waittime-total
> unmodified 7097 7108 0.31 956.09 161165.4
> patched 11484 11550 0.30 411.80 50245.83
>
> The maximum wait time went down by 544.29 us (-57%) and the total wait time
> decreased by 69%. This shows that the virtqueue kick is indeed hogging the
> lock.
>
> The patched version actually has higher contention than the unmodified version.
> I think the reason for this is that each virtqueue kick now includes a short
> release and reacquire. This short release gives other vcpus a chance to
> acquire the lock and progress, hence more contention but overall better wait
> time numbers.
>
> name acq-bounces acquisitions holdtime-min holdtime-max holdtime-total
> unmodified 10771 5038346 0.00 3271.81 59016905.47
> patched 31594 5857813 0.00 219.76 24104915.55
>
> Here we see the full impact of this patch: hold time reduced to 219.76 us
> (-93%).
>
> Again the acquisitions have increased since we're now doing an extra
> unlock+lock per virtqueue kick.
>
> Testing, ideas, and comments appreciated.
>
> drivers/block/virtio_blk.c | 2 +-
> drivers/char/hw_random/virtio-rng.c | 2 +-
> drivers/char/virtio_console.c | 6 +++---
> drivers/net/virtio_net.c | 6 +++---
> drivers/virtio/virtio_balloon.c | 6 +++---
> drivers/virtio/virtio_ring.c | 13 +++++++++++--
> include/linux/virtio.h | 3 ++-
> net/9p/trans_virtio.c | 2 +-
> 8 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 258bc2a..de033bf 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -187,7 +187,7 @@ static void do_virtblk_request(struct request_queue *q)
> }
>
> if (issued)
> - virtqueue_kick(vblk->vq);
> + virtqueue_kick(vblk->vq, &vblk->lock);
> }
>
> static void virtblk_prepare_flush(struct request_queue *q, struct request *req)
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index 75f1cbd..852d563 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -49,7 +49,7 @@ static void register_buffer(u8 *buf, size_t size)
> if (virtqueue_add_buf(vq, &sg, 0, 1, buf) < 0)
> BUG();
>
> - virtqueue_kick(vq);
> + virtqueue_kick(vq, NULL);
> }
>
> static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 942a982..677714d 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -328,7 +328,7 @@ static int add_inbuf(struct virtqueue *vq, struct port_buffer *buf)
> sg_init_one(sg, buf->buf, buf->size);
>
> ret = virtqueue_add_buf(vq, sg, 0, 1, buf);
> - virtqueue_kick(vq);
> + virtqueue_kick(vq, NULL);
> return ret;
> }
>
> @@ -400,7 +400,7 @@ static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
>
> sg_init_one(sg, &cpkt, sizeof(cpkt));
> if (virtqueue_add_buf(vq, sg, 1, 0, &cpkt) >= 0) {
> - virtqueue_kick(vq);
> + virtqueue_kick(vq, NULL);
> while (!virtqueue_get_buf(vq, &len))
> cpu_relax();
> }
> @@ -444,7 +444,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
> ret = virtqueue_add_buf(out_vq, sg, 1, 0, in_buf);
>
> /* Tell Host to go! */
> - virtqueue_kick(out_vq);
> + virtqueue_kick(out_vq, NULL);
>
> if (ret < 0) {
> in_count = 0;
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1edb7a6..6a837b3 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -433,7 +433,7 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
> } while (err > 0);
> if (unlikely(vi->num > vi->max))
> vi->max = vi->num;
> - virtqueue_kick(vi->rvq);
> + virtqueue_kick(vi->rvq, NULL);
> return !oom;
> }
>
> @@ -581,7 +581,7 @@ again:
> }
> return NETDEV_TX_BUSY;
> }
> - virtqueue_kick(vi->svq);
> + virtqueue_kick(vi->svq, NULL);
>
> /* Don't wait up for transmitted skbs to be freed. */
> skb_orphan(skb);
> @@ -680,7 +680,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
>
> BUG_ON(virtqueue_add_buf(vi->cvq, sg, out, in, vi) < 0);
>
> - virtqueue_kick(vi->cvq);
> + virtqueue_kick(vi->cvq, NULL);
>
> /*
> * Spin for a response, the kick causes an ioport write, trapping
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 0f1da45..c9c5c4a 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -91,7 +91,7 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
> /* We should always be able to add one buffer to an empty queue. */
> if (virtqueue_add_buf(vq, &sg, 1, 0, vb) < 0)
> BUG();
> - virtqueue_kick(vq);
> + virtqueue_kick(vq, NULL);
>
> /* When host has read buffer, this completes via balloon_ack */
> wait_for_completion(&vb->acked);
> @@ -223,7 +223,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
> sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> if (virtqueue_add_buf(vq, &sg, 1, 0, vb) < 0)
> BUG();
> - virtqueue_kick(vq);
> + virtqueue_kick(vq, NULL);
> }
>
> static void virtballoon_changed(struct virtio_device *vdev)
> @@ -316,7 +316,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> sg_init_one(&sg, vb->stats, sizeof vb->stats);
> if (virtqueue_add_buf(vb->stats_vq, &sg, 1, 0, vb) < 0)
> BUG();
> - virtqueue_kick(vb->stats_vq);
> + virtqueue_kick(vb->stats_vq, NULL);
> }
>
> vb->thread = kthread_run(balloon, vb, "vballoon");
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 1ca8890..163a237 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -236,7 +236,7 @@ add_head:
> }
> EXPORT_SYMBOL_GPL(virtqueue_add_buf_gfp);
>
> -void virtqueue_kick(struct virtqueue *_vq)
> +void virtqueue_kick(struct virtqueue *_vq, spinlock_t *lock)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> START_USE(vq);
> @@ -250,10 +250,19 @@ void virtqueue_kick(struct virtqueue *_vq)
> /* Need to update avail index before checking if we should notify */
> virtio_mb();
>
> - if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY))
> + if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY)) {
> + /* Release lock while doing the kick because the guest should
> + * not exit with the lock held. */
> + if (lock)
> + spin_unlock(lock);
> +
> /* Prod other side to tell it about changes. */
> vq->notify(&vq->vq);
>
> + if (lock)
> + spin_lock(lock);
> + }
> +
The queue lock requires irq's to be disabled in addition to lock
acquision.
^ permalink raw reply
* Re: [PATCH] vhost: break out of polling loop on error
From: Sridhar Samudrala @ 2010-06-28 17:11 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Sridhar Samudrala, Arnd Bergmann, Paul E. McKenney, Juan Quintela,
Rusty Russell, Jes.Sorensen, kraxel, Takuya Yoshikawa, kvm,
virtualization, netdev, linux-kernel
In-Reply-To: <20100627085907.GA8588@redhat.com>
On Sun, 2010-06-27 at 11:59 +0300, Michael S. Tsirkin wrote:
> When ring parsing fails, we currently handle this
> as ring empty condition. This means that we enable
> kicks and recheck ring empty: if this not empty,
> we re-start polling which of course will fail again.
>
> Instead, let's return a negative error code and stop polling.
One minor comment on error return below. With that change,
Acked-by: Sridhar Samudrala <sri@us.ibm.com>
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Dave, I'm sending this out so it can get reviewed.
> I'll put this on my vhost tree
> so no need for you to pick this patch directly.
>
> drivers/vhost/net.c | 12 ++++++++++--
> drivers/vhost/vhost.c | 33 +++++++++++++++++----------------
> drivers/vhost/vhost.h | 8 ++++----
> 3 files changed, 31 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 0f41c91..54096ee 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -98,7 +98,8 @@ static void tx_poll_start(struct vhost_net *net, struct socket *sock)
> static void handle_tx(struct vhost_net *net)
> {
> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
> - unsigned head, out, in, s;
> + unsigned out, in, s;
> + int head;
> struct msghdr msg = {
> .msg_name = NULL,
> .msg_namelen = 0,
> @@ -135,6 +136,9 @@ static void handle_tx(struct vhost_net *net)
> ARRAY_SIZE(vq->iov),
> &out, &in,
> NULL, NULL);
> + /* On error, stop handling until the next kick. */
> + if (head < 0)
> + break;
> /* Nothing new? Wait for eventfd to tell us they refilled. */
> if (head == vq->num) {
> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> @@ -192,7 +196,8 @@ static void handle_tx(struct vhost_net *net)
> 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 out, in, log, s;
> + int head;
> struct vhost_log *vq_log;
> struct msghdr msg = {
> .msg_name = NULL,
> @@ -228,6 +233,9 @@ static void handle_rx(struct vhost_net *net)
> ARRAY_SIZE(vq->iov),
> &out, &in,
> vq_log, &log);
> + /* On error, stop handling until the next kick. */
> + if (head < 0)
> + break;
> /* OK, now we need to know about added descriptors. */
> if (head == vq->num) {
> if (unlikely(vhost_enable_notify(vq))) {
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 3b83382..5ccd384 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -873,12 +873,13 @@ static unsigned get_indirect(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> * number of output then some number of input descriptors, it's actually two
> * iovecs, but we pack them into one and note how many of each there were.
> *
> - * This function returns the descriptor number found, or vq->num (which
> - * is never a valid descriptor number) if none was found. */
> -unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> - struct iovec iov[], unsigned int iov_size,
> - unsigned int *out_num, unsigned int *in_num,
> - struct vhost_log *log, unsigned int *log_num)
> + * This function returns the descriptor number found, or vq->num (which is
> + * never a valid descriptor number) if none was found. A negative code is
> + * returned on error. */
> +int vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> + struct iovec iov[], unsigned int iov_size,
> + unsigned int *out_num, unsigned int *in_num,
> + struct vhost_log *log, unsigned int *log_num)
> {
> struct vring_desc desc;
> unsigned int i, head, found = 0;
> @@ -890,13 +891,13 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> if (get_user(vq->avail_idx, &vq->avail->idx)) {
> vq_err(vq, "Failed to access avail idx at %p\n",
> &vq->avail->idx);
> - return vq->num;
> + return -EFAULT;
> }
>
> if ((u16)(vq->avail_idx - last_avail_idx) > vq->num) {
> vq_err(vq, "Guest moved used index from %u to %u",
> last_avail_idx, vq->avail_idx);
> - return vq->num;
> + return -EFAULT;
This should be -EINVAL
> }
>
> /* If there's nothing new since last we looked, return invalid. */
> @@ -912,14 +913,14 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> vq_err(vq, "Failed to read head: idx %d address %p\n",
> last_avail_idx,
> &vq->avail->ring[last_avail_idx % vq->num]);
> - return vq->num;
> + return -EFAULT;
> }
>
> /* If their number is silly, that's an error. */
> if (head >= vq->num) {
> vq_err(vq, "Guest says index %u > %u is available",
> head, vq->num);
> - return vq->num;
> + return -EINVAL;
> }
>
> /* When we start there are none of either input nor output. */
> @@ -933,19 +934,19 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> if (i >= vq->num) {
> vq_err(vq, "Desc index is %u > %u, head = %u",
> i, vq->num, head);
> - return vq->num;
> + return -EINVAL;
> }
> if (++found > vq->num) {
> vq_err(vq, "Loop detected: last one at %u "
> "vq size %u head %u\n",
> i, vq->num, head);
> - return vq->num;
> + return -EINVAL;
> }
> ret = copy_from_user(&desc, vq->desc + i, sizeof desc);
> if (ret) {
> vq_err(vq, "Failed to get descriptor: idx %d addr %p\n",
> i, vq->desc + i);
> - return vq->num;
> + return -EFAULT;
> }
> if (desc.flags & VRING_DESC_F_INDIRECT) {
> ret = get_indirect(dev, vq, iov, iov_size,
> @@ -954,7 +955,7 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> if (ret < 0) {
> vq_err(vq, "Failure detected "
> "in indirect descriptor at idx %d\n", i);
> - return vq->num;
> + return ret;
> }
> continue;
> }
> @@ -964,7 +965,7 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> if (ret < 0) {
> vq_err(vq, "Translation failure %d descriptor idx %d\n",
> ret, i);
> - return vq->num;
> + return ret;
> }
> if (desc.flags & VRING_DESC_F_WRITE) {
> /* If this is an input descriptor,
> @@ -981,7 +982,7 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> if (*in_num) {
> vq_err(vq, "Descriptor has out after in: "
> "idx %d\n", i);
> - return vq->num;
> + return -EINVAL;
> }
> *out_num += ret;
> }
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 44591ba..11ee13d 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -120,10 +120,10 @@ long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, unsigned long arg);
> int vhost_vq_access_ok(struct vhost_virtqueue *vq);
> int vhost_log_access_ok(struct vhost_dev *);
>
> -unsigned vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
> - struct iovec iov[], unsigned int iov_count,
> - unsigned int *out_num, unsigned int *in_num,
> - struct vhost_log *log, unsigned int *log_num);
> +int vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
> + struct iovec iov[], unsigned int iov_count,
> + unsigned int *out_num, unsigned int *in_num,
> + struct vhost_log *log, unsigned int *log_num);
> void vhost_discard_vq_desc(struct vhost_virtqueue *);
>
> int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len);
^ permalink raw reply
* Re: [PATCH] vhost: break out of polling loop on error
From: Michael S. Tsirkin @ 2010-06-28 17:25 UTC (permalink / raw)
To: Sridhar Samudrala
Cc: Sridhar Samudrala, Arnd Bergmann, Paul E. McKenney, Juan Quintela,
Rusty Russell, Jes.Sorensen, kraxel, Takuya Yoshikawa, kvm,
virtualization, netdev, linux-kernel
In-Reply-To: <1277745103.23755.2.camel@w-sridhar.beaverton.ibm.com>
On Mon, Jun 28, 2010 at 10:11:43AM -0700, Sridhar Samudrala wrote:
> On Sun, 2010-06-27 at 11:59 +0300, Michael S. Tsirkin wrote:
> > When ring parsing fails, we currently handle this
> > as ring empty condition. This means that we enable
> > kicks and recheck ring empty: if this not empty,
> > we re-start polling which of course will fail again.
> >
> > Instead, let's return a negative error code and stop polling.
>
> One minor comment on error return below. With that change,
>
> Acked-by: Sridhar Samudrala <sri@us.ibm.com>
Right. In fact, we don't really use the return code,
and the generated binary is smaller if we return
-EINVAL always.
So that's what I'll do.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > Dave, I'm sending this out so it can get reviewed.
> > I'll put this on my vhost tree
> > so no need for you to pick this patch directly.
> >
> > drivers/vhost/net.c | 12 ++++++++++--
> > drivers/vhost/vhost.c | 33 +++++++++++++++++----------------
> > drivers/vhost/vhost.h | 8 ++++----
> > 3 files changed, 31 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 0f41c91..54096ee 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -98,7 +98,8 @@ static void tx_poll_start(struct vhost_net *net, struct socket *sock)
> > static void handle_tx(struct vhost_net *net)
> > {
> > struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
> > - unsigned head, out, in, s;
> > + unsigned out, in, s;
> > + int head;
> > struct msghdr msg = {
> > .msg_name = NULL,
> > .msg_namelen = 0,
> > @@ -135,6 +136,9 @@ static void handle_tx(struct vhost_net *net)
> > ARRAY_SIZE(vq->iov),
> > &out, &in,
> > NULL, NULL);
> > + /* On error, stop handling until the next kick. */
> > + if (head < 0)
> > + break;
> > /* Nothing new? Wait for eventfd to tell us they refilled. */
> > if (head == vq->num) {
> > wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> > @@ -192,7 +196,8 @@ static void handle_tx(struct vhost_net *net)
> > 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 out, in, log, s;
> > + int head;
> > struct vhost_log *vq_log;
> > struct msghdr msg = {
> > .msg_name = NULL,
> > @@ -228,6 +233,9 @@ static void handle_rx(struct vhost_net *net)
> > ARRAY_SIZE(vq->iov),
> > &out, &in,
> > vq_log, &log);
> > + /* On error, stop handling until the next kick. */
> > + if (head < 0)
> > + break;
> > /* OK, now we need to know about added descriptors. */
> > if (head == vq->num) {
> > if (unlikely(vhost_enable_notify(vq))) {
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 3b83382..5ccd384 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -873,12 +873,13 @@ static unsigned get_indirect(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> > * number of output then some number of input descriptors, it's actually two
> > * iovecs, but we pack them into one and note how many of each there were.
> > *
> > - * This function returns the descriptor number found, or vq->num (which
> > - * is never a valid descriptor number) if none was found. */
> > -unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> > - struct iovec iov[], unsigned int iov_size,
> > - unsigned int *out_num, unsigned int *in_num,
> > - struct vhost_log *log, unsigned int *log_num)
> > + * This function returns the descriptor number found, or vq->num (which is
> > + * never a valid descriptor number) if none was found. A negative code is
> > + * returned on error. */
> > +int vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> > + struct iovec iov[], unsigned int iov_size,
> > + unsigned int *out_num, unsigned int *in_num,
> > + struct vhost_log *log, unsigned int *log_num)
> > {
> > struct vring_desc desc;
> > unsigned int i, head, found = 0;
> > @@ -890,13 +891,13 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> > if (get_user(vq->avail_idx, &vq->avail->idx)) {
> > vq_err(vq, "Failed to access avail idx at %p\n",
> > &vq->avail->idx);
> > - return vq->num;
> > + return -EFAULT;
> > }
> >
> > if ((u16)(vq->avail_idx - last_avail_idx) > vq->num) {
> > vq_err(vq, "Guest moved used index from %u to %u",
> > last_avail_idx, vq->avail_idx);
> > - return vq->num;
> > + return -EFAULT;
>
> This should be -EINVAL
> > }
> >
> > /* If there's nothing new since last we looked, return invalid. */
> > @@ -912,14 +913,14 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> > vq_err(vq, "Failed to read head: idx %d address %p\n",
> > last_avail_idx,
> > &vq->avail->ring[last_avail_idx % vq->num]);
> > - return vq->num;
> > + return -EFAULT;
> > }
> >
> > /* If their number is silly, that's an error. */
> > if (head >= vq->num) {
> > vq_err(vq, "Guest says index %u > %u is available",
> > head, vq->num);
> > - return vq->num;
> > + return -EINVAL;
> > }
> >
> > /* When we start there are none of either input nor output. */
> > @@ -933,19 +934,19 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> > if (i >= vq->num) {
> > vq_err(vq, "Desc index is %u > %u, head = %u",
> > i, vq->num, head);
> > - return vq->num;
> > + return -EINVAL;
> > }
> > if (++found > vq->num) {
> > vq_err(vq, "Loop detected: last one at %u "
> > "vq size %u head %u\n",
> > i, vq->num, head);
> > - return vq->num;
> > + return -EINVAL;
> > }
> > ret = copy_from_user(&desc, vq->desc + i, sizeof desc);
> > if (ret) {
> > vq_err(vq, "Failed to get descriptor: idx %d addr %p\n",
> > i, vq->desc + i);
> > - return vq->num;
> > + return -EFAULT;
> > }
> > if (desc.flags & VRING_DESC_F_INDIRECT) {
> > ret = get_indirect(dev, vq, iov, iov_size,
> > @@ -954,7 +955,7 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> > if (ret < 0) {
> > vq_err(vq, "Failure detected "
> > "in indirect descriptor at idx %d\n", i);
> > - return vq->num;
> > + return ret;
> > }
> > continue;
> > }
> > @@ -964,7 +965,7 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> > if (ret < 0) {
> > vq_err(vq, "Translation failure %d descriptor idx %d\n",
> > ret, i);
> > - return vq->num;
> > + return ret;
> > }
> > if (desc.flags & VRING_DESC_F_WRITE) {
> > /* If this is an input descriptor,
> > @@ -981,7 +982,7 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
> > if (*in_num) {
> > vq_err(vq, "Descriptor has out after in: "
> > "idx %d\n", i);
> > - return vq->num;
> > + return -EINVAL;
> > }
> > *out_num += ret;
> > }
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 44591ba..11ee13d 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -120,10 +120,10 @@ long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, unsigned long arg);
> > int vhost_vq_access_ok(struct vhost_virtqueue *vq);
> > int vhost_log_access_ok(struct vhost_dev *);
> >
> > -unsigned vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
> > - struct iovec iov[], unsigned int iov_count,
> > - unsigned int *out_num, unsigned int *in_num,
> > - struct vhost_log *log, unsigned int *log_num);
> > +int vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
> > + struct iovec iov[], unsigned int iov_count,
> > + unsigned int *out_num, unsigned int *in_num,
> > + struct vhost_log *log, unsigned int *log_num);
> > void vhost_discard_vq_desc(struct vhost_virtqueue *);
> >
> > int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len);
^ permalink raw reply
* Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
From: Sridhar Samudrala @ 2010-06-28 22:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Aristeu Rozanski, Herbert Xu, Juan Quintela, David S. Miller, kvm,
virtualization, netdev, linux-kernel, ykaul, markmc
In-Reply-To: <20100628100807.GA30685@redhat.com>
On Mon, 2010-06-28 at 13:08 +0300, Michael S. Tsirkin wrote:
> Userspace virtio server has the following hack
> so guests rely on it, and we have to replicate it, too:
>
> Use port number to detect incoming IPv4 DHCP response packets,
> and fill in the checksum for these.
>
> The issue we are solving is that on linux guests, some apps
> that use recvmsg with AF_PACKET sockets, don't know how to
> handle CHECKSUM_PARTIAL;
> The interface to return the relevant information was added
> in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> and older userspace does not use it.
> One important user of recvmsg with AF_PACKET is dhclient,
> so we add a work-around just for DHCP.
>
> Don't bother applying the hack to IPv6 as userspace virtio does not
> have a work-around for that - let's hope guests will do the right
> thing wrt IPv6.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Dave, I'm going to put this patch on the vhost tree,
> no need for you to bother merging it - you'll get
> it with a pull request.
>
>
> drivers/vhost/net.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 43 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index cc19595..03bba6a 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -24,6 +24,10 @@
> #include <linux/if_tun.h>
> #include <linux/if_macvlan.h>
>
> +#include <linux/ip.h>
> +#include <linux/udp.h>
> +#include <linux/netdevice.h>
> +
> #include <net/sock.h>
>
> #include "vhost.h"
> @@ -186,6 +190,44 @@ static void handle_tx(struct vhost_net *net)
> unuse_mm(net->dev.mm);
> }
>
> +static int peek_head(struct sock *sk)
This routine is doing more than just peeking the head of sk's receive
queue. May be this should be named similar to what qemu calls
'work_around_broken_dhclient()'
> +{
> + struct sk_buff *skb;
> +
> + lock_sock(sk);
> + skb = skb_peek(&sk->sk_receive_queue);
> + if (unlikely(!skb)) {
> + release_sock(sk);
> + return 0;
> + }
> + /* Userspace virtio server has the following hack so
> + * guests rely on it, and we have to replicate it, too: */
> + /* Use port number to detect incoming IPv4 DHCP response packets,
> + * and fill in the checksum. */
> +
> + /* The issue we are solving is that on linux guests, some apps
> + * that use recvmsg with AF_PACKET sockets, don't know how to
> + * handle CHECKSUM_PARTIAL;
> + * The interface to return the relevant information was added in
> + * 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> + * and older userspace does not use it.
> + * One important user of recvmsg with AF_PACKET is dhclient,
> + * so we add a work-around just for DHCP. */
> + if (skb->ip_summed == CHECKSUM_PARTIAL &&
> + skb_headlen(skb) >= skb_transport_offset(skb) +
> + sizeof(struct udphdr) &&
> + udp_hdr(skb)->dest == htons(68) &&
> + skb_network_header_len(skb) >= sizeof(struct iphdr) &&
> + ip_hdr(skb)->protocol == IPPROTO_UDP &&
> + skb->protocol == htons(ETH_P_IP)) {
Isn't it more logical to check for skb->protocol, followed by ip_hdr and
then udp_hdr?
> + skb_checksum_help(skb);
> + /* Restore ip_summed value: tun passes it to user. */
> + skb->ip_summed = CHECKSUM_PARTIAL;
> + }
> + release_sock(sk);
> + return 1;
> +}
> +
> /* 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)
> @@ -222,7 +264,7 @@ static void handle_rx(struct vhost_net *net)
> vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> vq->log : NULL;
>
> - for (;;) {
> + while (peek_head(sock->sk)) {
> head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> ARRAY_SIZE(vq->iov),
> &out, &in,
^ permalink raw reply
* Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
From: Michael S. Tsirkin @ 2010-06-29 6:55 UTC (permalink / raw)
To: Sridhar Samudrala
Cc: Aristeu Rozanski, Herbert Xu, Juan Quintela, David S. Miller, kvm,
virtualization, netdev, linux-kernel, ykaul, markmc
In-Reply-To: <1277763581.23755.16.camel@w-sridhar.beaverton.ibm.com>
On Mon, Jun 28, 2010 at 03:19:41PM -0700, Sridhar Samudrala wrote:
> On Mon, 2010-06-28 at 13:08 +0300, Michael S. Tsirkin wrote:
> > Userspace virtio server has the following hack
> > so guests rely on it, and we have to replicate it, too:
> >
> > Use port number to detect incoming IPv4 DHCP response packets,
> > and fill in the checksum for these.
> >
> > The issue we are solving is that on linux guests, some apps
> > that use recvmsg with AF_PACKET sockets, don't know how to
> > handle CHECKSUM_PARTIAL;
> > The interface to return the relevant information was added
> > in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> > and older userspace does not use it.
> > One important user of recvmsg with AF_PACKET is dhclient,
> > so we add a work-around just for DHCP.
> >
> > Don't bother applying the hack to IPv6 as userspace virtio does not
> > have a work-around for that - let's hope guests will do the right
> > thing wrt IPv6.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > Dave, I'm going to put this patch on the vhost tree,
> > no need for you to bother merging it - you'll get
> > it with a pull request.
> >
> >
> > drivers/vhost/net.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > 1 files changed, 43 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index cc19595..03bba6a 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -24,6 +24,10 @@
> > #include <linux/if_tun.h>
> > #include <linux/if_macvlan.h>
> >
> > +#include <linux/ip.h>
> > +#include <linux/udp.h>
> > +#include <linux/netdevice.h>
> > +
> > #include <net/sock.h>
> >
> > #include "vhost.h"
> > @@ -186,6 +190,44 @@ static void handle_tx(struct vhost_net *net)
> > unuse_mm(net->dev.mm);
> > }
> >
> > +static int peek_head(struct sock *sk)
>
> This routine is doing more than just peeking the head of sk's receive
> queue. May be this should be named similar to what qemu calls
> 'work_around_broken_dhclient()'
> > +{
> > + struct sk_buff *skb;
> > +
> > + lock_sock(sk);
> > + skb = skb_peek(&sk->sk_receive_queue);
> > + if (unlikely(!skb)) {
> > + release_sock(sk);
> > + return 0;
> > + }
> > + /* Userspace virtio server has the following hack so
> > + * guests rely on it, and we have to replicate it, too: */
> > + /* Use port number to detect incoming IPv4 DHCP response packets,
> > + * and fill in the checksum. */
> > +
> > + /* The issue we are solving is that on linux guests, some apps
> > + * that use recvmsg with AF_PACKET sockets, don't know how to
> > + * handle CHECKSUM_PARTIAL;
> > + * The interface to return the relevant information was added in
> > + * 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> > + * and older userspace does not use it.
> > + * One important user of recvmsg with AF_PACKET is dhclient,
> > + * so we add a work-around just for DHCP. */
> > + if (skb->ip_summed == CHECKSUM_PARTIAL &&
> > + skb_headlen(skb) >= skb_transport_offset(skb) +
> > + sizeof(struct udphdr) &&
> > + udp_hdr(skb)->dest == htons(68) &&
> > + skb_network_header_len(skb) >= sizeof(struct iphdr) &&
> > + ip_hdr(skb)->protocol == IPPROTO_UDP &&
> > + skb->protocol == htons(ETH_P_IP)) {
>
> Isn't it more logical to check for skb->protocol, followed by ip_hdr and
> then udp_hdr?
Yes, but then we'll only exit after checking them all.
My way we'll almost always exit after port check.
> > + skb_checksum_help(skb);
> > + /* Restore ip_summed value: tun passes it to user. */
> > + skb->ip_summed = CHECKSUM_PARTIAL;
> > + }
> > + release_sock(sk);
> > + return 1;
> > +}
> > +
> > /* 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)
> > @@ -222,7 +264,7 @@ static void handle_rx(struct vhost_net *net)
> > vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> > vq->log : NULL;
> >
> > - for (;;) {
> > + while (peek_head(sock->sk)) {
> > head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> > ARRAY_SIZE(vq->iov),
> > &out, &in,
^ permalink raw reply
* Re: [RFC] virtio: Support releasing lock during kick
From: Stefan Hajnoczi @ 2010-06-29 7:08 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Stefan Hajnoczi, kvm, virtualization
In-Reply-To: <20100628155505.GB4717@amt.cnet>
On Mon, Jun 28, 2010 at 4:55 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> On Wed, Jun 23, 2010 at 10:24:02PM +0100, Stefan Hajnoczi wrote:
>> The virtio block device holds a lock during I/O request processing.
>> Kicking the virtqueue while the lock is held results in long lock hold
>> times and increases contention for the lock.
>>
>> This patch modifies virtqueue_kick() to optionally release a lock while
>> notifying the host. Virtio block is modified to pass in its lock. This
>> allows other vcpus to queue I/O requests during the time spent servicing
>> the virtqueue notify in the host.
>>
>> The virtqueue_kick() function is modified to know about locking because
>> it changes the state of the virtqueue and should execute with the lock
>> held (it would not be correct for virtio block to release the lock
>> before calling virtqueue_kick()).
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> ---
>> I am not yet 100% happy with this patch which aims to reduce guest CPU
>> consumption related to vblk->lock contention. Although this patch reduces
>> wait/hold times it does not affect I/O throughput or guest CPU utilization.
>> More investigation is required to get to the bottom of why guest CPU
>> utilization does not decrease when a lock bottleneck has been removed.
>>
>> Performance figures:
>>
>> Host: 2.6.34 upstream kernel, qemu-kvm-0.12.4 if=virtio,cache=none
>> Guest: 2.6.35-rc3-kvm.git upstream kernel
>> Storage: 12 disks as striped LVM volume
>> Benchmark: 4 concurrent dd bs=4k iflag=direct
>>
>> Lockstat data for &vblk->lock:
>>
>> test con-bounces contentions waittime-min waittime-max waittime-total
>> unmodified 7097 7108 0.31 956.09 161165.4
>> patched 11484 11550 0.30 411.80 50245.83
>>
>> The maximum wait time went down by 544.29 us (-57%) and the total wait time
>> decreased by 69%. This shows that the virtqueue kick is indeed hogging the
>> lock.
>>
>> The patched version actually has higher contention than the unmodified version.
>> I think the reason for this is that each virtqueue kick now includes a short
>> release and reacquire. This short release gives other vcpus a chance to
>> acquire the lock and progress, hence more contention but overall better wait
>> time numbers.
>>
>> name acq-bounces acquisitions holdtime-min holdtime-max holdtime-total
>> unmodified 10771 5038346 0.00 3271.81 59016905.47
>> patched 31594 5857813 0.00 219.76 24104915.55
>>
>> Here we see the full impact of this patch: hold time reduced to 219.76 us
>> (-93%).
>>
>> Again the acquisitions have increased since we're now doing an extra
>> unlock+lock per virtqueue kick.
>>
>> Testing, ideas, and comments appreciated.
>>
>> drivers/block/virtio_blk.c | 2 +-
>> drivers/char/hw_random/virtio-rng.c | 2 +-
>> drivers/char/virtio_console.c | 6 +++---
>> drivers/net/virtio_net.c | 6 +++---
>> drivers/virtio/virtio_balloon.c | 6 +++---
>> drivers/virtio/virtio_ring.c | 13 +++++++++++--
>> include/linux/virtio.h | 3 ++-
>> net/9p/trans_virtio.c | 2 +-
>> 8 files changed, 25 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 258bc2a..de033bf 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -187,7 +187,7 @@ static void do_virtblk_request(struct request_queue *q)
>> }
>>
>> if (issued)
>> - virtqueue_kick(vblk->vq);
>> + virtqueue_kick(vblk->vq, &vblk->lock);
>> }
>>
>> static void virtblk_prepare_flush(struct request_queue *q, struct request *req)
>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>> index 75f1cbd..852d563 100644
>> --- a/drivers/char/hw_random/virtio-rng.c
>> +++ b/drivers/char/hw_random/virtio-rng.c
>> @@ -49,7 +49,7 @@ static void register_buffer(u8 *buf, size_t size)
>> if (virtqueue_add_buf(vq, &sg, 0, 1, buf) < 0)
>> BUG();
>>
>> - virtqueue_kick(vq);
>> + virtqueue_kick(vq, NULL);
>> }
>>
>> static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
>> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
>> index 942a982..677714d 100644
>> --- a/drivers/char/virtio_console.c
>> +++ b/drivers/char/virtio_console.c
>> @@ -328,7 +328,7 @@ static int add_inbuf(struct virtqueue *vq, struct port_buffer *buf)
>> sg_init_one(sg, buf->buf, buf->size);
>>
>> ret = virtqueue_add_buf(vq, sg, 0, 1, buf);
>> - virtqueue_kick(vq);
>> + virtqueue_kick(vq, NULL);
>> return ret;
>> }
>>
>> @@ -400,7 +400,7 @@ static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
>>
>> sg_init_one(sg, &cpkt, sizeof(cpkt));
>> if (virtqueue_add_buf(vq, sg, 1, 0, &cpkt) >= 0) {
>> - virtqueue_kick(vq);
>> + virtqueue_kick(vq, NULL);
>> while (!virtqueue_get_buf(vq, &len))
>> cpu_relax();
>> }
>> @@ -444,7 +444,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
>> ret = virtqueue_add_buf(out_vq, sg, 1, 0, in_buf);
>>
>> /* Tell Host to go! */
>> - virtqueue_kick(out_vq);
>> + virtqueue_kick(out_vq, NULL);
>>
>> if (ret < 0) {
>> in_count = 0;
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 1edb7a6..6a837b3 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -433,7 +433,7 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
>> } while (err > 0);
>> if (unlikely(vi->num > vi->max))
>> vi->max = vi->num;
>> - virtqueue_kick(vi->rvq);
>> + virtqueue_kick(vi->rvq, NULL);
>> return !oom;
>> }
>>
>> @@ -581,7 +581,7 @@ again:
>> }
>> return NETDEV_TX_BUSY;
>> }
>> - virtqueue_kick(vi->svq);
>> + virtqueue_kick(vi->svq, NULL);
>>
>> /* Don't wait up for transmitted skbs to be freed. */
>> skb_orphan(skb);
>> @@ -680,7 +680,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
>>
>> BUG_ON(virtqueue_add_buf(vi->cvq, sg, out, in, vi) < 0);
>>
>> - virtqueue_kick(vi->cvq);
>> + virtqueue_kick(vi->cvq, NULL);
>>
>> /*
>> * Spin for a response, the kick causes an ioport write, trapping
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index 0f1da45..c9c5c4a 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -91,7 +91,7 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
>> /* We should always be able to add one buffer to an empty queue. */
>> if (virtqueue_add_buf(vq, &sg, 1, 0, vb) < 0)
>> BUG();
>> - virtqueue_kick(vq);
>> + virtqueue_kick(vq, NULL);
>>
>> /* When host has read buffer, this completes via balloon_ack */
>> wait_for_completion(&vb->acked);
>> @@ -223,7 +223,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
>> sg_init_one(&sg, vb->stats, sizeof(vb->stats));
>> if (virtqueue_add_buf(vq, &sg, 1, 0, vb) < 0)
>> BUG();
>> - virtqueue_kick(vq);
>> + virtqueue_kick(vq, NULL);
>> }
>>
>> static void virtballoon_changed(struct virtio_device *vdev)
>> @@ -316,7 +316,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
>> sg_init_one(&sg, vb->stats, sizeof vb->stats);
>> if (virtqueue_add_buf(vb->stats_vq, &sg, 1, 0, vb) < 0)
>> BUG();
>> - virtqueue_kick(vb->stats_vq);
>> + virtqueue_kick(vb->stats_vq, NULL);
>> }
>>
>> vb->thread = kthread_run(balloon, vb, "vballoon");
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index 1ca8890..163a237 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -236,7 +236,7 @@ add_head:
>> }
>> EXPORT_SYMBOL_GPL(virtqueue_add_buf_gfp);
>>
>> -void virtqueue_kick(struct virtqueue *_vq)
>> +void virtqueue_kick(struct virtqueue *_vq, spinlock_t *lock)
>> {
>> struct vring_virtqueue *vq = to_vvq(_vq);
>> START_USE(vq);
>> @@ -250,10 +250,19 @@ void virtqueue_kick(struct virtqueue *_vq)
>> /* Need to update avail index before checking if we should notify */
>> virtio_mb();
>>
>> - if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY))
>> + if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY)) {
>> + /* Release lock while doing the kick because the guest should
>> + * not exit with the lock held. */
>> + if (lock)
>> + spin_unlock(lock);
>> +
>> /* Prod other side to tell it about changes. */
>> vq->notify(&vq->vq);
>>
>> + if (lock)
>> + spin_lock(lock);
>> + }
>> +
>
> The queue lock requires irq's to be disabled in addition to lock
> acquision.
Irqs are already disabled by the block layer when acquiring the queue
lock. During the virtqueue kick irqs stay disabled on the local CPU
but other CPUs may acquire the lock and make progress.
Is it incorrect to have the following pattern?
spin_lock_irqsave(q->queue_lock);
spin_unlock(q->queue_lock);
spin_lock(q->queue_lock);
spin_unlock_irqrestore(q->queue_lock);
Stefan
^ permalink raw reply
* Re: [RFC] virtio: Support releasing lock during kick
From: Avi Kivity @ 2010-06-29 7:12 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Stefan Hajnoczi, kvm, virtualization
In-Reply-To: <AANLkTikhPJnw81hhh3PQGSwv7Hkd56YMlOZQM0N-fTLp@mail.gmail.com>
On 06/29/2010 10:08 AM, Stefan Hajnoczi wrote:
>
> Is it incorrect to have the following pattern?
> spin_lock_irqsave(q->queue_lock);
> spin_unlock(q->queue_lock);
> spin_lock(q->queue_lock);
> spin_unlock_irqrestore(q->queue_lock);
>
Perfectly legitimate. spin_lock_irqsave() is equivalent to
local_irq_save() followed by spin_lock() (with the potential
optimization that we can service interrupts while spinning).
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply
* Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
From: David Miller @ 2010-06-29 7:36 UTC (permalink / raw)
To: mst
Cc: arozansk, herbert.xu, quintela, kvm, virtualization, netdev,
linux-kernel, ykaul, markmc
In-Reply-To: <20100628100807.GA30685@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 28 Jun 2010 13:08:07 +0300
> Userspace virtio server has the following hack
> so guests rely on it, and we have to replicate it, too:
>
> Use port number to detect incoming IPv4 DHCP response packets,
> and fill in the checksum for these.
>
> The issue we are solving is that on linux guests, some apps
> that use recvmsg with AF_PACKET sockets, don't know how to
> handle CHECKSUM_PARTIAL;
> The interface to return the relevant information was added
> in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> and older userspace does not use it.
> One important user of recvmsg with AF_PACKET is dhclient,
> so we add a work-around just for DHCP.
>
> Don't bother applying the hack to IPv6 as userspace virtio does not
> have a work-around for that - let's hope guests will do the right
> thing wrt IPv6.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Yikes, this is awful too.
Nothing in the kernel should be mucking around with procotol packets
like this by default. In particular, what the heck does port 67 mean?
Locally I can use it for whatever I want for my own purposes, I don't
have to follow the conventions for service ports as specified by the
IETF.
But I can't have the packet checksum state be left alone for port 67
traffic on a box using virtio because you have this hack there.
And yes it's broken on machines using the qemu thing, but at least the
hack there is restricted to userspace.
I really don't want anything in the kernel that looks like this.
These applications are broken, and we've provided a way for them to
work properly. What's the point of having fixed applications if
all of these hacks grow like fungus over every virtualization transport?
It just means that people won't fix the apps, since they don't have
to. There is no incentive, and the mechanism we created to properly
handle this loses it's value.
At best, you can write a netfilter module that mucks up the packet
checksum state in these situations. At least in that case, you can
make it generic (it mangles iff a packet matches a certain rule,
so for your virtio guests you'd make it match for DHCP frames) instead
of being some hard-coded DHCP thing by design.
And since this is so cleanly seperated and portable you don't even
need to push it upstream. It's a temporary workaround for a temporary
problem. You can just delete it as soon as the majority of guests
have the fixed dhcp. The qemu crap should disappear similarly.
^ permalink raw reply
* Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
From: Michael S. Tsirkin @ 2010-06-29 13:04 UTC (permalink / raw)
To: David Miller
Cc: arozansk, herbert.xu, quintela, kvm, virtualization, netdev,
linux-kernel, ykaul, markmc
In-Reply-To: <20100629.003647.214219303.davem@davemloft.net>
On Tue, Jun 29, 2010 at 12:36:47AM -0700, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Mon, 28 Jun 2010 13:08:07 +0300
>
> > Userspace virtio server has the following hack
> > so guests rely on it, and we have to replicate it, too:
> >
> > Use port number to detect incoming IPv4 DHCP response packets,
> > and fill in the checksum for these.
> >
> > The issue we are solving is that on linux guests, some apps
> > that use recvmsg with AF_PACKET sockets, don't know how to
> > handle CHECKSUM_PARTIAL;
> > The interface to return the relevant information was added
> > in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> > and older userspace does not use it.
> > One important user of recvmsg with AF_PACKET is dhclient,
> > so we add a work-around just for DHCP.
> >
> > Don't bother applying the hack to IPv6 as userspace virtio does not
> > have a work-around for that - let's hope guests will do the right
> > thing wrt IPv6.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Yikes, this is awful too.
>
> Nothing in the kernel should be mucking around with procotol packets
> like this by default. In particular, what the heck does port 67 mean?
> Locally I can use it for whatever I want for my own purposes, I don't
> have to follow the conventions for service ports as specified by the
> IETF.
>
> But I can't have the packet checksum state be left alone for port 67
> traffic on a box using virtio because you have this hack there.
>
> And yes it's broken on machines using the qemu thing, but at least the
> hack there is restricted to userspace.
Yes, and I think it was a mistake to add the hack there. This is what
prevented applications from using the new interface in the 3 years
since it was first introduced.
> I really don't want anything in the kernel that looks like this.
>
> These applications are broken, and we've provided a way for them to
> work properly. What's the point of having fixed applications if
> all of these hacks grow like fungus over every virtualization transport?
>
> It just means that people won't fix the apps, since they don't have
> to. There is no incentive, and the mechanism we created to properly
> handle this loses it's value.
>
> At best, you can write a netfilter module that mucks up the packet
> checksum state in these situations. At least in that case, you can
> make it generic (it mangles iff a packet matches a certain rule,
> so for your virtio guests you'd make it match for DHCP frames) instead
> of being some hard-coded DHCP thing by design.
Nod.
One question on implementation:
why does skb_checksum_help set the checksum state to
CHECKSUM_NONE? Shouldn't it be CHECKSUM_COMPLETE?
> And since this is so cleanly seperated and portable you don't even
> need to push it upstream. It's a temporary workaround for a temporary
> problem. You can just delete it as soon as the majority of guests
> have the fixed dhcp. The qemu crap should disappear similarly.
Since using the module involves updating the management tools
as well, if we go down this route it will be much less painful
for everyone to do push it upstream.
--
MST
^ permalink raw reply
* Hypervisor detection from within a Linux VM
From: Chetan Loke @ 2010-06-29 21:25 UTC (permalink / raw)
To: Linux Kernel Mailing List, virtualization, kvm, chetanloke
Hello,
Requirement:
I have the need to support my apps(running on a Linux VM) on different
*nix hypervisors(ESX/Xen etc). I need to know on which hypervisor my
app is running. I read the CPUID usage thread -
http://thread.gmane.org/gmane.comp.emulators.kvm.devel/22643 but to be
honest in the end I looked at
http://lxr.linux.no/#linux+v2.6.34/arch/x86/kernel/cpu/vmware.c#L88
The vmware_platform() detection code is straight forward.
Current-hack:
As a quick hack we just grep lspci for VMware's pci-ids.
Solution:
I can write a bare minimal driver, check the cpu-id as VMware's
balloon driver does and then emit a proc/sysfs node. The setup
packages and the apps can then check for this node-string.I'm
currently working on ESX and I am hoping that this thin-driver will
work.
Question:
Q1)Is it possible to get this functionality as part of the stock
kernel or is that a bad idea? I suspect there could be other
users/apps who would need to know what *nix hypervisor(or a
non-virtualized environment) they are
running on?
Q2)If this is not the right approach then can someone please suggest
another approach?
Regards
Chetan Loke
^ permalink raw reply
* Re: Hypervisor detection from within a Linux VM
From: Anthony Liguori @ 2010-06-29 21:44 UTC (permalink / raw)
To: Chetan Loke; +Cc: Linux Kernel Mailing List, kvm, virtualization
In-Reply-To: <AANLkTinl3ighc51F5V4YP7fuOSscAA_qcynYMtM5j5wk@mail.gmail.com>
On 06/29/2010 04:25 PM, Chetan Loke wrote:
> Hello,
>
>
> Requirement:
> I have the need to support my apps(running on a Linux VM) on different
> *nix hypervisors(ESX/Xen etc). I need to know on which hypervisor my
> app is running. I read the CPUID usage thread -
> http://thread.gmane.org/gmane.comp.emulators.kvm.devel/22643 but to be
> honest in the end I looked at
> http://lxr.linux.no/#linux+v2.6.34/arch/x86/kernel/cpu/vmware.c#L88
> The vmware_platform() detection code is straight forward.
>
> Current-hack:
> As a quick hack we just grep lspci for VMware's pci-ids.
>
> Solution:
> I can write a bare minimal driver, check the cpu-id as VMware's
> balloon driver does and then emit a proc/sysfs node. The setup
> packages and the apps can then check for this node-string.I'm
> currently working on ESX and I am hoping that this thin-driver will
> work.
>
It can be done entirely in userspace. Take a look at virt-what:
http://people.redhat.com/~rjones/virt-what/
> Question:
> Q1)Is it possible to get this functionality as part of the stock
> kernel or is that a bad idea? I suspect there could be other
> users/apps who would need to know what *nix hypervisor(or a
> non-virtualized environment) they are
> running on?
> Q2)If this is not the right approach then can someone please suggest
> another approach?
>
It might be reasonable to list the hypervisor signature as a field in
/proc/cpuinfo. There's also a /sys/hypervisor where such information
could go.
Regards,
Anthony Liguori
> Regards
> Chetan Loke
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [PATCH] virt-what.in: Added VMware virt detection using cpuid in wrapper script
From: Chetan Loke @ 2010-06-29 23:29 UTC (permalink / raw)
To: Anthony Liguori, rjones, rjones
Cc: Tzy-Jye Daniel Lin, chetanloke, Linux Kernel Mailing List, kvm,
virtualization
Brief discussion thread - http://lkml.org/lkml/2010/6/29/355
Added VMware detection via cpuid.Mimic'd VMware's balloon driver's init-time
check(cpuid followed by dmi-checks). Moved the existing logic into a simple
function to achieve that.
Tested virt-what.in(ran as a script) and virt-what-cpuid-helper within
a Linux guest on ESX. Thanks.
Signed-off-by: Chetan Loke <loke.c@alumni.neu.edu>
---
virt-what.in | 150 ++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 93 insertions(+), 57 deletions(-)
--- virt-what.in.orig 2010-06-29 18:42:20.166647585 -0400
+++ virt-what.in 2010-06-29 18:59:54.765647739 -0400
@@ -25,9 +25,19 @@
#
# The following resources were useful in writing this script:
# . http://www.dmo.ca/blog/20080530151107
+#
+#
+# Added VMware detection using cpuid.
+# Added functions to check the environment type
+# 06/29/2010 - Chetan Loke <loke.c@alumni.neu.edu>
+#
+
VERSION="@VERSION@"
+cpuid=
+is_xen=1
+
function fail {
echo "virt-what: $1"
exit 1
@@ -38,7 +48,8 @@
echo "Options:"
echo " --help Display this help"
echo " --version Display version and exit"
- exit 0
+ # exit after printing
+ exit 1
}
# Handle the command line arguments, if any.
@@ -70,74 +81,99 @@
exec_prefix=@exec_prefix@
PATH=@libexecdir@:/sbin:/usr/sbin:$PATH
-# Check for various products in the BIOS information.
+detect_virt_env_using_misc_info() {
-dmi=`dmidecode 2>&1`
+ # Check for various products in the BIOS information.
-if echo "$dmi" | grep -q 'Manufacturer: VMware'; then
- echo vmware
-fi
+ dmi=`dmidecode 2>&1`
-if echo "$dmi" | grep -q 'Manufacturer: Microsoft Corporation'; then
- echo virtualpc
-fi
+ if echo "$dmi" | grep -q 'Manufacturer: VMware'; then
+ echo vmware
+ exit 0
+ fi
-# Check for VirtualBox.
-# Added by Laurent Léonard.
-if echo "$dmi" | grep -q 'Manufacturer: innotek GmbH'; then
- echo virtualbox
-fi
+ if echo "$dmi" | grep -q 'Manufacturer: Microsoft Corporation'; then
+ echo virtualpc
+ exit 0
+ fi
-# Check for OpenVZ / Virtuozzo.
-# Added by Evgeniy Sokolov.
-# /proc/vz - always exists if OpenVZ kernel is running (inside and outside
-# container)
-# /proc/bc - exists on node, but not inside container.
+ # Check for VirtualBox.
+ # Added by Laurent Léonard.
+ if echo "$dmi" | grep -q 'Manufacturer: innotek GmbH'; then
+ echo virtualbox
+ exit 0
+ fi
-if [ -d /proc/vz -a ! -d /proc/bc ]; then
- echo openvz
-fi
+ # Check for OpenVZ / Virtuozzo.
+ # Added by Evgeniy Sokolov.
+ # /proc/vz - always exists if OpenVZ kernel is running (inside
and outside
+ # container)
+ # /proc/bc - exists on node, but not inside container.
+
+ if [ -d /proc/vz -a ! -d /proc/bc ]; then
+ echo openvz
+ exit 0
+ fi
-# Check for UML.
-# Added by Laurent Léonard.
-if grep -q 'UML' /proc/cpuinfo; then
- echo uml
-fi
+ # Check for UML.
+ # Added by Laurent Léonard.
+ if grep -q 'UML' /proc/cpuinfo; then
+ echo uml
+ exit 0
+ fi
+
+ exit 1
+}
# To tell if it is Xen and KVM HVM (fully virtualized) we can use this
# helper C program.
-cpuid=`virt-what-cpuid-helper`
-
-# Check for Xen.
+detect_virt_env_using_cpuid () {
+ cpuid=`virt-what-cpuid-helper`
+
+ # Check for Xen.
+
+ if [ "$cpuid" = "XenVMMXenVMM" ]; then
+ echo xen; echo xen-hvm
+ exit 0
+ elif [ -f /proc/xen/privcmd ]; then
+ echo xen; echo xen-dom0
+ exit 0
+ elif [ -f /proc/xen/capabilities ]; then
+ echo xen; echo xen-domU
+ exit 0
+ elif [ -d /proc/xen ]; then
+ # This directory can be present when Xen paravirt drivers are
+ # installed, even on baremetal. Don't confuse people by
+ # printing anything.
+ :
+ fi
-if [ "$cpuid" = "XenVMMXenVMM" ]; then
- echo xen; echo xen-hvm
- is_xen=1
-elif [ -f /proc/xen/privcmd ]; then
- echo xen; echo xen-dom0
- is_xen=1
-elif [ -f /proc/xen/capabilities ]; then
- echo xen; echo xen-domU
- is_xen=1
-elif [ -d /proc/xen ]; then
- # This directory can be present when Xen paravirt drivers are
- # installed, even on baremetal. Don't confuse people by
- # printing anything.
- :
-fi
+ # Check for QEMU/KVM.
-# Check for QEMU/KVM.
+ if [ ! "$is_xen" ]; then
+ # Disable this test if we know this is Xen already, because Xen
+ # uses QEMU for its device model.
+
+ if grep -q 'QEMU' /proc/cpuinfo; then
+ if [ "$cpuid" = "KVMKVMKVM" ]; then
+ echo kvm
+ exit 0
+ else
+ echo qemu
+ exit 0
+ fi
+ fi
+ fi
-if [ ! "$is_xen" ]; then
- # Disable this test if we know this is Xen already, because Xen
- # uses QEMU for its device model.
-
- if grep -q 'QEMU' /proc/cpuinfo; then
- if [ "$cpuid" = "KVMKVMKVM" ]; then
- echo kvm
- else
- echo qemu
+ # Check for VMware
+ if [ "$cpuid" = "VMwareVMware" ]; then
+ echo vmware
+ exit 0
fi
- fi
-fi
+}
+
+
+detect_virt_env_using_cpuid
+# If the above fails then call the next function
+detect_virt_env_using_misc_info
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: Hypervisor detection from within a Linux VM
From: Chetan Loke @ 2010-06-29 23:41 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Linux Kernel Mailing List, kvm, virtualization
In-Reply-To: <4C2A6921.1010405@codemonkey.ws>
On Tue, Jun 29, 2010 at 5:44 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 06/29/2010 04:25 PM, Chetan Loke wrote:
> It can be done entirely in userspace. Take a look at virt-what:
>
> http://people.redhat.com/~rjones/virt-what/
>
Just posted a patch for virt-what.in which can be used as a wrapper
script. I will be using that for now.
>> Question:
>> Q1)Is it possible to get this functionality as part of the stock
>> kernel or is that a bad idea? I suspect there could be other
>> users/apps who would need to know what *nix hypervisor(or a
>> non-virtualized environment) they are
>> running on?
>
> It might be reasonable to list the hypervisor signature as a field in
> /proc/cpuinfo. There's also a /sys/hypervisor where such information could
> go.
>
I like the idea of spitting out a signature under /proc/cpuinfo
primarily because everyone is familiar with that node.
> Regards,
> Anthony Liguori
Regards
Chetan Loke
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
From: David Miller @ 2010-06-30 21:30 UTC (permalink / raw)
To: mst
Cc: arozansk, herbert.xu, quintela, kvm, virtualization, netdev,
linux-kernel, ykaul, markmc
In-Reply-To: <20100629130439.GD3603@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 29 Jun 2010 16:04:39 +0300
> Since using the module involves updating the management tools
> as well, if we go down this route it will be much less painful
> for everyone to do push it upstream.
Ok, you can make your case to Patrick McHardy and if he'll merge
it into his netfilter GIT tree I guess I'll have to take it :)
^ permalink raw reply
* Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
From: Anthony Liguori @ 2010-06-30 22:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Miller, arozansk, herbert.xu, quintela, kvm, virtualization,
netdev, linux-kernel, ykaul, markmc
In-Reply-To: <20100629130439.GD3603@redhat.com>
On 06/29/2010 08:04 AM, Michael S. Tsirkin wrote:
> On Tue, Jun 29, 2010 at 12:36:47AM -0700, David Miller wrote:
>
>> From: "Michael S. Tsirkin"<mst@redhat.com>
>> Date: Mon, 28 Jun 2010 13:08:07 +0300
>>
>>
>>> Userspace virtio server has the following hack
>>> so guests rely on it, and we have to replicate it, too:
>>>
>>> Use port number to detect incoming IPv4 DHCP response packets,
>>> and fill in the checksum for these.
>>>
>>> The issue we are solving is that on linux guests, some apps
>>> that use recvmsg with AF_PACKET sockets, don't know how to
>>> handle CHECKSUM_PARTIAL;
>>> The interface to return the relevant information was added
>>> in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
>>> and older userspace does not use it.
>>> One important user of recvmsg with AF_PACKET is dhclient,
>>> so we add a work-around just for DHCP.
>>>
>>> Don't bother applying the hack to IPv6 as userspace virtio does not
>>> have a work-around for that - let's hope guests will do the right
>>> thing wrt IPv6.
>>>
>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>>
>> Yikes, this is awful too.
>>
>> Nothing in the kernel should be mucking around with procotol packets
>> like this by default. In particular, what the heck does port 67 mean?
>> Locally I can use it for whatever I want for my own purposes, I don't
>> have to follow the conventions for service ports as specified by the
>> IETF.
>>
>> But I can't have the packet checksum state be left alone for port 67
>> traffic on a box using virtio because you have this hack there.
>>
>> And yes it's broken on machines using the qemu thing, but at least the
>> hack there is restricted to userspace.
>>
> Yes, and I think it was a mistake to add the hack there. This is what
> prevented applications from using the new interface in the 3 years
> since it was first introduced.
>
It's far more complicated than that. dhclient is part of ISC's DHCP
package. They do not have a public SCM and instead require you to join
their Software Guild to get access to it.
This problem was identified in one distribution and the patch was pushed
upstream but because they did not have a public SCM, most other
distributions did not see the fix until it appeared in a release. ISC
has a pretty long release cycle historically.
ISC's had the fix for a long time but there was a 3-year gap in their
releases and since their SCM isn't public, users are stuck with the last
release.
This hack makes sense in QEMU as we have a few hacks like this to fix
broken guests. A primary use of virtualization is to run old
applications so it makes sense for us to do that.
I don't think it makes sense for vhost to do this. These guests are so
old that they don't have the requisite features to achieve really high
performance anyway.
I've always thought making vhost totally transparent was a bad idea and
this is one of the reasons. We can do a lot of ugly things in userspace
that we shouldn't be doing in the kernel.
Regards,
Anthony Liguori
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox