* [PATCH 01/11] Revert "virtio: disable multiport console support."
From: Amit Shah @ 2010-04-08 14:49 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Juan Quintela, Michael S. Tsirkin
In-Reply-To: <1270738180-21170-1-git-send-email-amit.shah@redhat.com>
This reverts commit b7a413015d2986edf020fba765c906cc9cbcbfc9.
Multiport support was disabled for 2.6.34 because we wanted to introduce
a new ABI and since we didn't have any released kernel with the older
ABI and were out of the merge window, it didn't make sense keeping the
older ABI around.
Now we revert the patch disabling multiport and rework the ABI in the
following patches.
---
drivers/char/virtio_console.c | 49 ++++++---------------------------------
include/linux/virtio_console.h | 23 ++++++++++++++++++
2 files changed, 31 insertions(+), 41 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 196428c..86e9011 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -33,35 +33,6 @@
#include <linux/workqueue.h>
#include "hvc_console.h"
-/* Moved here from .h file in order to disable MULTIPORT. */
-#define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */
-
-struct virtio_console_multiport_conf {
- struct virtio_console_config config;
- /* max. number of ports this device can hold */
- __u32 max_nr_ports;
- /* number of ports added so far */
- __u32 nr_ports;
-} __attribute__((packed));
-
-/*
- * A message that's passed between the Host and the Guest for a
- * particular port.
- */
-struct virtio_console_control {
- __u32 id; /* Port number */
- __u16 event; /* The kind of control event (see below) */
- __u16 value; /* Extra information for the key */
-};
-
-/* Some events for control messages */
-#define VIRTIO_CONSOLE_PORT_READY 0
-#define VIRTIO_CONSOLE_CONSOLE_PORT 1
-#define VIRTIO_CONSOLE_RESIZE 2
-#define VIRTIO_CONSOLE_PORT_OPEN 3
-#define VIRTIO_CONSOLE_PORT_NAME 4
-#define VIRTIO_CONSOLE_PORT_REMOVE 5
-
/*
* This is a global struct for storing common data for all the devices
* this driver handles.
@@ -150,7 +121,7 @@ struct ports_device {
spinlock_t cvq_lock;
/* The current config space is stored here */
- struct virtio_console_multiport_conf config;
+ struct virtio_console_config config;
/* The virtio device we're associated with */
struct virtio_device *vdev;
@@ -1243,7 +1214,7 @@ fail:
*/
static void config_work_handler(struct work_struct *work)
{
- struct virtio_console_multiport_conf virtconconf;
+ struct virtio_console_config virtconconf;
struct ports_device *portdev;
struct virtio_device *vdev;
int err;
@@ -1252,8 +1223,7 @@ static void config_work_handler(struct work_struct *work)
vdev = portdev->vdev;
vdev->config->get(vdev,
- offsetof(struct virtio_console_multiport_conf,
- nr_ports),
+ offsetof(struct virtio_console_config, nr_ports),
&virtconconf.nr_ports,
sizeof(virtconconf.nr_ports));
@@ -1445,19 +1415,16 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
multiport = false;
portdev->config.nr_ports = 1;
portdev->config.max_nr_ports = 1;
-#if 0 /* Multiport is not quite ready yet --RR */
if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
multiport = true;
vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
- vdev->config->get(vdev,
- offsetof(struct virtio_console_multiport_conf,
- nr_ports),
+ vdev->config->get(vdev, offsetof(struct virtio_console_config,
+ nr_ports),
&portdev->config.nr_ports,
sizeof(portdev->config.nr_ports));
- vdev->config->get(vdev,
- offsetof(struct virtio_console_multiport_conf,
- max_nr_ports),
+ vdev->config->get(vdev, offsetof(struct virtio_console_config,
+ max_nr_ports),
&portdev->config.max_nr_ports,
sizeof(portdev->config.max_nr_ports));
if (portdev->config.nr_ports > portdev->config.max_nr_ports) {
@@ -1473,7 +1440,6 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
/* Let the Host know we support multiple ports.*/
vdev->config->finalize_features(vdev);
-#endif
err = init_vqs(portdev);
if (err < 0) {
@@ -1556,6 +1522,7 @@ static struct virtio_device_id id_table[] = {
static unsigned int features[] = {
VIRTIO_CONSOLE_F_SIZE,
+ VIRTIO_CONSOLE_F_MULTIPORT,
};
static struct virtio_driver virtio_console = {
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index 92228a8..ae4f039 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -12,14 +12,37 @@
/* Feature bits */
#define VIRTIO_CONSOLE_F_SIZE 0 /* Does host provide console size? */
+#define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */
struct virtio_console_config {
/* colums of the screens */
__u16 cols;
/* rows of the screens */
__u16 rows;
+ /* max. number of ports this device can hold */
+ __u32 max_nr_ports;
+ /* number of ports added so far */
+ __u32 nr_ports;
} __attribute__((packed));
+/*
+ * A message that's passed between the Host and the Guest for a
+ * particular port.
+ */
+struct virtio_console_control {
+ __u32 id; /* Port number */
+ __u16 event; /* The kind of control event (see below) */
+ __u16 value; /* Extra information for the key */
+};
+
+/* Some events for control messages */
+#define VIRTIO_CONSOLE_PORT_READY 0
+#define VIRTIO_CONSOLE_CONSOLE_PORT 1
+#define VIRTIO_CONSOLE_RESIZE 2
+#define VIRTIO_CONSOLE_PORT_OPEN 3
+#define VIRTIO_CONSOLE_PORT_NAME 4
+#define VIRTIO_CONSOLE_PORT_REMOVE 5
+
#ifdef __KERNEL__
int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
#endif /* __KERNEL__ */
--
1.6.2.5
^ permalink raw reply related
* [PATCH 00/11] (v6) virtio: console: Fixes, new way of discovering ports
From: Amit Shah @ 2010-04-08 14:49 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Juan Quintela, Michael S. Tsirkin
Hello,
This series reworks the ABI to allow port discovery (only) via the
control queue and enable multiport again.
In addition, it adds support for non-blocking write() support, which
means no spinning. This works fine with the recent patches that are on
qemu-devel.
Also included is removal of hvc_remove() as removing one such console
port causes other console ports (registered with hvc) to stall. This
has to be debugged in the hvc_console.c file, I'll do that later, but
we have a nice workaround for this: returning -EPIPE on any hvc
operations will make the hvc console core perform any cleanups for the
removed ports. Looks like we don't lose much by removing hvc_remove().
New in this version:
- Locking for the out_vq. Slightly changed version compared to the
diff I sent earlier in that I don't lock inside
reclaim_used_buffers() now, letting us use spin_lock_irq() instead
of spin_lock_irqsave() from non-irq contexts.
- Remove extra line spotted by Rusty
Rusty, please apply for -next if all is well.
Amit Shah (11):
Revert "virtio: disable multiport console support."
virtio: console: Add a __send_control_msg() that can send messages
without a valid port
virtio: console: Let host know of port or device add failures
virtio: console: Return -EPIPE to hvc_console if we lost the
connection
virtio: console: Don't call hvc_remove() on unplugging console ports
virtio: console: Remove config work handler
virtio: console: Move code around for future patches
virtio: console: Use a control message to add ports
virtio: console: Don't always create a port 0 if using multiport
virtio: console: Rename wait_is_over() to will_read_block()
virtio: console: Add support for nonblocking write()s
drivers/char/virtio_console.c | 605 ++++++++++++++++++++--------------------
include/linux/virtio_console.h | 25 ++
2 files changed, 334 insertions(+), 296 deletions(-)
^ permalink raw reply
* Re: [PATCH v3] Add Mergeable receive buffer support to vhost_net
From: Michael S. Tsirkin @ 2010-04-08 8:35 UTC (permalink / raw)
To: David Stevens; +Cc: kvm, kvm-owner, netdev, rusty, virtualization
In-Reply-To: <OF09358937.789A275B-ON882576FE.0067E654-882576FE.007405B5@us.ibm.com>
On Wed, Apr 07, 2010 at 02:07:18PM -0700, David Stevens wrote:
> kvm-owner@vger.kernel.org wrote on 04/07/2010 11:09:30 AM:
>
> > On Wed, Apr 07, 2010 at 10:37:17AM -0700, David Stevens wrote:
> > > >
> > > > Thanks!
> > > > There's some whitespace damage, are you sending with your new
> > > > sendmail setup? It seems to have worked for qemu patches ...
> > >
> > > Yes, I saw some line wraps in what I received, but I checked
> > > the original draft to be sure and they weren't there. Possibly from
> > > the relay; Sigh.
> > >
> > >
> > > > > @@ -167,8 +166,15 @@ static void handle_tx(struct vhost_net *
> > > > > /* 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);
> > > > > - tx_poll_start(net, sock);
> > > > > + if (err == -EAGAIN) {
> > > > > + vhost_discard_desc(vq, 1);
> > > > > + tx_poll_start(net, sock);
> > > > > + } else {
> > > > > + vq_err(vq, "sendmsg: errno %d\n", -err);
> > > > > + /* drop packet; do not discard/resend */
> > > > > + vhost_add_used_and_signal(&net->dev, vq, head,
> > > > > + 0);
> > > >
> > > > vhost does not currently has a consistent error handling strategy:
> > > > if we drop packets, need to think which other errors should cause
> > > > packet drops. I prefer to just call vq_err for now,
> > > > and have us look at handling segfaults etc in a consistent way
> > > > separately.
> > >
> > > I had to add this to avoid an infinite loop when I wrote a bad
> > > packet on the socket. I agree error handling needs a better look,
> > > but retrying a bad packet continuously while dumping in the log
> > > is what it was doing when I hit an error before this code. Isn't
> > > this better, at least until a second look?
> > >
> >
> > Hmm, what do you mean 'continuously'? Don't we only try again
> > on next kick?
>
> If the packet is corrupt (in my case, a missing vnet header
> during testing), every send will fail and we never make progress.
> I had thousands of error messages in the log (for the same packet)
> before I added this code.
Hmm, we do not want a buggy guest to be able to fill
host logs. This is only if debugging is enabled though, right?
We can also rate-limit the errors.
> If the problem is with the packet,
> retrying the same one as the original code does will never recover.
> This isn't required for mergeable rx buffer support, so I
> can certainly remove it from this patch, but I think the original
> error handling doesn't handle a single corrupted packet very
> gracefully.
>
An approach I considered was to have qemu poll vq_err fd
and stop the device when an error is seen. My concern with
dropping a tx packet is that it would make debugging
very difficult.
> > > > > @@ -222,31 +242,25 @@ static void handle_rx(struct vhost_net *
> > > > > 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(sock->sk))) {
> > > > > + headcount = vhost_get_desc_n(vq, vq->heads, datalen, &in,
> > > > > + vq_log, &log);
> > > >
> > > > This looks like a bug, I think we need to pass
> > > > datalen + header size to vhost_get_desc_n.
> > > > Not sure how we know the header size that backend will use though.
> > > > Maybe just look at our features.
> > >
> > > Yes; we have hdr_size, so I can add it here. It'll be 0 for
> > > the cases where the backend and guest both have vnet header (either
> > > the regular or larger mergeable buffers one), but should be added
> > > in for future raw socket support.
> >
> > So hdr_size is the wrong thing to add then.
> > We need to add non-zero value for tap now.
>
> datalen includes the vnet_hdr in the tap case, so we don't need
> a non-zero hdr_size. The socket data has the entire packet and vnet_hdr
> and that length is what we're getting from vhost_head_len().
I only see vhost_head_len returning skb->len. You are sure skb->len
includes vnet_hdr for tap rx?
> >
> > > >
> > > > > /* OK, now we need to know about added descriptors. */
> > > > > - if (head == vq->num) {
> > > > > - if (unlikely(vhost_enable_notify(vq))) {
> > > > > + if (!headcount) {
> > > > > + if (retries == 0 && unlikely(vhost_enable_notify(vq))) {
> > > > > /* They have slipped one in as we were
> > > > > * doing that: check again. */
> > > > > vhost_disable_notify(vq);
> > > > > + retries++;
> > > > > continue;
> > > > > }
> > > >
> > > > Hmm. The reason we have the code at all, as the comment says, is
> because
> > > > guest could have added more buffers between the time we read last
> index
> > > > and the time we enabled notification. So if we just break like this
> > > > the race still exists. We could remember the
> > > > last head value we observed, and have vhost_enable_notify check
> > > > against this value?
> > >
> > > This is to prevent a spin loop in the case where we have some
> > > buffers available, but not enough for the current packet (ie, this
> > > is the replacement code for the "rxmaxheadcount" business). If they
> > > actually added something new, retrying once should see it, but what
> > > vhost_enable_notify() returns non-zero on is not "new buffers" but
> > > rather "not empty". In the case mentioned, we aren't empty, so
> > > vhost_enable_notify() returns nonzero every time, but the guest hasn't
> > > given us enough buffers to proceed, so we continuously retry; this
> > > code breaks the spin loop until we've really got new buffers from
> > > the guest.
> >
> > What about the race I point out above? It seems to potentially
> > cause a deadlock.
>
> It certainly handles a single race, since the code is identical
> when retries==0. I was assuming that a real update would always get us
> enough buffers, so could not happen twice in a row. The false case of
> having 1 buffer when we need 3, say, would return nonzero every time,
> so this code would detect that and break that loop; never hang if a
> real guest update gives us a full ring.
> If we think we've exhausted the ring and a guest update gives us
> a couple buffers, but not the complete ring (which is what it does in
> practice), then we'd still have a race. In that case, we should be
> comparing avail_idx with itself across multiple calls, rather than
> last_avail_idx (which is only updated when we post a new packet).
> I'll look at this some more. With the guest I have, this code
> will always work, but I don't know that the guest is required to fill
> the ring, which is what this assumes.
I think it is legal for a guest to add buffers one by one.
My suggesting for handling this is to cache last value of available
index we have seen so that vhost_enable_notify returns
true if it changed meanwhile.
Care needs to be taken when index is changed with an ioctl.
> >
> > > >
> > > > Need to think about it.
> > > >
> > > > Another concern here is that on retries vhost_get_desc_n
> > > > is doing extra work, rescanning the same descriptor
> > > > again and again. Not sure how common this is, might be
> > > > worthwhile to add a TODO to consider this at least.
> > >
> > > I had a printk in there to test the code and with the
> > > retries counter, it happens when we fill the ring (once,
> > > because of the retries checks), and then proceeds as
> > > desired when the guest gives us more buffers. Without the
> > > check, it spews until we hear from the guest.
> >
> > I don't understand whether what you write above means 'yes this happens
> > and is worth optimizing for' or 'this happens very rarely
> > and is not worth optimizing for'.
>
> I'm saying "no",
OK then.
> even with high load, we don't hit the
> retries==1 very often with the new code. It only happens when the ring
> is completely full. In that case, with the old code, we would spin until
> we
> hear from the guest (tens of thousands of times); with the new code,
> we hit it once when the ring is full and wait for the guest to give
> us more buffers; then we proceed.
> With the new code in a test run of tens of millions of packets,
> I got maybe 5 or 6 times where we exhausted the ring and waited, but
> with the old code, we did enable/disable tens of thousands of times
> because the ring wasn't completely empty and we were spinning until
> we got new buffers from the guest.
>
I presume under old code you mean some previous version of
merg. buffers patch? I don't see where current upstream would spin
waiting for guest (because a single descriptor is always
enough to fit a packet in), if we have this must fix ASAP.
> > > > > - vhost_discard_vq_desc(vq);
> > > > > + vhost_discard_desc(vq, headcount);
> > > > > break;
> > > > > }
> > > > > /* TODO: Should check and handle checksum. */
> > > > > + if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF)) {
> > > > > + struct virtio_net_hdr_mrg_rxbuf *vhdr =
> > > > > + (struct virtio_net_hdr_mrg_rxbuf *)
> > > > > + vq->iov[0].iov_base;
> > > > > + /* add num_buffers */
> > > > > + if (vhost_has_feature(&net->dev,
> > > > > + VHOST_NET_F_VIRTIO_NET_HDR))
> > > > > + hdr.num_buffers = headcount;
> > > >
> > > > Why is the above necessary?
> > >
> > > This adds mergeable buffer support for the raw case.
> >
> > So my suggestion is to have a copy of the header
> > and then same code will fill in the field for
> > both raw and tap.
>
> The recvmsg() has written the vnethdr into the user
> buffer in the tap case. We don't have an in-kernel copy of it at
> all (hdr_size == 0) in vhost. In the raw case, recvmsg isn't writing it
> to the user buffer and then we do have the local copy in hdr.
> So the code updates num_buffer (only) in user space in
> the tap case, and does the necessary copy of the entire header
> in the raw case. I don't think a double copy of the entire vnet_hdr
> on every packet is a good idea in the tap case,
Right. But what I suggested only copies the num_buffer.
> and the simple
> assignment with the existing copy_to_user() of the header is
> cheaper than double-copying num_buffers in the raw case.
Here I disagree. I think a single write to a buffer that
we know is in cache will be cheaper than a branch.
It's also less codelines :)
> So, I
> do think this code is best. It could use hdr_size instead of
> the (in-line) feature-bit check, but there are no unnecessary
> copies as-is, and I think there would be if we don't use the
> two separate ways of updating num_buffers.
A 2 byte copy is almost free.
> > >
> > > >
> > > > > + vq_err(vq, "tiny buffers < %d unsupported",
> > > > > + vq->iov[0].iov_len);
> > > > > + vhost_discard_desc(vq, headcount);
> > > > > + break;
> > > >
> > > > Problem here is that recvmsg might modify iov.
> > > > This is why I think we need to use vq->hdr here.
> > >
> > > rcvmsg() can never modify the iovec;
> >
> >
> > Look at af_packet code for an example where it does.
> > The pointer is non-const, it's OK to modify.
> > tap used to modify iovec as well, the fact it doesn't
> > now is a side-effect of reusing same code for
> > recvmsg and aio read.
>
> OK, even assuming it can, the check is done after
> the recvmsg -- it can't change between the length check
> and the put_user() of num_buffer, so I'm not sure what
> your concern is here. Are you saying that vq->iov may
> be trashed so that it no longer points to the ring buffer?
recvmsg updates at least the length.
> What I need is to write the num_buffer value at
> offset 10 in the userspace ring buffer after the recvmsg().
> If the iovec has been modified, I'm using the modified.
> If you're saying that it may be garbage, then your suggestion
> is that I save the pointer to offset 10 of the ring buffer
> before the call to recvmsg so I can update it after?
Essentially, yes. Upstream code (move_iovec_hdr) assumes that header
size we might need to save is same as size we hide from backend,
but here it is different. If we generalize move_iovec_hdr to something like
the below (warning: completely untested, likely has integer overflow
or other problems), then I think it will do what we want,
so that memcpy_toiovecend will work:
/* Copy len bytes, and pop the first pop_len bytes from iovec.
* Arguments must satisfy pop_len <= len.
* Return number of segments used. */
static int move_iovec_hdr(struct iovec *from, struct iovec *to,
size_t len, size_t pop_len, int iov_count)
{
int seg = 0;
size_t size;
while (len && seg < iov_count) {
size = min(from->iov_len, len);
to->iov_base = from->iov_base;
to->iov_len = size;
if (pop_len > 0) {
from->iov_len -= size;
from->iov_base += size;
pop_len -= size;
}
len -= size;
++from;
++to;
++seg;
}
return seg;
}
> > > To use vq->hdr, we'd have to copy
> > > it in from user space, modify it, and then copy it back
> > > in to user space, on every packet. In the normal tap case,
> > > hdr_size is 0 and we read it directly from the socket to
> > > user space. It is already correct for mergeable buffers,
> > > except for the num_buffers value added here.
> >
> >
> > I don't understand what you write above, sorry.
> > We have iov, all we need is store the first address+length
> > in the hdr field.
>
> Sorry, in that discussion, I was confusing "hdr" with vq->hdr.
> In the tap case, hdr_size is 0 and we have nothing in vq->hdr.
> As discussed before, if you mean updating a local copy of the
> header, we don't have a local copy of the header -- recvmsg()
> has written it directly to the user buffer. To get a local
> copy, we'd need to either copy_from_user() out of the ring or
> get it from recvmsg() by changing the iovec and then later
> do an extra copy of it to get it in the user buffer where we
> need it.
Yes but we only need to update 2 bytes in userspace memory.
Don't touch anything else.
> >
> > > >
> > > > How about using
> > > > memcpy_toiovecend(vq->hdr, &headcount,
> > > > offsetof(struct virtio_net_hdr_mrg_rxbuf, num_buffers),
> > > > sizeof headcount);
> > > >
> > > > this way we also do not make any assumptions about layout.
> > >
> > > Because this overwrites the valid vnet header we got from
> > > the tap socket with a local copy that isn't correct.
> >
> > It does? I think that this only writes out 2 bytes at an offset.
>
> Oh, sorry, I misread. vq->hdr doesn't have anything in it in
> the tap case, but something like this may eliminate the need to check
> iov_len > sizeof(*vhdr); will investigate.
>
>
> +-DLS
>
^ permalink raw reply
* [PULL] virtio console fixes
From: Rusty Russell @ 2010-04-08 1:07 UTC (permalink / raw)
To: Linus Torvalds
Cc: Amit Shah, François Diakhate, Michael S. Tsirkin,
Anton Blanchard, virtualization
The following changes since commit 48de8cb7847d040c8892701c1ff3c55eff1f46b4:
Linus Torvalds (1):
Merge branch 'perf-fixes-for-linus' of git://git.kernel.org/.../tip/linux-2.6-tip
are available in the git repository at:
ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-for-linus.git master
Amit Shah (1):
MAINTAINERS: Put the virtio-console entry in correct alphabetical order
Anton Blanchard (1):
hvc_console: Fix race between hvc_close and hvc_remove
François Diakhaté (1):
virtio: console: Fix early_put_chars usage
Michael S. Tsirkin (1):
virtio: disable multiport console support.
Rusty Russell (1):
virtio: console makes incorrect assumption about virtio API
MAINTAINERS | 13 ++++----
drivers/char/hvc_console.c | 4 --
drivers/char/virtio_console.c | 65 +++++++++++++++++++++++++++++-----------
include/linux/virtio_console.h | 23 --------------
4 files changed, 54 insertions(+), 51 deletions(-)
commit 9a82446bd269b130a9ac270e720e65c3843d4d0c
Author: Amit Shah <amit.shah@redhat.com>
Date: Tue Mar 23 18:23:09 2010 +0530
MAINTAINERS: Put the virtio-console entry in correct alphabetical order
Move around the entry for virtio-console to keep the file sorted.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
MAINTAINERS | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
commit 162a689a13ed61c0752726edb75427b2cd4186c1
Author: François Diakhaté <fdiakh@gmail.com>
Date: Tue Mar 23 18:23:15 2010 +0530
virtio: console: Fix early_put_chars usage
Currently early_put_chars is not used by virtio_console because it can
only be used once a port has been found, at which point it's too late
because it is no longer needed. This patch should fix it.
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
drivers/char/virtio_console.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
commit 9ff4cfab82d27e9fda72315f911bbaa9516e04bc
Author: Rusty Russell <rusty@rustcorp.com.au>
Date: Thu Apr 8 09:46:16 2010 -0600
virtio: console makes incorrect assumption about virtio API
The get_buf() API sets the second arg to the number of bytes *written*
by the other side; in this case it should be zero as these are output buffers.
lguest gets this right (obviously kvm's console doesn't), resulting in
continual buildup of console writes.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Acked-by: Amit Shah <amit.shah@redhat.com>
drivers/char/virtio_console.c | 10 +++-------
1 files changed, 3 insertions(+), 7 deletions(-)
commit b7a413015d2986edf020fba765c906cc9cbcbfc9
Author: Michael S. Tsirkin <mst@redhat.com>
Date: Wed Mar 31 21:56:42 2010 +0300
virtio: disable multiport console support.
Move MULTIPORT feature and related config changes
out of exported headers, and disable the feature
at runtime.
At this point, it seems less risky to keep code around
until we can enable it than rip it out completely.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
drivers/char/virtio_console.c | 49 +++++++++++++++++++++++++++++++++------
include/linux/virtio_console.h | 23 ------------------
2 files changed, 41 insertions(+), 31 deletions(-)
commit 320718ee074acce5ffced6506cb51af1388942aa
Author: Anton Blanchard <anton@samba.org>
Date: Tue Apr 6 21:42:38 2010 +1000
hvc_console: Fix race between hvc_close and hvc_remove
I don't claim to understand the tty layer, but it seems like hvc_open and
hvc_close should be balanced in their kref reference counting.
Right now we get a kref every call to hvc_open:
if (hp->count++ > 0) {
tty_kref_get(tty); <----- here
spin_unlock_irqrestore(&hp->lock, flags);
hvc_kick();
return 0;
} /* else count == 0 */
tty->driver_data = hp;
hp->tty = tty_kref_get(tty); <------ or here if hp->count was 0
But hvc_close has:
tty_kref_get(tty);
if (--hp->count == 0) {
...
/* Put the ref obtained in hvc_open() */
tty_kref_put(tty);
...
}
tty_kref_put(tty);
Since the outside kref get/put balance we only do a single kref_put when
count reaches 0.
The patch below changes things to call tty_kref_put once for every
hvc_close call, and with that my machine boots fine.
Signed-off-by: Anton Blanchard <anton@samba.org>
Acked-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
drivers/char/hvc_console.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
^ permalink raw reply
* Re: [GIT PULL] vhost-net fix for 2.6.34-rc3
From: David Miller @ 2010-04-07 23:52 UTC (permalink / raw)
To: mst; +Cc: kvm, virtualization, netdev, linux-kernel, jdike
In-Reply-To: <20100407173502.GA26061@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Wed, 7 Apr 2010 20:35:02 +0300
> David,
> The following tree includes a patch fixing an issue with vhost-net in
> 2.6.34-rc3. Please pull for 2.6.34.
Pulled, thanks Michael.
^ permalink raw reply
* Re: [PATCH v3] Add Mergeable receive buffer support to vhost_net
From: David Stevens @ 2010-04-07 21:07 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, kvm-owner, netdev, rusty, virtualization
In-Reply-To: <20100407180929.GB26186@redhat.com>
kvm-owner@vger.kernel.org wrote on 04/07/2010 11:09:30 AM:
> On Wed, Apr 07, 2010 at 10:37:17AM -0700, David Stevens wrote:
> > >
> > > Thanks!
> > > There's some whitespace damage, are you sending with your new
> > > sendmail setup? It seems to have worked for qemu patches ...
> >
> > Yes, I saw some line wraps in what I received, but I checked
> > the original draft to be sure and they weren't there. Possibly from
> > the relay; Sigh.
> >
> >
> > > > @@ -167,8 +166,15 @@ static void handle_tx(struct vhost_net *
> > > > /* 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);
> > > > - tx_poll_start(net, sock);
> > > > + if (err == -EAGAIN) {
> > > > + vhost_discard_desc(vq, 1);
> > > > + tx_poll_start(net, sock);
> > > > + } else {
> > > > + vq_err(vq, "sendmsg: errno %d\n", -err);
> > > > + /* drop packet; do not discard/resend */
> > > > + vhost_add_used_and_signal(&net->dev, vq, head,
> > > > + 0);
> > >
> > > vhost does not currently has a consistent error handling strategy:
> > > if we drop packets, need to think which other errors should cause
> > > packet drops. I prefer to just call vq_err for now,
> > > and have us look at handling segfaults etc in a consistent way
> > > separately.
> >
> > I had to add this to avoid an infinite loop when I wrote a bad
> > packet on the socket. I agree error handling needs a better look,
> > but retrying a bad packet continuously while dumping in the log
> > is what it was doing when I hit an error before this code. Isn't
> > this better, at least until a second look?
> >
>
> Hmm, what do you mean 'continuously'? Don't we only try again
> on next kick?
If the packet is corrupt (in my case, a missing vnet header
during testing), every send will fail and we never make progress.
I had thousands of error messages in the log (for the same packet)
before I added this code. If the problem is with the packet,
retrying the same one as the original code does will never recover.
This isn't required for mergeable rx buffer support, so I
can certainly remove it from this patch, but I think the original
error handling doesn't handle a single corrupted packet very
gracefully.
> > > > @@ -222,31 +242,25 @@ static void handle_rx(struct vhost_net *
> > > > 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(sock->sk))) {
> > > > + headcount = vhost_get_desc_n(vq, vq->heads, datalen, &in,
> > > > + vq_log, &log);
> > >
> > > This looks like a bug, I think we need to pass
> > > datalen + header size to vhost_get_desc_n.
> > > Not sure how we know the header size that backend will use though.
> > > Maybe just look at our features.
> >
> > Yes; we have hdr_size, so I can add it here. It'll be 0 for
> > the cases where the backend and guest both have vnet header (either
> > the regular or larger mergeable buffers one), but should be added
> > in for future raw socket support.
>
> So hdr_size is the wrong thing to add then.
> We need to add non-zero value for tap now.
datalen includes the vnet_hdr in the tap case, so we don't need
a non-zero hdr_size. The socket data has the entire packet and vnet_hdr
and that length is what we're getting from vhost_head_len().
>
> > >
> > > > /* OK, now we need to know about added descriptors. */
> > > > - if (head == vq->num) {
> > > > - if (unlikely(vhost_enable_notify(vq))) {
> > > > + if (!headcount) {
> > > > + if (retries == 0 && unlikely(vhost_enable_notify(vq))) {
> > > > /* They have slipped one in as we were
> > > > * doing that: check again. */
> > > > vhost_disable_notify(vq);
> > > > + retries++;
> > > > continue;
> > > > }
> > >
> > > Hmm. The reason we have the code at all, as the comment says, is
because
> > > guest could have added more buffers between the time we read last
index
> > > and the time we enabled notification. So if we just break like this
> > > the race still exists. We could remember the
> > > last head value we observed, and have vhost_enable_notify check
> > > against this value?
> >
> > This is to prevent a spin loop in the case where we have some
> > buffers available, but not enough for the current packet (ie, this
> > is the replacement code for the "rxmaxheadcount" business). If they
> > actually added something new, retrying once should see it, but what
> > vhost_enable_notify() returns non-zero on is not "new buffers" but
> > rather "not empty". In the case mentioned, we aren't empty, so
> > vhost_enable_notify() returns nonzero every time, but the guest hasn't
> > given us enough buffers to proceed, so we continuously retry; this
> > code breaks the spin loop until we've really got new buffers from
> > the guest.
>
> What about the race I point out above? It seems to potentially
> cause a deadlock.
It certainly handles a single race, since the code is identical
when retries==0. I was assuming that a real update would always get us
enough buffers, so could not happen twice in a row. The false case of
having 1 buffer when we need 3, say, would return nonzero every time,
so this code would detect that and break that loop; never hang if a
real guest update gives us a full ring.
If we think we've exhausted the ring and a guest update gives us
a couple buffers, but not the complete ring (which is what it does in
practice), then we'd still have a race. In that case, we should be
comparing avail_idx with itself across multiple calls, rather than
last_avail_idx (which is only updated when we post a new packet).
I'll look at this some more. With the guest I have, this code
will always work, but I don't know that the guest is required to fill
the ring, which is what this assumes.
>
> > >
> > > Need to think about it.
> > >
> > > Another concern here is that on retries vhost_get_desc_n
> > > is doing extra work, rescanning the same descriptor
> > > again and again. Not sure how common this is, might be
> > > worthwhile to add a TODO to consider this at least.
> >
> > I had a printk in there to test the code and with the
> > retries counter, it happens when we fill the ring (once,
> > because of the retries checks), and then proceeds as
> > desired when the guest gives us more buffers. Without the
> > check, it spews until we hear from the guest.
>
> I don't understand whether what you write above means 'yes this happens
> and is worth optimizing for' or 'this happens very rarely
> and is not worth optimizing for'.
I'm saying "no", even with high load, we don't hit the
retries==1 very often with the new code. It only happens when the ring
is completely full. In that case, with the old code, we would spin until
we
hear from the guest (tens of thousands of times); with the new code,
we hit it once when the ring is full and wait for the guest to give
us more buffers; then we proceed.
With the new code in a test run of tens of millions of packets,
I got maybe 5 or 6 times where we exhausted the ring and waited, but
with the old code, we did enable/disable tens of thousands of times
because the ring wasn't completely empty and we were spinning until
we got new buffers from the guest.
> > > > - vhost_discard_vq_desc(vq);
> > > > + vhost_discard_desc(vq, headcount);
> > > > break;
> > > > }
> > > > /* TODO: Should check and handle checksum. */
> > > > + if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF)) {
> > > > + struct virtio_net_hdr_mrg_rxbuf *vhdr =
> > > > + (struct virtio_net_hdr_mrg_rxbuf *)
> > > > + vq->iov[0].iov_base;
> > > > + /* add num_buffers */
> > > > + if (vhost_has_feature(&net->dev,
> > > > + VHOST_NET_F_VIRTIO_NET_HDR))
> > > > + hdr.num_buffers = headcount;
> > >
> > > Why is the above necessary?
> >
> > This adds mergeable buffer support for the raw case.
>
> So my suggestion is to have a copy of the header
> and then same code will fill in the field for
> both raw and tap.
The recvmsg() has written the vnethdr into the user
buffer in the tap case. We don't have an in-kernel copy of it at
all (hdr_size == 0) in vhost. In the raw case, recvmsg isn't writing it
to the user buffer and then we do have the local copy in hdr.
So the code updates num_buffer (only) in user space in
the tap case, and does the necessary copy of the entire header
in the raw case. I don't think a double copy of the entire vnet_hdr
on every packet is a good idea in the tap case, and the simple
assignment with the existing copy_to_user() of the header is
cheaper than double-copying num_buffers in the raw case. So, I
do think this code is best. It could use hdr_size instead of
the (in-line) feature-bit check, but there are no unnecessary
copies as-is, and I think there would be if we don't use the
two separate ways of updating num_buffers.
> >
> > >
> > > > + vq_err(vq, "tiny buffers < %d unsupported",
> > > > + vq->iov[0].iov_len);
> > > > + vhost_discard_desc(vq, headcount);
> > > > + break;
> > >
> > > Problem here is that recvmsg might modify iov.
> > > This is why I think we need to use vq->hdr here.
> >
> > rcvmsg() can never modify the iovec;
>
>
> Look at af_packet code for an example where it does.
> The pointer is non-const, it's OK to modify.
> tap used to modify iovec as well, the fact it doesn't
> now is a side-effect of reusing same code for
> recvmsg and aio read.
OK, even assuming it can, the check is done after
the recvmsg -- it can't change between the length check
and the put_user() of num_buffer, so I'm not sure what
your concern is here. Are you saying that vq->iov may
be trashed so that it no longer points to the ring buffer?
What I need is to write the num_buffer value at
offset 10 in the userspace ring buffer after the recvmsg().
If the iovec has been modified, I'm using the modified.
If you're saying that it may be garbage, then your suggestion
is that I save the pointer to offset 10 of the ring buffer
before the call to recvmsg so I can update it after?
> > To use vq->hdr, we'd have to copy
> > it in from user space, modify it, and then copy it back
> > in to user space, on every packet. In the normal tap case,
> > hdr_size is 0 and we read it directly from the socket to
> > user space. It is already correct for mergeable buffers,
> > except for the num_buffers value added here.
>
>
> I don't understand what you write above, sorry.
> We have iov, all we need is store the first address+length
> in the hdr field.
Sorry, in that discussion, I was confusing "hdr" with vq->hdr.
In the tap case, hdr_size is 0 and we have nothing in vq->hdr.
As discussed before, if you mean updating a local copy of the
header, we don't have a local copy of the header -- recvmsg()
has written it directly to the user buffer. To get a local
copy, we'd need to either copy_from_user() out of the ring or
get it from recvmsg() by changing the iovec and then later
do an extra copy of it to get it in the user buffer where we
need it.
>
> > >
> > > How about using
> > > memcpy_toiovecend(vq->hdr, &headcount,
> > > offsetof(struct virtio_net_hdr_mrg_rxbuf, num_buffers),
> > > sizeof headcount);
> > >
> > > this way we also do not make any assumptions about layout.
> >
> > Because this overwrites the valid vnet header we got from
> > the tap socket with a local copy that isn't correct.
>
> It does? I think that this only writes out 2 bytes at an offset.
Oh, sorry, I misread. vq->hdr doesn't have anything in it in
the tap case, but something like this may eliminate the need to check
iov_len > sizeof(*vhdr); will investigate.
+-DLS
^ permalink raw reply
* Re: A clocksource driver for HyperV
From: Jeremy Fitzhardinge @ 2010-04-07 19:40 UTC (permalink / raw)
To: Ky Srinivasan; +Cc: devel, Virtualization, haiyangz, gregkh
In-Reply-To: <4BBC789A0200003000082A41@sinclair.provo.novell.com>
On 04/07/2010 11:20 AM, Ky Srinivasan wrote:
> Jeremy, thank you for your comments. I am attaching the next version of this patch that addresses the comments I have gotten thus far.
>
It looks like you overlooked a couple of comments.
> +}
> +
> +static struct clocksource hyperv_cs = {
> + .name = "hyperv_clocksource",
> + .rating = 400, /* use this when running on Hyperv*/
> + .read = read_hv_clock,
>
Did you not want to call it "hyperv" for some reason?
> +static int __init hv_detect_hyperv(void)
> +{
> + u32 eax, ebx, ecx, edx;
> + char hyp_signature[20];
>
Why 20? You're only putting 12 bytes into it.
> +
> + cpuid(1,&eax,&ebx,&ecx,&edx);
> +
> + if (!(ecx& HV_HYPERVISOR_PRESENT_BIT))
> + return 1;
> +
> + cpuid(HV_CPUID_SIGNATURE,&eax,&ebx,&ecx,&edx);
> + *(u32 *)(hyp_signature + 0) = ebx;
> + *(u32 *)(hyp_signature + 4) = ecx;
> + *(u32 *)(hyp_signature + 8) = edx;
> +
> + if ((eax< HV_CPUID_MIN) ||
> + (memcmp("Microsoft Hv", hyp_signature, 12))) {
> + printk(KERN_WARNING
> + "Not on HyperV; signature %s, eax %x\n",
> + hyp_signature, eax);
>
The message is pointless. There's nothing to error or warn about
booting under some other
hypervisor.
Thanks,
J
^ permalink raw reply
* Re: A clocksource driver for HyperV
From: Ky Srinivasan @ 2010-04-07 18:20 UTC (permalink / raw)
To: jeremy; +Cc: devel, Virtualization, haiyangz, gregkh
In-Reply-To: <4BBC789A0200003000082A41@sinclair.provo.novell.com>
[-- Attachment #1: Type: text/plain, Size: 5489 bytes --]
>>> On 4/5/2010 at 5:36 PM, in message < 4BBA57FB.2000406@goop.org >, Jeremy
Fitzhardinge < jeremy@goop.org > wrote:
> On 04/05/2010 01:30 PM, Ky Srinivasan wrote:
>> +static cycle_t read_hv_clock(struct clocksource *arg)
>> +{
>> + cycle_t current_tick;
>> + /*
>> + * Read the partition counter to get the current tick count. This count
>> + * is set to 0 when the partition is created and is incremented in
>> + * 100 nanosecond units.
>> + */
>> + rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
>> + return current_tick;
>> +}
>> +
>> +static struct clocksource clocksource_hyperv = {
>> + .name = "hyperv_clocksource",
>>
>
> Seems like a redundantly long name; any use of this string is going to
> be in a context where it is obviously a clocksource. How about just
> "hyperv"
>
>> + .rating = 400, /* use this when running on Hyperv*/
>> + .read = read_hv_clock,
>> + .mask = CLOCKSOURCE_MASK(64),
>> + .shift = HV_CLOCK_SHIFT,
>> +};
>> +
>> +static struct dmi_system_id __initconst
>> +hv_timesource_dmi_table[] __maybe_unused = {
>> + {
>> + .ident = "Hyper-V",
>> + .matches = {
>> + DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> + DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
>> + DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
>> + },
>> + },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
>>
>
> So you use the DMI signatures to determine whether the module is needed,
> but cpuid to work out if the feature is present?
>
>> +
>> +static struct pci_device_id __initconst
>> +hv_timesource_pci_table[] __maybe_unused = {
>> + { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
>> + { 0 }
>> +};
>> +MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
>>
>
> And/or PCI?
>
> Seems a bit... ad-hoc? Is this the official way to determine the
> presence of Hyper-V?
The presence of HyperV and our ability to use the partition-wide counter obviously is checked via probing the cpuid leaves. The DMI/PCI signatures are used in auto-loading these modules.
>
>> +
>> +
>> +static int __init hv_detect_hyperv(void)
>>
>
> This looks generally useful. Should it be hidden away in the
> clocksource driver, or in some common hyper-v code? Do other hyper-v
> drivers have versions of this?
Good point. Right now, I can think of multiple drivers replicating this code. We could include hyperV detection code in cpu/hypervisor.c . I will spin up a patch for doing that shortly.
>
>> +{
>> + u32 eax, ebx, ecx, edx;
>> + static char hyp_signature[20];
>>
> 20? static?
>
>> +
>> + cpuid(1,&eax,&ebx,&ecx,&edx);
>> + if (!(ecx& HV_HYPERVISOR_PRESENT_BIT)) {
>> + printk(KERN_WARNING
>> + "Not on a Hypervisor\n");
>>
> This just looks like noise, especially since it doesn't identify what is
> generating the message. And if you compile this code in as =y
> (non-modular) then it will complain every boot.
>
>> + return 1;
>> + }
>> + cpuid(HV_CPUID_SIGNATURE,&eax,&ebx,&ecx,&edx);
>> + *(u32 *)(hyp_signature + 0) = ebx;
>> + *(u32 *)(hyp_signature + 4) = ecx;
>> + *(u32 *)(hyp_signature + 8) = edx;
>> + hyp_signature[12] = 0;
>> +
>> + if ((eax< HV_CPUID_MIN) || (strcmp("Microsoft Hv", hyp_signature))) {
>>
>
> memcmp, surely?
>
>> + printk(KERN_WARNING
>> + "Not on HyperV; signature %s, eax %x\n",
>> + hyp_signature, eax);
>> + return 1;
>> + }
>> + /*
>> + * Extract the features, recommendations etc.
>> + */
>> + cpuid(HV_CPUID_FEATURES,&eax,&ebx,&ecx,&edx);
>> + if (!(eax& 0x10)) {
>> + printk(KERN_WARNING "HyperV Time Ref Counter not available!\n");
>> + return 1;
>> + }
>> +
>> + cpuid(HV_CPUID_RECOMMENDATIONS,&eax,&ebx,&ecx,&edx);
>> + printk(KERN_INFO "HyperV recommendations: %x\n", eax);
>> + printk(KERN_INFO "HyperV spin count: %x\n", ebx);
>> + return 0;
>> +}
>> +
>> +
>> +static int __init init_hv_clocksource(void)
>> +{
>> + if (hv_detect_hyperv())
>> + return -ENODEV;
>> + /*
>> + * The time ref counter in HyperV is in 100ns units.
>> + * The definition of mult is:
>> + * mult/2^shift = ns/cyc = 100
>> + * mult = (100<< shift)
>> + */
>> + clocksource_hyperv.mult = (100<< HV_CLOCK_SHIFT);
>>
>
> Why not initialize this in the structure? It's just 100<<22 isn't it?
>
>> + printk(KERN_INFO "Registering HyperV clock source\n");
>> + return clocksource_register(&clocksource_hyperv);
>> +}
>> +
>> +module_init(init_hv_clocksource);
>> +MODULE_DESCRIPTION("HyperV based clocksource");
>> +MODULE_AUTHOR("K. Y. Srinivasan< ksrinivasan@novell.com >");
>> +MODULE_LICENSE("GPL");
>> Index: linux/drivers/staging/hv/Makefile
>> ===================================================================
>> --- linux.orig/drivers/staging/hv/Makefile 2010-04-05 13:02:06.000000000 -0600
>> +++ linux/drivers/staging/hv/Makefile 2010-04-05 13:02:13.000000000 -0600
>> @@ -1,4 +1,4 @@
>> -obj-$(CONFIG_HYPERV) += hv_vmbus.o
>> +obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_timesource.o
>> obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o
>> obj-$(CONFIG_HYPERV_BLOCK) += hv_blkvsc.o
>> obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
>>
Jeremy, thank you for your comments. I am attaching the next version of this patch that addresses the comments I have gotten thus far.
Regards,
K. Y
[-- Attachment #2: hyperv_clocksource.patch --]
[-- Type: text/plain, Size: 5525 bytes --]
From: K. Y. Srinivasan <ksrinivasan@novell.com>
Subject: A clocksource for Linux guests hosted on HyperV.
References: None
Patch-mainline:
This patch is a clocksource implementation suitable for guests hosted on HyperV.
Time keeping in Linux guests hosted on HyperV is unstable. This clocksource
driver fixes the problem.
Signed-off-by: K. Y. Srinivasan <ksrinivasan@novell.com>
Index: linux/drivers/staging/hv/hv_timesource.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux/drivers/staging/hv/hv_timesource.c 2010-04-07 12:17:29.000000000 -0600
@@ -0,0 +1,147 @@
+/*
+ * A clocksource for Linux running on HyperV.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan <ksrinivasan@novell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include <linux/version.h>
+#include <linux/clocksource.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/dmi.h>
+
+#define HV_CLOCK_SHIFT 22
+/*
+ * HyperV defined synthetic CPUID leaves:
+ */
+#define HV_CPUID_SIGNATURE 0x40000000
+#define HV_CPUID_MIN 0x40000005
+#define HV_HYPERVISOR_PRESENT_BIT 0x80000000
+#define HV_CPUID_FEATURES 0x40000003
+#define HV_CPUID_RECOMMENDATIONS 0x40000004
+
+/*
+ * HyperV defined synthetic MSRs
+ */
+
+#define HV_X64_MSR_TIME_REF_COUNT 0x40000020
+
+
+static cycle_t read_hv_clock(struct clocksource *arg)
+{
+ cycle_t current_tick;
+ /*
+ * Read the partition counter to get the current tick count. This count
+ * is set to 0 when the partition is created and is incremented in
+ * 100 nanosecond units.
+ */
+ rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
+ return current_tick;
+}
+
+static struct clocksource hyperv_cs = {
+ .name = "hyperv_clocksource",
+ .rating = 400, /* use this when running on Hyperv*/
+ .read = read_hv_clock,
+ .mask = CLOCKSOURCE_MASK(64),
+ /*
+ * The time ref counter in HyperV is in 100ns units.
+ * The definition of mult is:
+ * mult/2^shift = ns/cyc = 100
+ * mult = (100 << shift)
+ */
+ .mult = (100 << HV_CLOCK_SHIFT),
+ .shift = HV_CLOCK_SHIFT,
+};
+
+static const struct dmi_system_id __initconst
+hv_timesource_dmi_table[] __maybe_unused = {
+ {
+ .ident = "Hyper-V",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
+ DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
+ },
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
+
+static const struct pci_device_id __initconst
+hv_timesource_pci_table[] __maybe_unused = {
+ { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
+ { 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
+
+
+static int __init hv_detect_hyperv(void)
+{
+ u32 eax, ebx, ecx, edx;
+ char hyp_signature[20];
+
+ cpuid(1, &eax, &ebx, &ecx, &edx);
+
+ if (!(ecx & HV_HYPERVISOR_PRESENT_BIT))
+ return 1;
+
+ cpuid(HV_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
+ *(u32 *)(hyp_signature + 0) = ebx;
+ *(u32 *)(hyp_signature + 4) = ecx;
+ *(u32 *)(hyp_signature + 8) = edx;
+
+ if ((eax < HV_CPUID_MIN) ||
+ (memcmp("Microsoft Hv", hyp_signature, 12))) {
+ printk(KERN_WARNING
+ "Not on HyperV; signature %s, eax %x\n",
+ hyp_signature, eax);
+ return 1;
+ }
+ /*
+ * Extract the features, recommendations etc.
+ */
+ cpuid(HV_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
+ if (!(eax & 0x10)) {
+ printk(KERN_WARNING "HyperV Time Ref Counter not available!\n");
+ return 1;
+ }
+
+ cpuid(HV_CPUID_RECOMMENDATIONS, &eax, &ebx, &ecx, &edx);
+ printk(KERN_INFO "HyperV recommendations: %x\n", eax);
+ printk(KERN_INFO "HyperV spin count: %x\n", ebx);
+ return 0;
+}
+
+
+static int __init init_hv_clocksource(void)
+{
+ if (hv_detect_hyperv())
+ return -ENODEV;
+ printk(KERN_INFO "Registering HyperV clock source\n");
+ return clocksource_register(&hyperv_cs);
+}
+
+module_init(init_hv_clocksource);
+MODULE_DESCRIPTION("HyperV based clocksource");
+MODULE_AUTHOR("K. Y. Srinivasan <ksrinivasan@novell.com>");
+MODULE_LICENSE("GPL");
Index: linux/drivers/staging/hv/Makefile
===================================================================
--- linux.orig/drivers/staging/hv/Makefile 2010-04-07 12:17:25.000000000 -0600
+++ linux/drivers/staging/hv/Makefile 2010-04-07 12:17:29.000000000 -0600
@@ -1,4 +1,4 @@
-obj-$(CONFIG_HYPERV) += hv_vmbus.o
+obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_timesource.o
obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o
obj-$(CONFIG_HYPERV_BLOCK) += hv_blkvsc.o
obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
[-- Attachment #3: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: A clocksource driver for HyperV
From: Ky Srinivasan @ 2010-04-07 18:20 UTC (permalink / raw)
To: jeremy; +Cc: devel, Virtualization, haiyangz, gregkh
In-Reply-To: <4BBC788D0200003000082A39@sinclair.provo.novell.com>
[-- Attachment #1: Type: text/plain, Size: 5489 bytes --]
>>> On 4/5/2010 at 5:36 PM, in message < 4BBA57FB.2000406@goop.org >, Jeremy
Fitzhardinge < jeremy@goop.org > wrote:
> On 04/05/2010 01:30 PM, Ky Srinivasan wrote:
>> +static cycle_t read_hv_clock(struct clocksource *arg)
>> +{
>> + cycle_t current_tick;
>> + /*
>> + * Read the partition counter to get the current tick count. This count
>> + * is set to 0 when the partition is created and is incremented in
>> + * 100 nanosecond units.
>> + */
>> + rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
>> + return current_tick;
>> +}
>> +
>> +static struct clocksource clocksource_hyperv = {
>> + .name = "hyperv_clocksource",
>>
>
> Seems like a redundantly long name; any use of this string is going to
> be in a context where it is obviously a clocksource. How about just
> "hyperv"
>
>> + .rating = 400, /* use this when running on Hyperv*/
>> + .read = read_hv_clock,
>> + .mask = CLOCKSOURCE_MASK(64),
>> + .shift = HV_CLOCK_SHIFT,
>> +};
>> +
>> +static struct dmi_system_id __initconst
>> +hv_timesource_dmi_table[] __maybe_unused = {
>> + {
>> + .ident = "Hyper-V",
>> + .matches = {
>> + DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> + DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
>> + DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
>> + },
>> + },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
>>
>
> So you use the DMI signatures to determine whether the module is needed,
> but cpuid to work out if the feature is present?
>
>> +
>> +static struct pci_device_id __initconst
>> +hv_timesource_pci_table[] __maybe_unused = {
>> + { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
>> + { 0 }
>> +};
>> +MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
>>
>
> And/or PCI?
>
> Seems a bit... ad-hoc? Is this the official way to determine the
> presence of Hyper-V?
The presence of HyperV and our ability to use the partition-wide counter obviously is checked via probing the cpuid leaves. The DMI/PCI signatures are used in auto-loading these modules.
>
>> +
>> +
>> +static int __init hv_detect_hyperv(void)
>>
>
> This looks generally useful. Should it be hidden away in the
> clocksource driver, or in some common hyper-v code? Do other hyper-v
> drivers have versions of this?
Good point. Right now, I can think of multiple drivers replicating this code. We could include hyperV detection code in cpu/hypervisor.c . I will spin up a patch for doing that shortly.
>
>> +{
>> + u32 eax, ebx, ecx, edx;
>> + static char hyp_signature[20];
>>
> 20? static?
>
>> +
>> + cpuid(1,&eax,&ebx,&ecx,&edx);
>> + if (!(ecx& HV_HYPERVISOR_PRESENT_BIT)) {
>> + printk(KERN_WARNING
>> + "Not on a Hypervisor\n");
>>
> This just looks like noise, especially since it doesn't identify what is
> generating the message. And if you compile this code in as =y
> (non-modular) then it will complain every boot.
>
>> + return 1;
>> + }
>> + cpuid(HV_CPUID_SIGNATURE,&eax,&ebx,&ecx,&edx);
>> + *(u32 *)(hyp_signature + 0) = ebx;
>> + *(u32 *)(hyp_signature + 4) = ecx;
>> + *(u32 *)(hyp_signature + 8) = edx;
>> + hyp_signature[12] = 0;
>> +
>> + if ((eax< HV_CPUID_MIN) || (strcmp("Microsoft Hv", hyp_signature))) {
>>
>
> memcmp, surely?
>
>> + printk(KERN_WARNING
>> + "Not on HyperV; signature %s, eax %x\n",
>> + hyp_signature, eax);
>> + return 1;
>> + }
>> + /*
>> + * Extract the features, recommendations etc.
>> + */
>> + cpuid(HV_CPUID_FEATURES,&eax,&ebx,&ecx,&edx);
>> + if (!(eax& 0x10)) {
>> + printk(KERN_WARNING "HyperV Time Ref Counter not available!\n");
>> + return 1;
>> + }
>> +
>> + cpuid(HV_CPUID_RECOMMENDATIONS,&eax,&ebx,&ecx,&edx);
>> + printk(KERN_INFO "HyperV recommendations: %x\n", eax);
>> + printk(KERN_INFO "HyperV spin count: %x\n", ebx);
>> + return 0;
>> +}
>> +
>> +
>> +static int __init init_hv_clocksource(void)
>> +{
>> + if (hv_detect_hyperv())
>> + return -ENODEV;
>> + /*
>> + * The time ref counter in HyperV is in 100ns units.
>> + * The definition of mult is:
>> + * mult/2^shift = ns/cyc = 100
>> + * mult = (100<< shift)
>> + */
>> + clocksource_hyperv.mult = (100<< HV_CLOCK_SHIFT);
>>
>
> Why not initialize this in the structure? It's just 100<<22 isn't it?
>
>> + printk(KERN_INFO "Registering HyperV clock source\n");
>> + return clocksource_register(&clocksource_hyperv);
>> +}
>> +
>> +module_init(init_hv_clocksource);
>> +MODULE_DESCRIPTION("HyperV based clocksource");
>> +MODULE_AUTHOR("K. Y. Srinivasan< ksrinivasan@novell.com >");
>> +MODULE_LICENSE("GPL");
>> Index: linux/drivers/staging/hv/Makefile
>> ===================================================================
>> --- linux.orig/drivers/staging/hv/Makefile 2010-04-05 13:02:06.000000000 -0600
>> +++ linux/drivers/staging/hv/Makefile 2010-04-05 13:02:13.000000000 -0600
>> @@ -1,4 +1,4 @@
>> -obj-$(CONFIG_HYPERV) += hv_vmbus.o
>> +obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_timesource.o
>> obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o
>> obj-$(CONFIG_HYPERV_BLOCK) += hv_blkvsc.o
>> obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
>>
Jeremy, thank you for your comments. I am attaching the next version of this patch that addresses the comments I have gotten thus far.
Regards,
K. Y
[-- Attachment #2: hyperv_clocksource.patch --]
[-- Type: text/plain, Size: 5525 bytes --]
From: K. Y. Srinivasan <ksrinivasan@novell.com>
Subject: A clocksource for Linux guests hosted on HyperV.
References: None
Patch-mainline:
This patch is a clocksource implementation suitable for guests hosted on HyperV.
Time keeping in Linux guests hosted on HyperV is unstable. This clocksource
driver fixes the problem.
Signed-off-by: K. Y. Srinivasan <ksrinivasan@novell.com>
Index: linux/drivers/staging/hv/hv_timesource.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux/drivers/staging/hv/hv_timesource.c 2010-04-07 12:17:29.000000000 -0600
@@ -0,0 +1,147 @@
+/*
+ * A clocksource for Linux running on HyperV.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan <ksrinivasan@novell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include <linux/version.h>
+#include <linux/clocksource.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/dmi.h>
+
+#define HV_CLOCK_SHIFT 22
+/*
+ * HyperV defined synthetic CPUID leaves:
+ */
+#define HV_CPUID_SIGNATURE 0x40000000
+#define HV_CPUID_MIN 0x40000005
+#define HV_HYPERVISOR_PRESENT_BIT 0x80000000
+#define HV_CPUID_FEATURES 0x40000003
+#define HV_CPUID_RECOMMENDATIONS 0x40000004
+
+/*
+ * HyperV defined synthetic MSRs
+ */
+
+#define HV_X64_MSR_TIME_REF_COUNT 0x40000020
+
+
+static cycle_t read_hv_clock(struct clocksource *arg)
+{
+ cycle_t current_tick;
+ /*
+ * Read the partition counter to get the current tick count. This count
+ * is set to 0 when the partition is created and is incremented in
+ * 100 nanosecond units.
+ */
+ rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
+ return current_tick;
+}
+
+static struct clocksource hyperv_cs = {
+ .name = "hyperv_clocksource",
+ .rating = 400, /* use this when running on Hyperv*/
+ .read = read_hv_clock,
+ .mask = CLOCKSOURCE_MASK(64),
+ /*
+ * The time ref counter in HyperV is in 100ns units.
+ * The definition of mult is:
+ * mult/2^shift = ns/cyc = 100
+ * mult = (100 << shift)
+ */
+ .mult = (100 << HV_CLOCK_SHIFT),
+ .shift = HV_CLOCK_SHIFT,
+};
+
+static const struct dmi_system_id __initconst
+hv_timesource_dmi_table[] __maybe_unused = {
+ {
+ .ident = "Hyper-V",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
+ DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
+ },
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
+
+static const struct pci_device_id __initconst
+hv_timesource_pci_table[] __maybe_unused = {
+ { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
+ { 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
+
+
+static int __init hv_detect_hyperv(void)
+{
+ u32 eax, ebx, ecx, edx;
+ char hyp_signature[20];
+
+ cpuid(1, &eax, &ebx, &ecx, &edx);
+
+ if (!(ecx & HV_HYPERVISOR_PRESENT_BIT))
+ return 1;
+
+ cpuid(HV_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
+ *(u32 *)(hyp_signature + 0) = ebx;
+ *(u32 *)(hyp_signature + 4) = ecx;
+ *(u32 *)(hyp_signature + 8) = edx;
+
+ if ((eax < HV_CPUID_MIN) ||
+ (memcmp("Microsoft Hv", hyp_signature, 12))) {
+ printk(KERN_WARNING
+ "Not on HyperV; signature %s, eax %x\n",
+ hyp_signature, eax);
+ return 1;
+ }
+ /*
+ * Extract the features, recommendations etc.
+ */
+ cpuid(HV_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
+ if (!(eax & 0x10)) {
+ printk(KERN_WARNING "HyperV Time Ref Counter not available!\n");
+ return 1;
+ }
+
+ cpuid(HV_CPUID_RECOMMENDATIONS, &eax, &ebx, &ecx, &edx);
+ printk(KERN_INFO "HyperV recommendations: %x\n", eax);
+ printk(KERN_INFO "HyperV spin count: %x\n", ebx);
+ return 0;
+}
+
+
+static int __init init_hv_clocksource(void)
+{
+ if (hv_detect_hyperv())
+ return -ENODEV;
+ printk(KERN_INFO "Registering HyperV clock source\n");
+ return clocksource_register(&hyperv_cs);
+}
+
+module_init(init_hv_clocksource);
+MODULE_DESCRIPTION("HyperV based clocksource");
+MODULE_AUTHOR("K. Y. Srinivasan <ksrinivasan@novell.com>");
+MODULE_LICENSE("GPL");
Index: linux/drivers/staging/hv/Makefile
===================================================================
--- linux.orig/drivers/staging/hv/Makefile 2010-04-07 12:17:25.000000000 -0600
+++ linux/drivers/staging/hv/Makefile 2010-04-07 12:17:29.000000000 -0600
@@ -1,4 +1,4 @@
-obj-$(CONFIG_HYPERV) += hv_vmbus.o
+obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_timesource.o
obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o
obj-$(CONFIG_HYPERV_BLOCK) += hv_blkvsc.o
obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
[-- Attachment #3: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3] Add Mergeable receive buffer support to vhost_net
From: Michael S. Tsirkin @ 2010-04-07 18:09 UTC (permalink / raw)
To: David Stevens; +Cc: kvm, kvm-owner, netdev, rusty, virtualization
In-Reply-To: <OFC2D0B87C.35124B1B-ON882576FE.005ADDB2-882576FE.0060CB7B@us.ibm.com>
On Wed, Apr 07, 2010 at 10:37:17AM -0700, David Stevens wrote:
> >
> > Thanks!
> > There's some whitespace damage, are you sending with your new
> > sendmail setup? It seems to have worked for qemu patches ...
>
> Yes, I saw some line wraps in what I received, but I checked
> the original draft to be sure and they weren't there. Possibly from
> the relay; Sigh.
>
>
> > > @@ -167,8 +166,15 @@ static void handle_tx(struct vhost_net *
> > > /* 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);
> > > - tx_poll_start(net, sock);
> > > + if (err == -EAGAIN) {
> > > + vhost_discard_desc(vq, 1);
> > > + tx_poll_start(net, sock);
> > > + } else {
> > > + vq_err(vq, "sendmsg: errno %d\n", -err);
> > > + /* drop packet; do not discard/resend */
> > > + vhost_add_used_and_signal(&net->dev, vq, head,
> > > + 0);
> >
> > vhost does not currently has a consistent error handling strategy:
> > if we drop packets, need to think which other errors should cause
> > packet drops. I prefer to just call vq_err for now,
> > and have us look at handling segfaults etc in a consistent way
> > separately.
>
> I had to add this to avoid an infinite loop when I wrote a bad
> packet on the socket. I agree error handling needs a better look,
> but retrying a bad packet continuously while dumping in the log
> is what it was doing when I hit an error before this code. Isn't
> this better, at least until a second look?
>
Hmm, what do you mean 'continuously'? Don't we only try again
on next kick?
> > > +}
> > > +
> >
> > I wonder whether it makes sense to check
> > skb_queue_empty(&sk->sk_receive_queue)
> > outside the lock, to reduce the cost of this call
> > on an empty queue (we know that it happens at least once
> > each time we exit the loop on rx)?
>
> I was looking at alternatives to adding the lock in the
> first place, but I found I couldn't measure a difference in the
> cost with and without the lock.
>
>
> > > + int retries = 0;
> > > size_t len, total_len = 0;
> > > - int err;
> > > + int err, headcount, datalen;
> > > size_t hdr_size;
> > > struct socket *sock = rcu_dereference(vq->private_data);
> > > if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
> > > @@ -222,31 +242,25 @@ static void handle_rx(struct vhost_net *
> > > 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(sock->sk))) {
> > > + headcount = vhost_get_desc_n(vq, vq->heads, datalen, &in,
> > > + vq_log, &log);
> >
> > This looks like a bug, I think we need to pass
> > datalen + header size to vhost_get_desc_n.
> > Not sure how we know the header size that backend will use though.
> > Maybe just look at our features.
>
> Yes; we have hdr_size, so I can add it here. It'll be 0 for
> the cases where the backend and guest both have vnet header (either
> the regular or larger mergeable buffers one), but should be added
> in for future raw socket support.
So hdr_size is the wrong thing to add then.
We need to add non-zero value for tap now.
> >
> > > /* OK, now we need to know about added descriptors. */
> > > - if (head == vq->num) {
> > > - if (unlikely(vhost_enable_notify(vq))) {
> > > + if (!headcount) {
> > > + if (retries == 0 && unlikely(vhost_enable_notify(vq))) {
> > > /* They have slipped one in as we were
> > > * doing that: check again. */
> > > vhost_disable_notify(vq);
> > > + retries++;
> > > continue;
> > > }
> >
> > Hmm. The reason we have the code at all, as the comment says, is because
> > guest could have added more buffers between the time we read last index
> > and the time we enabled notification. So if we just break like this
> > the race still exists. We could remember the
> > last head value we observed, and have vhost_enable_notify check
> > against this value?
>
> This is to prevent a spin loop in the case where we have some
> buffers available, but not enough for the current packet (ie, this
> is the replacement code for the "rxmaxheadcount" business). If they
> actually added something new, retrying once should see it, but what
> vhost_enable_notify() returns non-zero on is not "new buffers" but
> rather "not empty". In the case mentioned, we aren't empty, so
> vhost_enable_notify() returns nonzero every time, but the guest hasn't
> given us enough buffers to proceed, so we continuously retry; this
> code breaks the spin loop until we've really got new buffers from
> the guest.
What about the race I point out above? It seems to potentially
cause a deadlock.
> >
> > Need to think about it.
> >
> > Another concern here is that on retries vhost_get_desc_n
> > is doing extra work, rescanning the same descriptor
> > again and again. Not sure how common this is, might be
> > worthwhile to add a TODO to consider this at least.
>
> I had a printk in there to test the code and with the
> retries counter, it happens when we fill the ring (once,
> because of the retries checks), and then proceeds as
> desired when the guest gives us more buffers. Without the
> check, it spews until we hear from the guest.
I don't understand whether what you write above means 'yes this happens
and is worth optimizing for' or 'this happens very rarely
and is not worth optimizing for'.
> > > @@ -261,14 +275,33 @@ static void handle_rx(struct vhost_net *
> > > len, MSG_DONTWAIT | MSG_TRUNC);
> > > /* TODO: Check specific error and bomb out unless EAGAIN? */
> > > if (err < 0) {
> >
> > I think we need to compare err and datalen and drop packet on mismatch
> as well.
> > The check err > len won't be needed then.
>
> OK, but this is the original code, unchanged by my patch. :-)
Right. original code does not know the datalen.
> > > - vhost_discard_vq_desc(vq);
> > > + vhost_discard_desc(vq, headcount);
> > > break;
> > > }
> > > /* TODO: Should check and handle checksum. */
> > > + if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF)) {
> > > + struct virtio_net_hdr_mrg_rxbuf *vhdr =
> > > + (struct virtio_net_hdr_mrg_rxbuf *)
> > > + vq->iov[0].iov_base;
> > > + /* add num_buffers */
> > > + if (vhost_has_feature(&net->dev,
> > > + VHOST_NET_F_VIRTIO_NET_HDR))
> > > + hdr.num_buffers = headcount;
> >
> > Why is the above necessary?
>
> This adds mergeable buffer support for the raw case.
So my suggestion is to have a copy of the header
and then same code will fill in the field for
both raw and tap.
> >
> > > + else if (vq->iov[0].iov_len < sizeof(*vhdr)) {
> >
> > I think length check is already done when we copy the header. No?
>
> This sets num_buffers for the tap case (where hdr_size is 0).
Ah, right. So let's just check the total length is > sizeof(*vhdr).
> We don't copy any headers at all for this case, but we need to
> add num_buffers at offset 10 in the user buffer already read by
> recvmsg().
>
> >
> > > + vq_err(vq, "tiny buffers < %d unsupported",
> > > + vq->iov[0].iov_len);
> > > + vhost_discard_desc(vq, headcount);
> > > + break;
> >
> > Problem here is that recvmsg might modify iov.
> > This is why I think we need to use vq->hdr here.
>
> rcvmsg() can never modify the iovec;
Look at af_packet code for an example where it does.
The pointer is non-const, it's OK to modify.
tap used to modify iovec as well, the fact it doesn't
now is a side-effect of reusing same code for
recvmsg and aio read.
> it's the standard socket call.
It's an internal API that is used to implement the call.
iovec it gets is a kernel side copy of what user passed in.
> To use vq->hdr, we'd have to copy
> it in from user space, modify it, and then copy it back
> in to user space, on every packet. In the normal tap case,
> hdr_size is 0 and we read it directly from the socket to
> user space. It is already correct for mergeable buffers,
> except for the num_buffers value added here.
I don't understand what you write above, sorry.
We have iov, all we need is store the first address+length
in the hdr field.
This should be close to free and does not involve any copies
from/to userspace. All the branching and special-casing
is possibly more expensive.
> >
> > > + } else if (put_user(headcount, &vhdr->num_buffers)) {
> >
> > The above put_user writes out a 32 bit value, right?
> > This seems wrong.
>
> I'll recheck this -- I'll make the type of "headcount" be
> the type of "num_buffers" in the MRXB vnet header, if it isn't
> already.
No, I was confused Sorry about the noise.
> >
> > How about using
> > memcpy_toiovecend(vq->hdr, &headcount,
> > offsetof(struct virtio_net_hdr_mrg_rxbuf, num_buffers),
> > sizeof headcount);
> >
> > this way we also do not make any assumptions about layout.
>
> Because this overwrites the valid vnet header we got from
> the tap socket with a local copy that isn't correct.
It does? I think that this only writes out 2 bytes at an offset.
> For this to
> work, we first need to copy_from_user() the vnet header from the
> socket into vq->hdr (on every packet).
copy_from_user from vnet header to vq->hdr does not
seem to make any sense and is not what I suggested.
> It doesn't assume anything here-- it requires (and checks)
> that the user didn't give us a <12 byte buffer, which I think is
> reasonable.
> That's about the size of the descriptor for the buffer,
> and I'd call that a broken guest.
I think it's better not to assume this. virtio spec does
mentions layout assumptions as legacy limitations.
Guest can post descriptor consisting of 2 buffers:
1. 10 bytes. 2. 2 bytes. and I don't see a reason
not to support this unless this makes code simpler.
In this case code is more complex.
> The cost of supporting those
> tiny buffers in the general case would be a copy_from_user() and
> copy_to_user() of the vnet_hdr on every packet, which I think is
> not worth it (and especially since I don't expect any guest is
> ever going to give us a <12 byte buffer in the first place).
You keep saying this, but I do not see any need for extra
copy_to_user. Just use memcpy_toiovecend instead of put_user.
> > > @@ -560,9 +593,14 @@ done:
> > >
> > > static int vhost_net_set_features(struct vhost_net *n, u64 features)
> > > {
> > > - size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> > > - sizeof(struct virtio_net_hdr) : 0;
> > > + size_t hdr_size = 0;
> > > int i;
> > > +
> > > + if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)) {
> > > + hdr_size = sizeof(struct virtio_net_hdr);
> > > + if (features & (1 << VIRTIO_NET_F_MRG_RXBUF))
> > > + hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> >
> > My personal style for this would be:
> > if (!(features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)))
> > hdr_size = 0
> > else if (!(features & (1 << VIRTIO_NET_F_MRG_RXBUF)))
> > hdr_size = sizeof(virtio_net_hdr);
> > else
> > hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> >
> > which results in more symmetry and less nesting.
>
> OK.
>
> >
> > > mutex_lock(&n->dev.mutex);
> > > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > > !vhost_log_access_ok(&n->dev)) {
> > > diff -ruNp net-next-p0/drivers/vhost/vhost.c
> > > net-next-v3/drivers/vhost/vhost.c
> > > --- net-next-p0/drivers/vhost/vhost.c 2010-03-22 12:04:38.000000000
> > > -0700
> > > +++ net-next-v3/drivers/vhost/vhost.c 2010-04-06 12:57:51.000000000
> > > -0700
> > > @@ -856,6 +856,47 @@ static unsigned get_indirect(struct vhos
> > > return 0;
> > > }
> > >
> > > +/* This is a multi-buffer version of vhost_get_vq_desc
> > > + * @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, 0 on error
> >
> > This is unusual. Let's return a negative error code on error.
>
> This is like malloc -- "0" is never a valid return value, and
> we don't have actual error values to return and handle; they generate
> vq_err() messages within the code itself. In all cases,
> it is the count of buffers we allocated (which is 0 when we fail to
> allocate). So, I don't agree with this, but can do it if you insist.
malloc returns NULL, not zero.
standard error handling approaches are:
- NULL on error
- ERR_PTR on error
- >= 0 for success, -errno for error
- true for success false for error
And of course
- some custom strategy so you need to read documentation to figure out
how to use a function
which is where this one gets classified.
So I am not saying 0 won't work, just that standard stuff is better.
> >
> > > + */
> > > +int vhost_get_desc_n(struct vhost_virtqueue *vq, struct
> vring_used_elem
> > > *heads,
> > > + int datalen, int *iovcount, struct vhost_log *log,
> > > + unsigned int *log_num)
> > > +{
> > > + int out, in;
> > > + int seg = 0; /* iov index */
> > > + int hc = 0; /* head count */
> > > +
> > > + while (datalen > 0) {
> > > + if (hc >= VHOST_NET_MAX_SG)
> > > + goto err;
> > > + heads[hc].id = vhost_get_desc(vq->dev, vq, vq->iov+seg,
> > > + ARRAY_SIZE(vq->iov)-seg, &out,
> > > + &in, log, log_num);
> > > + if (heads[hc].id == vq->num)
> > > + goto err;
> > > + if (out || in <= 0) {
> > > + vq_err(vq, "unexpected descriptor format for RX: "
> > > + "out %d, in %d\n", out, in);
> > > + goto err;
> > > + }
> > > + heads[hc].len = iov_length(vq->iov+seg, in);
> > > + datalen -= heads[hc].len;
> >
> > This signed/unsigned mix makes me nervuous.
> > Let's make datalen unsigned, add unsigned total_len, and
> > while (datalen < total_len).
> >
> > > + hc++;
> > > + seg += in;
> > > + }
> > > + *iovcount = seg;
> > > + return hc;
> > > +err:
> > > + vhost_discard_desc(vq, hc);
> > > + return 0;
> > > +}
> > > +
> > > /* This looks in the virtqueue and for the first available buffer,
> and
> > > converts
> > > * it to an iovec for convenient access. Since descriptors consist
> of
> > > some
> > > * number of output then some number of input descriptors, it's
> > > actually two
> > > @@ -863,7 +904,7 @@ static unsigned get_indirect(struct vhos
> > > *
> > > * 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,
> > > +unsigned vhost_get_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)
> > > @@ -981,31 +1022,42 @@ unsigned vhost_get_vq_desc(struct vhost_
> > > }
> > >
> > > /* Reverse the effect of vhost_get_vq_desc. Useful for error
> handling.
> > > */
> > > -void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
> > > +void vhost_discard_desc(struct vhost_virtqueue *vq, int n)
> > > {
> > > - vq->last_avail_idx--;
> > > + vq->last_avail_idx -= n;
> > > }
> > >
> > > /* After we've used one of their buffers, we tell them about it.
> We'll
> > > then
> > > * want to notify the guest, using eventfd. */
> > > -int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int
> > > len)
> > > +int vhost_add_used(struct vhost_virtqueue *vq, struct vring_used_elem
> > > *heads,
> > > + int count)
> >
> > I think we are better off with vhost_add_used and vhost_add_used_n:
> > the version with _n has a lot of extra complexity, and tx always
> > adds them 1 by one.
>
> The external code only calls vhost_add_used_and_signal(), so I
> split that. I can add a single variant of vhost_add_used() too, but
> I was trying to avoid adding duplicate code and external interface.
> I don't actually agree with splitting these; isn't it better to have
> them going through the same code path whether it's single or multiple?
> A bug in one would show up as an intermittent failure, and a change
> to one requires changing both.
It's a small function so I am not really worried about maintainance.
> I don't think the multiple should just
> do the single repeatedly, either, since the multiple variant now can
> do the work in one or two copy_to_user()'s, without a loop;
Yes.
> so splitting
> these just makes a special case for single to carry in code maintenance
> with no benefit, I think.
I donnu, I have a feeling all these loops and memory accesses
on data path won't be free, and for tx this just adds overhead.
> >
> > > {
> > > struct vring_used_elem *used;
> > > + int start, n;
> > > +
> > > + if (count <= 0)
> > > + return -EINVAL;
> > >
> > > - /* The virtqueue contains a ring of used buffers. Get a pointer
> to
> > > the
> > > - * next entry in that used ring. */
> > > - used = &vq->used->ring[vq->last_used_idx % vq->num];
> > > - if (put_user(head, &used->id)) {
> > > - vq_err(vq, "Failed to write used id");
> > > + start = vq->last_used_idx % vq->num;
> > > + if (vq->num - start < count)
> > > + n = vq->num - start;
> > > + else
> > > + n = count;
> >
> > use min?
>
> Sure.
>
> >
> > > + used = vq->used->ring + start;
> > > + if (copy_to_user(used, heads, sizeof(heads[0])*n)) {
> > > + vq_err(vq, "Failed to write used");
> > > return -EFAULT;
> > > }
> > > - if (put_user(len, &used->len)) {
> > > - vq_err(vq, "Failed to write used len");
> > > - return -EFAULT;
> > > + if (n < count) { /* wrapped the ring */
> > > + used = vq->used->ring;
> > > + if (copy_to_user(used, heads+n, sizeof(heads[0])*(count-n))) {
> > > + vq_err(vq, "Failed to write used");
> > > + return -EFAULT;
> > > + }
> > > }
> > > /* Make sure buffer is written before we update index. */
> > > smp_wmb();
> > > - if (put_user(vq->last_used_idx + 1, &vq->used->idx)) {
> > > + if (put_user(vq->last_used_idx+count, &vq->used->idx)) {
> >
> > I am a bit confused ... will this write a 32 or 16 bit value?
> > count is 32 bit ... Maybe we are better off with
> > u16 idx = vq->last_used_idx + count
> > put_user(idx, &vq->used->idx)
> > vq->last_used_idx = idx
>
> How about a cast?:
>
> if (put_user((u16)(vq->last_used_idx+count), &vq->used->idx)) {
>
>
> +-DLS
No, I was confused. Code is ok (just whitespace damaged).
^ permalink raw reply
* Re: [PATCH v3] Add Mergeable receive buffer support to vhost_net
From: David Stevens @ 2010-04-07 17:37 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, kvm-owner, netdev, rusty, virtualization
In-Reply-To: <20100407105910.GD9550@redhat.com>
>
> Thanks!
> There's some whitespace damage, are you sending with your new
> sendmail setup? It seems to have worked for qemu patches ...
Yes, I saw some line wraps in what I received, but I checked
the original draft to be sure and they weren't there. Possibly from
the relay; Sigh.
> > @@ -167,8 +166,15 @@ static void handle_tx(struct vhost_net *
> > /* 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);
> > - tx_poll_start(net, sock);
> > + if (err == -EAGAIN) {
> > + vhost_discard_desc(vq, 1);
> > + tx_poll_start(net, sock);
> > + } else {
> > + vq_err(vq, "sendmsg: errno %d\n", -err);
> > + /* drop packet; do not discard/resend */
> > + vhost_add_used_and_signal(&net->dev, vq, head,
> > + 0);
>
> vhost does not currently has a consistent error handling strategy:
> if we drop packets, need to think which other errors should cause
> packet drops. I prefer to just call vq_err for now,
> and have us look at handling segfaults etc in a consistent way
> separately.
I had to add this to avoid an infinite loop when I wrote a bad
packet on the socket. I agree error handling needs a better look,
but retrying a bad packet continuously while dumping in the log
is what it was doing when I hit an error before this code. Isn't
this better, at least until a second look?
> > +}
> > +
>
> I wonder whether it makes sense to check
> skb_queue_empty(&sk->sk_receive_queue)
> outside the lock, to reduce the cost of this call
> on an empty queue (we know that it happens at least once
> each time we exit the loop on rx)?
I was looking at alternatives to adding the lock in the
first place, but I found I couldn't measure a difference in the
cost with and without the lock.
> > + int retries = 0;
> > size_t len, total_len = 0;
> > - int err;
> > + int err, headcount, datalen;
> > size_t hdr_size;
> > struct socket *sock = rcu_dereference(vq->private_data);
> > if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
> > @@ -222,31 +242,25 @@ static void handle_rx(struct vhost_net *
> > 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(sock->sk))) {
> > + headcount = vhost_get_desc_n(vq, vq->heads, datalen, &in,
> > + vq_log, &log);
>
> This looks like a bug, I think we need to pass
> datalen + header size to vhost_get_desc_n.
> Not sure how we know the header size that backend will use though.
> Maybe just look at our features.
Yes; we have hdr_size, so I can add it here. It'll be 0 for
the cases where the backend and guest both have vnet header (either
the regular or larger mergeable buffers one), but should be added
in for future raw socket support.
>
> > /* OK, now we need to know about added descriptors. */
> > - if (head == vq->num) {
> > - if (unlikely(vhost_enable_notify(vq))) {
> > + if (!headcount) {
> > + if (retries == 0 && unlikely(vhost_enable_notify(vq))) {
> > /* They have slipped one in as we were
> > * doing that: check again. */
> > vhost_disable_notify(vq);
> > + retries++;
> > continue;
> > }
>
> Hmm. The reason we have the code at all, as the comment says, is because
> guest could have added more buffers between the time we read last index
> and the time we enabled notification. So if we just break like this
> the race still exists. We could remember the
> last head value we observed, and have vhost_enable_notify check
> against this value?
This is to prevent a spin loop in the case where we have some
buffers available, but not enough for the current packet (ie, this
is the replacement code for the "rxmaxheadcount" business). If they
actually added something new, retrying once should see it, but what
vhost_enable_notify() returns non-zero on is not "new buffers" but
rather "not empty". In the case mentioned, we aren't empty, so
vhost_enable_notify() returns nonzero every time, but the guest hasn't
given us enough buffers to proceed, so we continuously retry; this
code breaks the spin loop until we've really got new buffers from
the guest.
>
> Need to think about it.
>
> Another concern here is that on retries vhost_get_desc_n
> is doing extra work, rescanning the same descriptor
> again and again. Not sure how common this is, might be
> worthwhile to add a TODO to consider this at least.
I had a printk in there to test the code and with the
retries counter, it happens when we fill the ring (once,
because of the retries checks), and then proceeds as
desired when the guest gives us more buffers. Without the
check, it spews until we hear from the guest.
> > @@ -261,14 +275,33 @@ static void handle_rx(struct vhost_net *
> > len, MSG_DONTWAIT | MSG_TRUNC);
> > /* TODO: Check specific error and bomb out unless EAGAIN? */
> > if (err < 0) {
>
> I think we need to compare err and datalen and drop packet on mismatch
as well.
> The check err > len won't be needed then.
OK, but this is the original code, unchanged by my patch. :-)
>
> > - vhost_discard_vq_desc(vq);
> > + vhost_discard_desc(vq, headcount);
> > break;
> > }
> > /* TODO: Should check and handle checksum. */
> > + if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF)) {
> > + struct virtio_net_hdr_mrg_rxbuf *vhdr =
> > + (struct virtio_net_hdr_mrg_rxbuf *)
> > + vq->iov[0].iov_base;
> > + /* add num_buffers */
> > + if (vhost_has_feature(&net->dev,
> > + VHOST_NET_F_VIRTIO_NET_HDR))
> > + hdr.num_buffers = headcount;
>
> Why is the above necessary?
This adds mergeable buffer support for the raw case.
>
> > + else if (vq->iov[0].iov_len < sizeof(*vhdr)) {
>
> I think length check is already done when we copy the header. No?
This sets num_buffers for the tap case (where hdr_size is 0).
We don't copy any headers at all for this case, but we need to
add num_buffers at offset 10 in the user buffer already read by
recvmsg().
>
> > + vq_err(vq, "tiny buffers < %d unsupported",
> > + vq->iov[0].iov_len);
> > + vhost_discard_desc(vq, headcount);
> > + break;
>
> Problem here is that recvmsg might modify iov.
> This is why I think we need to use vq->hdr here.
rcvmsg() can never modify the iovec; it's the
standard socket call. To use vq->hdr, we'd have to copy
it in from user space, modify it, and then copy it back
in to user space, on every packet. In the normal tap case,
hdr_size is 0 and we read it directly from the socket to
user space. It is already correct for mergeable buffers,
except for the num_buffers value added here.
>
> > + } else if (put_user(headcount, &vhdr->num_buffers)) {
>
> The above put_user writes out a 32 bit value, right?
> This seems wrong.
I'll recheck this -- I'll make the type of "headcount" be
the type of "num_buffers" in the MRXB vnet header, if it isn't
already.
>
> How about using
> memcpy_toiovecend(vq->hdr, &headcount,
> offsetof(struct virtio_net_hdr_mrg_rxbuf, num_buffers),
> sizeof headcount);
>
> this way we also do not make any assumptions about layout.
Because this overwrites the valid vnet header we got from
the tap socket with a local copy that isn't correct. For this to
work, we first need to copy_from_user() the vnet header from the
socket into vq->hdr (on every packet).
It doesn't assume anything here-- it requires (and checks)
that the user didn't give us a <12 byte buffer, which I think is
reasonable. That's about the size of the descriptor for the buffer,
and I'd call that a broken guest. The cost of supporting those
tiny buffers in the general case would be a copy_from_user() and
copy_to_user() of the vnet_hdr on every packet, which I think is
not worth it (and especially since I don't expect any guest is
ever going to give us a <12 byte buffer in the first place).
> > @@ -560,9 +593,14 @@ done:
> >
> > static int vhost_net_set_features(struct vhost_net *n, u64 features)
> > {
> > - size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> > - sizeof(struct virtio_net_hdr) : 0;
> > + size_t hdr_size = 0;
> > int i;
> > +
> > + if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)) {
> > + hdr_size = sizeof(struct virtio_net_hdr);
> > + if (features & (1 << VIRTIO_NET_F_MRG_RXBUF))
> > + hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>
> My personal style for this would be:
> if (!(features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)))
> hdr_size = 0
> else if (!(features & (1 << VIRTIO_NET_F_MRG_RXBUF)))
> hdr_size = sizeof(virtio_net_hdr);
> else
> hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>
> which results in more symmetry and less nesting.
OK.
>
> > mutex_lock(&n->dev.mutex);
> > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > !vhost_log_access_ok(&n->dev)) {
> > diff -ruNp net-next-p0/drivers/vhost/vhost.c
> > net-next-v3/drivers/vhost/vhost.c
> > --- net-next-p0/drivers/vhost/vhost.c 2010-03-22 12:04:38.000000000
> > -0700
> > +++ net-next-v3/drivers/vhost/vhost.c 2010-04-06 12:57:51.000000000
> > -0700
> > @@ -856,6 +856,47 @@ static unsigned get_indirect(struct vhos
> > return 0;
> > }
> >
> > +/* This is a multi-buffer version of vhost_get_vq_desc
> > + * @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, 0 on error
>
> This is unusual. Let's return a negative error code on error.
This is like malloc -- "0" is never a valid return value, and
we don't have actual error values to return and handle; they generate
vq_err() messages within the code itself. In all cases,
it is the count of buffers we allocated (which is 0 when we fail to
allocate). So, I don't agree with this, but can do it if you insist.
>
> > + */
> > +int vhost_get_desc_n(struct vhost_virtqueue *vq, struct
vring_used_elem
> > *heads,
> > + int datalen, int *iovcount, struct vhost_log *log,
> > + unsigned int *log_num)
> > +{
> > + int out, in;
> > + int seg = 0; /* iov index */
> > + int hc = 0; /* head count */
> > +
> > + while (datalen > 0) {
> > + if (hc >= VHOST_NET_MAX_SG)
> > + goto err;
> > + heads[hc].id = vhost_get_desc(vq->dev, vq, vq->iov+seg,
> > + ARRAY_SIZE(vq->iov)-seg, &out,
> > + &in, log, log_num);
> > + if (heads[hc].id == vq->num)
> > + goto err;
> > + if (out || in <= 0) {
> > + vq_err(vq, "unexpected descriptor format for RX: "
> > + "out %d, in %d\n", out, in);
> > + goto err;
> > + }
> > + heads[hc].len = iov_length(vq->iov+seg, in);
> > + datalen -= heads[hc].len;
>
> This signed/unsigned mix makes me nervuous.
> Let's make datalen unsigned, add unsigned total_len, and
> while (datalen < total_len).
>
> > + hc++;
> > + seg += in;
> > + }
> > + *iovcount = seg;
> > + return hc;
> > +err:
> > + vhost_discard_desc(vq, hc);
> > + return 0;
> > +}
> > +
> > /* This looks in the virtqueue and for the first available buffer,
and
> > converts
> > * it to an iovec for convenient access. Since descriptors consist
of
> > some
> > * number of output then some number of input descriptors, it's
> > actually two
> > @@ -863,7 +904,7 @@ static unsigned get_indirect(struct vhos
> > *
> > * 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,
> > +unsigned vhost_get_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)
> > @@ -981,31 +1022,42 @@ unsigned vhost_get_vq_desc(struct vhost_
> > }
> >
> > /* Reverse the effect of vhost_get_vq_desc. Useful for error
handling.
> > */
> > -void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
> > +void vhost_discard_desc(struct vhost_virtqueue *vq, int n)
> > {
> > - vq->last_avail_idx--;
> > + vq->last_avail_idx -= n;
> > }
> >
> > /* After we've used one of their buffers, we tell them about it.
We'll
> > then
> > * want to notify the guest, using eventfd. */
> > -int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int
> > len)
> > +int vhost_add_used(struct vhost_virtqueue *vq, struct vring_used_elem
> > *heads,
> > + int count)
>
> I think we are better off with vhost_add_used and vhost_add_used_n:
> the version with _n has a lot of extra complexity, and tx always
> adds them 1 by one.
The external code only calls vhost_add_used_and_signal(), so I
split that. I can add a single variant of vhost_add_used() too, but
I was trying to avoid adding duplicate code and external interface.
I don't actually agree with splitting these; isn't it better to have
them going through the same code path whether it's single or multiple?
A bug in one would show up as an intermittent failure, and a change
to one requires changing both. I don't think the multiple should just
do the single repeatedly, either, since the multiple variant now can
do the work in one or two copy_to_user()'s, without a loop; so splitting
these just makes a special case for single to carry in code maintenance
with no benefit, I think.
>
> > {
> > struct vring_used_elem *used;
> > + int start, n;
> > +
> > + if (count <= 0)
> > + return -EINVAL;
> >
> > - /* The virtqueue contains a ring of used buffers. Get a pointer
to
> > the
> > - * next entry in that used ring. */
> > - used = &vq->used->ring[vq->last_used_idx % vq->num];
> > - if (put_user(head, &used->id)) {
> > - vq_err(vq, "Failed to write used id");
> > + start = vq->last_used_idx % vq->num;
> > + if (vq->num - start < count)
> > + n = vq->num - start;
> > + else
> > + n = count;
>
> use min?
Sure.
>
> > + used = vq->used->ring + start;
> > + if (copy_to_user(used, heads, sizeof(heads[0])*n)) {
> > + vq_err(vq, "Failed to write used");
> > return -EFAULT;
> > }
> > - if (put_user(len, &used->len)) {
> > - vq_err(vq, "Failed to write used len");
> > - return -EFAULT;
> > + if (n < count) { /* wrapped the ring */
> > + used = vq->used->ring;
> > + if (copy_to_user(used, heads+n, sizeof(heads[0])*(count-n))) {
> > + vq_err(vq, "Failed to write used");
> > + return -EFAULT;
> > + }
> > }
> > /* Make sure buffer is written before we update index. */
> > smp_wmb();
> > - if (put_user(vq->last_used_idx + 1, &vq->used->idx)) {
> > + if (put_user(vq->last_used_idx+count, &vq->used->idx)) {
>
> I am a bit confused ... will this write a 32 or 16 bit value?
> count is 32 bit ... Maybe we are better off with
> u16 idx = vq->last_used_idx + count
> put_user(idx, &vq->used->idx)
> vq->last_used_idx = idx
How about a cast?:
if (put_user((u16)(vq->last_used_idx+count), &vq->used->idx)) {
+-DLS
^ permalink raw reply
* [GIT PULL] vhost-net fix for 2.6.34-rc3
From: Michael S. Tsirkin @ 2010-04-07 17:35 UTC (permalink / raw)
To: David Miller; +Cc: kvm, virtualization, netdev, linux-kernel, Jeff Dike
In-Reply-To: <20100318095355.GA24194@redhat.com>
David,
The following tree includes a patch fixing an issue with vhost-net in
2.6.34-rc3. Please pull for 2.6.34.
Thanks!
The following changes since commit 2eaa9cfdf33b8d7fb7aff27792192e0019ae8fc6:
Linux 2.6.34-rc3 (2010-03-30 09:24:39 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost
Jeff Dike (1):
vhost-net: fix vq_memory_access_ok error checking
drivers/vhost/vhost.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
^ permalink raw reply
* Re: [PATCH v3] Add Mergeable receive buffer support to vhost_net
From: Michael S. Tsirkin @ 2010-04-07 11:35 UTC (permalink / raw)
To: David L Stevens; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <20100407105910.GD9550@redhat.com>
Some corrections:
On Wed, Apr 07, 2010 at 01:59:10PM +0300, Michael S. Tsirkin wrote:
> On Tue, Apr 06, 2010 at 01:32:53PM -0700, David L Stevens wrote:
> >
> > This patch adds support for the Mergeable Receive Buffers feature to
> > vhost_net.
> >
> > +-DLS
> >
> > Changes from previous revision:
> > 1) renamed:
> > vhost_discard_vq_desc -> vhost_discard_desc
> > vhost_get_heads -> vhost_get_desc_n
> > vhost_get_vq_desc -> vhost_get_desc
> > 2) added heads as argument to ghost_get_desc_n
> > 3) changed "vq->heads" from iovec to vring_used_elem, removed casts
> > 4) changed vhost_add_used to do multiple elements in a single
> > copy_to_user,
> > or two when we wrap the ring.
> > 5) removed rxmaxheadcount and available buffer checks in favor of
> > running until
> > an allocation failure, but making sure we break the loop if we get
> > two in a row, indicating we have at least 1 buffer, but not enough
> > for the current receive packet
> > 6) restore non-vnet header handling
> >
> > Signed-Off-By: David L Stevens <dlstevens@us.ibm.com>
>
> Thanks!
> There's some whitespace damage, are you sending with your new
> sendmail setup? It seems to have worked for qemu patches ...
>
> > diff -ruNp net-next-p0/drivers/vhost/net.c
> > net-next-v3/drivers/vhost/net.c
> > --- net-next-p0/drivers/vhost/net.c 2010-03-22 12:04:38.000000000 -0700
> > +++ net-next-v3/drivers/vhost/net.c 2010-04-06 12:54:56.000000000 -0700
> > @@ -130,9 +130,8 @@ static void handle_tx(struct vhost_net *
> > hdr_size = vq->hdr_size;
> >
> > for (;;) {
> > - head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> > - ARRAY_SIZE(vq->iov),
> > - &out, &in,
> > + 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) {
> > @@ -167,8 +166,15 @@ static void handle_tx(struct vhost_net *
> > /* 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);
> > - tx_poll_start(net, sock);
> > + if (err == -EAGAIN) {
> > + vhost_discard_desc(vq, 1);
> > + tx_poll_start(net, sock);
> > + } else {
> > + vq_err(vq, "sendmsg: errno %d\n", -err);
> > + /* drop packet; do not discard/resend */
> > + vhost_add_used_and_signal(&net->dev, vq, head,
> > + 0);
>
> vhost does not currently has a consistent error handling strategy:
> if we drop packets, need to think which other errors should cause
> packet drops. I prefer to just call vq_err for now,
> and have us look at handling segfaults etc in a consistent way
> separately.
>
> > + }
> > break;
> > }
> > if (err != len)
> > @@ -186,12 +192,25 @@ static void handle_tx(struct vhost_net *
> > unuse_mm(net->dev.mm);
> > }
> >
> > +static int vhost_head_len(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;
> > + release_sock(sk);
> > + return len;
> > +}
> > +
>
> I wonder whether it makes sense to check
> skb_queue_empty(&sk->sk_receive_queue)
> outside the lock, to reduce the cost of this call
> on an empty queue (we know that it happens at least once
> each time we exit the loop on rx)?
>
> > /* 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,
> > @@ -202,13 +221,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
> > };
> >
> > + int retries = 0;
> > size_t len, total_len = 0;
> > - int err;
> > + int err, headcount, datalen;
> > size_t hdr_size;
> > struct socket *sock = rcu_dereference(vq->private_data);
> > if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
> > @@ -222,31 +242,25 @@ static void handle_rx(struct vhost_net *
> > 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(sock->sk))) {
> > + headcount = vhost_get_desc_n(vq, vq->heads, datalen, &in,
> > + vq_log, &log);
>
> This looks like a bug, I think we need to pass
> datalen + header size to vhost_get_desc_n.
> Not sure how we know the header size that backend will use though.
> Maybe just look at our features.
>
> > /* OK, now we need to know about added descriptors. */
> > - if (head == vq->num) {
> > - if (unlikely(vhost_enable_notify(vq))) {
> > + if (!headcount) {
> > + if (retries == 0 && unlikely(vhost_enable_notify(vq))) {
> > /* They have slipped one in as we were
> > * doing that: check again. */
> > vhost_disable_notify(vq);
> > + retries++;
> > continue;
> > }
>
> Hmm. The reason we have the code at all, as the comment says, is because
> guest could have added more buffers between the time we read last index
> and the time we enabled notification. So if we just break like this
> the race still exists. We could remember the
> last head value we observed, and have vhost_enable_notify check
> against this value?
>
> Need to think about it.
>
> Another concern here is that on retries vhost_get_desc_n
> is doing extra work, rescanning the same descriptor
> again and again. Not sure how common this is, might be
> worthwhile to add a TODO to consider this at least.
>
> > + retries = 0;
> > /* Nothing new? Wait for eventfd to tell us
> > * they refilled. */
> > 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. */
> > + /* Skip header. TODO: support TSO. */
> > s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> > msg.msg_iovlen = in;
> > len = iov_length(vq->iov, in);
> > @@ -261,14 +275,33 @@ static void handle_rx(struct vhost_net *
> > len, MSG_DONTWAIT | MSG_TRUNC);
> > /* TODO: Check specific error and bomb out unless EAGAIN? */
> > if (err < 0) {
>
> I think we need to compare err and datalen and drop packet on mismatch as well.
> The check err > len won't be needed then.
>
> > - vhost_discard_vq_desc(vq);
> > + vhost_discard_desc(vq, headcount);
> > break;
> > }
> > /* TODO: Should check and handle checksum. */
> > + if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF)) {
> > + struct virtio_net_hdr_mrg_rxbuf *vhdr =
> > + (struct virtio_net_hdr_mrg_rxbuf *)
> > + vq->iov[0].iov_base;
> > + /* add num_buffers */
> > + if (vhost_has_feature(&net->dev,
> > + VHOST_NET_F_VIRTIO_NET_HDR))
> > + hdr.num_buffers = headcount;
>
> Why is the above necessary?
>
> > + else if (vq->iov[0].iov_len < sizeof(*vhdr)) {
>
> I think length check is already done when we copy the header. No?
>
> > + vq_err(vq, "tiny buffers < %d unsupported",
> > + vq->iov[0].iov_len);
> > + vhost_discard_desc(vq, headcount);
> > + break;
>
> Problem here is that recvmsg might modify iov.
> This is why I think we need to use vq->hdr here.
>
> > + } else if (put_user(headcount, &vhdr->num_buffers)) {
>
> The above put_user writes out a 32 bit value, right?
> This seems wrong.
Sorry, put_user looks at the pointer type, so that's ok.
I still suggest memcpy_toiovecend to remove layout assumptions.
> How about using
> memcpy_toiovecend(vq->hdr, &headcount,
> offsetof(struct virtio_net_hdr_mrg_rxbuf, num_buffers),
> sizeof headcount);
>
> this way we also do not make any assumptions about layout.
>
> > + vq_err(vq, "Failed num_buffers write");
> > + vhost_discard_desc(vq, headcount);
> > + break;
> > + }
> > + }
> > if (err > len) {
> > pr_err("Discarded truncated rx packet: "
> > " len %d > %zd\n", err, len);
> > - vhost_discard_vq_desc(vq);
> > + vhost_discard_desc(vq, headcount);
> > continue;
> > }
> > len = err;
> > @@ -279,7 +312,7 @@ static void handle_rx(struct vhost_net *
> > break;
> > }
> > len += hdr_size;
> > - vhost_add_used_and_signal(&net->dev, vq, head, len);
> > + 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;
> > @@ -560,9 +593,14 @@ done:
> >
> > static int vhost_net_set_features(struct vhost_net *n, u64 features)
> > {
> > - size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> > - sizeof(struct virtio_net_hdr) : 0;
> > + size_t hdr_size = 0;
> > int i;
> > +
> > + if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)) {
> > + hdr_size = sizeof(struct virtio_net_hdr);
> > + if (features & (1 << VIRTIO_NET_F_MRG_RXBUF))
> > + hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>
> My personal style for this would be:
> if (!(features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)))
> hdr_size = 0
> else if (!(features & (1 << VIRTIO_NET_F_MRG_RXBUF)))
> hdr_size = sizeof(virtio_net_hdr);
> else
> hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>
> which results in more symmetry and less nesting.
>
> > mutex_lock(&n->dev.mutex);
> > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > !vhost_log_access_ok(&n->dev)) {
> > diff -ruNp net-next-p0/drivers/vhost/vhost.c
> > net-next-v3/drivers/vhost/vhost.c
> > --- net-next-p0/drivers/vhost/vhost.c 2010-03-22 12:04:38.000000000
> > -0700
> > +++ net-next-v3/drivers/vhost/vhost.c 2010-04-06 12:57:51.000000000
> > -0700
> > @@ -856,6 +856,47 @@ static unsigned get_indirect(struct vhos
> > return 0;
> > }
> >
> > +/* This is a multi-buffer version of vhost_get_vq_desc
> > + * @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, 0 on error
>
> This is unusual. Let's return a negative error code on error.
>
> > + */
> > +int vhost_get_desc_n(struct vhost_virtqueue *vq, struct vring_used_elem
> > *heads,
> > + int datalen, int *iovcount, struct vhost_log *log,
> > + unsigned int *log_num)
> > +{
> > + int out, in;
> > + int seg = 0; /* iov index */
> > + int hc = 0; /* head count */
> > +
> > + while (datalen > 0) {
> > + if (hc >= VHOST_NET_MAX_SG)
> > + goto err;
> > + heads[hc].id = vhost_get_desc(vq->dev, vq, vq->iov+seg,
> > + ARRAY_SIZE(vq->iov)-seg, &out,
> > + &in, log, log_num);
> > + if (heads[hc].id == vq->num)
> > + goto err;
> > + if (out || in <= 0) {
> > + vq_err(vq, "unexpected descriptor format for RX: "
> > + "out %d, in %d\n", out, in);
> > + goto err;
> > + }
> > + heads[hc].len = iov_length(vq->iov+seg, in);
> > + datalen -= heads[hc].len;
>
> This signed/unsigned mix makes me nervuous.
> Let's make datalen unsigned, add unsigned total_len, and
> while (datalen < total_len).
>
> > + hc++;
> > + seg += in;
> > + }
> > + *iovcount = seg;
> > + return hc;
> > +err:
> > + vhost_discard_desc(vq, hc);
> > + return 0;
> > +}
> > +
> > /* This looks in the virtqueue and for the first available buffer, and
> > converts
> > * it to an iovec for convenient access. Since descriptors consist of
> > some
> > * number of output then some number of input descriptors, it's
> > actually two
> > @@ -863,7 +904,7 @@ static unsigned get_indirect(struct vhos
> > *
> > * 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,
> > +unsigned vhost_get_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)
> > @@ -981,31 +1022,42 @@ unsigned vhost_get_vq_desc(struct vhost_
> > }
> >
> > /* Reverse the effect of vhost_get_vq_desc. Useful for error handling.
> > */
> > -void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
> > +void vhost_discard_desc(struct vhost_virtqueue *vq, int n)
> > {
> > - vq->last_avail_idx--;
> > + vq->last_avail_idx -= n;
> > }
> >
> > /* After we've used one of their buffers, we tell them about it. We'll
> > then
> > * want to notify the guest, using eventfd. */
> > -int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int
> > len)
> > +int vhost_add_used(struct vhost_virtqueue *vq, struct vring_used_elem
> > *heads,
> > + int count)
>
> I think we are better off with vhost_add_used and vhost_add_used_n:
> the version with _n has a lot of extra complexity, and tx always
> adds them 1 by one.
>
> > {
> > struct vring_used_elem *used;
> > + int start, n;
> > +
> > + if (count <= 0)
> > + return -EINVAL;
> >
> > - /* The virtqueue contains a ring of used buffers. Get a pointer to
> > the
> > - * next entry in that used ring. */
> > - used = &vq->used->ring[vq->last_used_idx % vq->num];
> > - if (put_user(head, &used->id)) {
> > - vq_err(vq, "Failed to write used id");
> > + start = vq->last_used_idx % vq->num;
> > + if (vq->num - start < count)
> > + n = vq->num - start;
> > + else
> > + n = count;
>
> use min?
>
> > + used = vq->used->ring + start;
> > + if (copy_to_user(used, heads, sizeof(heads[0])*n)) {
> > + vq_err(vq, "Failed to write used");
> > return -EFAULT;
> > }
> > - if (put_user(len, &used->len)) {
> > - vq_err(vq, "Failed to write used len");
> > - return -EFAULT;
> > + if (n < count) { /* wrapped the ring */
> > + used = vq->used->ring;
> > + if (copy_to_user(used, heads+n, sizeof(heads[0])*(count-n))) {
> > + vq_err(vq, "Failed to write used");
> > + return -EFAULT;
> > + }
> > }
> > /* Make sure buffer is written before we update index. */
> > smp_wmb();
> > - if (put_user(vq->last_used_idx + 1, &vq->used->idx)) {
> > + if (put_user(vq->last_used_idx+count, &vq->used->idx)) {
>
> I am a bit confused ... will this write a 32 or 16 bit value?
> count is 32 bit ... Maybe we are better off with
> u16 idx = vq->last_used_idx + count
> put_user(idx, &vq->used->idx)
> vq->last_used_idx = idx
The above's not necessary, put_user gets type from the pointer.
> > vq_err(vq, "Failed to increment used idx");
> > return -EFAULT;
> > }
> > @@ -1023,7 +1075,7 @@ int vhost_add_used(struct vhost_virtqueu
> > if (vq->log_ctx)
> > eventfd_signal(vq->log_ctx, 1);
> > }
> > - vq->last_used_idx++;
> > + vq->last_used_idx += count;
> > return 0;
> > }
> >
> > @@ -1049,10 +1101,23 @@ void vhost_signal(struct vhost_dev *dev,
> >
> > /* And here's the combo meal deal. Supersize me! */
> > void vhost_add_used_and_signal(struct vhost_dev *dev,
> > - struct vhost_virtqueue *vq,
> > - unsigned int head, int len)
> > + struct vhost_virtqueue *vq, unsigned int id,
> > + int len)
> > +{
> > + struct vring_used_elem head;
> > +
> > + head.id = id;
> > + head.len = len;
> > + vhost_add_used(vq, &head, 1);
> > + vhost_signal(dev, vq);
> > +}
> > +
> > +/* multi-buffer version of vhost_add_used_and_signal */
> > +void vhost_add_used_and_signal_n(struct vhost_dev *dev,
> > + struct vhost_virtqueue *vq,
> > + struct vring_used_elem *heads, int count)
> > {
> > - vhost_add_used(vq, head, len);
> > + vhost_add_used(vq, heads, count);
> > vhost_signal(dev, vq);
> > }
> >
> > diff -ruNp net-next-p0/drivers/vhost/vhost.h
> > net-next-v3/drivers/vhost/vhost.h
> > --- net-next-p0/drivers/vhost/vhost.h 2010-03-22 12:04:38.000000000
> > -0700
> > +++ net-next-v3/drivers/vhost/vhost.h 2010-04-05 20:33:57.000000000
> > -0700
> > @@ -85,6 +85,7 @@ struct vhost_virtqueue {
> > struct iovec iov[VHOST_NET_MAX_SG];
> > struct iovec hdr[VHOST_NET_MAX_SG];
> > size_t hdr_size;
> > + struct vring_used_elem heads[VHOST_NET_MAX_SG];
> > /* We use a kind of RCU to access private pointer.
> > * All readers access it from workqueue, which makes it possible to
> > * flush the workqueue instead of synchronize_rcu. Therefore readers
> > do
> > @@ -120,16 +121,22 @@ long vhost_dev_ioctl(struct vhost_dev *,
> > 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
> > *,
> > +int vhost_get_desc_n(struct vhost_virtqueue *, struct vring_used_elem
> > *heads,
> > + int datalen, int *iovcount, struct vhost_log *log,
> > + unsigned int *log_num);
> > +unsigned vhost_get_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 *);
> > +void vhost_discard_desc(struct vhost_virtqueue *, int);
> >
> > -int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int
> > len);
> > -void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
> > +int vhost_add_used(struct vhost_virtqueue *, struct vring_used_elem
> > *heads,
> > + int count);
> > void vhost_add_used_and_signal(struct vhost_dev *, struct
> > vhost_virtqueue *,
> > - unsigned int head, int len);
> > + unsigned int id, int len);
> > +void vhost_add_used_and_signal_n(struct vhost_dev *, struct
> > vhost_virtqueue *,
> > + struct vring_used_elem *heads, int count);
> > +void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
> > void vhost_disable_notify(struct vhost_virtqueue *);
> > bool vhost_enable_notify(struct vhost_virtqueue *);
> >
> > @@ -149,7 +156,8 @@ enum {
> > VHOST_FEATURES = (1 << VIRTIO_F_NOTIFY_ON_EMPTY) |
> > (1 << VIRTIO_RING_F_INDIRECT_DESC) |
> > (1 << VHOST_F_LOG_ALL) |
> > - (1 << VHOST_NET_F_VIRTIO_NET_HDR),
> > + (1 << VHOST_NET_F_VIRTIO_NET_HDR) |
> > + (1 << VIRTIO_NET_F_MRG_RXBUF),
> > };
> >
> > static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
> >
^ permalink raw reply
* Re: [PATCH v3] Add Mergeable receive buffer support to vhost_net
From: Michael S. Tsirkin @ 2010-04-07 10:59 UTC (permalink / raw)
To: David L Stevens; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <1270585973.28407.3.camel@lab1.dls>
On Tue, Apr 06, 2010 at 01:32:53PM -0700, David L Stevens wrote:
>
> This patch adds support for the Mergeable Receive Buffers feature to
> vhost_net.
>
> +-DLS
>
> Changes from previous revision:
> 1) renamed:
> vhost_discard_vq_desc -> vhost_discard_desc
> vhost_get_heads -> vhost_get_desc_n
> vhost_get_vq_desc -> vhost_get_desc
> 2) added heads as argument to ghost_get_desc_n
> 3) changed "vq->heads" from iovec to vring_used_elem, removed casts
> 4) changed vhost_add_used to do multiple elements in a single
> copy_to_user,
> or two when we wrap the ring.
> 5) removed rxmaxheadcount and available buffer checks in favor of
> running until
> an allocation failure, but making sure we break the loop if we get
> two in a row, indicating we have at least 1 buffer, but not enough
> for the current receive packet
> 6) restore non-vnet header handling
>
> Signed-Off-By: David L Stevens <dlstevens@us.ibm.com>
Thanks!
There's some whitespace damage, are you sending with your new
sendmail setup? It seems to have worked for qemu patches ...
> diff -ruNp net-next-p0/drivers/vhost/net.c
> net-next-v3/drivers/vhost/net.c
> --- net-next-p0/drivers/vhost/net.c 2010-03-22 12:04:38.000000000 -0700
> +++ net-next-v3/drivers/vhost/net.c 2010-04-06 12:54:56.000000000 -0700
> @@ -130,9 +130,8 @@ static void handle_tx(struct vhost_net *
> hdr_size = vq->hdr_size;
>
> for (;;) {
> - head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> - ARRAY_SIZE(vq->iov),
> - &out, &in,
> + 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) {
> @@ -167,8 +166,15 @@ static void handle_tx(struct vhost_net *
> /* 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);
> - tx_poll_start(net, sock);
> + if (err == -EAGAIN) {
> + vhost_discard_desc(vq, 1);
> + tx_poll_start(net, sock);
> + } else {
> + vq_err(vq, "sendmsg: errno %d\n", -err);
> + /* drop packet; do not discard/resend */
> + vhost_add_used_and_signal(&net->dev, vq, head,
> + 0);
vhost does not currently has a consistent error handling strategy:
if we drop packets, need to think which other errors should cause
packet drops. I prefer to just call vq_err for now,
and have us look at handling segfaults etc in a consistent way
separately.
> + }
> break;
> }
> if (err != len)
> @@ -186,12 +192,25 @@ static void handle_tx(struct vhost_net *
> unuse_mm(net->dev.mm);
> }
>
> +static int vhost_head_len(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;
> + release_sock(sk);
> + return len;
> +}
> +
I wonder whether it makes sense to check
skb_queue_empty(&sk->sk_receive_queue)
outside the lock, to reduce the cost of this call
on an empty queue (we know that it happens at least once
each time we exit the loop on rx)?
> /* 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,
> @@ -202,13 +221,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
> };
>
> + int retries = 0;
> size_t len, total_len = 0;
> - int err;
> + int err, headcount, datalen;
> size_t hdr_size;
> struct socket *sock = rcu_dereference(vq->private_data);
> if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
> @@ -222,31 +242,25 @@ static void handle_rx(struct vhost_net *
> 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(sock->sk))) {
> + headcount = vhost_get_desc_n(vq, vq->heads, datalen, &in,
> + vq_log, &log);
This looks like a bug, I think we need to pass
datalen + header size to vhost_get_desc_n.
Not sure how we know the header size that backend will use though.
Maybe just look at our features.
> /* OK, now we need to know about added descriptors. */
> - if (head == vq->num) {
> - if (unlikely(vhost_enable_notify(vq))) {
> + if (!headcount) {
> + if (retries == 0 && unlikely(vhost_enable_notify(vq))) {
> /* They have slipped one in as we were
> * doing that: check again. */
> vhost_disable_notify(vq);
> + retries++;
> continue;
> }
Hmm. The reason we have the code at all, as the comment says, is because
guest could have added more buffers between the time we read last index
and the time we enabled notification. So if we just break like this
the race still exists. We could remember the
last head value we observed, and have vhost_enable_notify check
against this value?
Need to think about it.
Another concern here is that on retries vhost_get_desc_n
is doing extra work, rescanning the same descriptor
again and again. Not sure how common this is, might be
worthwhile to add a TODO to consider this at least.
> + retries = 0;
> /* Nothing new? Wait for eventfd to tell us
> * they refilled. */
> 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. */
> + /* Skip header. TODO: support TSO. */
> s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> msg.msg_iovlen = in;
> len = iov_length(vq->iov, in);
> @@ -261,14 +275,33 @@ static void handle_rx(struct vhost_net *
> len, MSG_DONTWAIT | MSG_TRUNC);
> /* TODO: Check specific error and bomb out unless EAGAIN? */
> if (err < 0) {
I think we need to compare err and datalen and drop packet on mismatch as well.
The check err > len won't be needed then.
> - vhost_discard_vq_desc(vq);
> + vhost_discard_desc(vq, headcount);
> break;
> }
> /* TODO: Should check and handle checksum. */
> + if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF)) {
> + struct virtio_net_hdr_mrg_rxbuf *vhdr =
> + (struct virtio_net_hdr_mrg_rxbuf *)
> + vq->iov[0].iov_base;
> + /* add num_buffers */
> + if (vhost_has_feature(&net->dev,
> + VHOST_NET_F_VIRTIO_NET_HDR))
> + hdr.num_buffers = headcount;
Why is the above necessary?
> + else if (vq->iov[0].iov_len < sizeof(*vhdr)) {
I think length check is already done when we copy the header. No?
> + vq_err(vq, "tiny buffers < %d unsupported",
> + vq->iov[0].iov_len);
> + vhost_discard_desc(vq, headcount);
> + break;
Problem here is that recvmsg might modify iov.
This is why I think we need to use vq->hdr here.
> + } else if (put_user(headcount, &vhdr->num_buffers)) {
The above put_user writes out a 32 bit value, right?
This seems wrong.
How about using
memcpy_toiovecend(vq->hdr, &headcount,
offsetof(struct virtio_net_hdr_mrg_rxbuf, num_buffers),
sizeof headcount);
this way we also do not make any assumptions about layout.
> + vq_err(vq, "Failed num_buffers write");
> + vhost_discard_desc(vq, headcount);
> + break;
> + }
> + }
> if (err > len) {
> pr_err("Discarded truncated rx packet: "
> " len %d > %zd\n", err, len);
> - vhost_discard_vq_desc(vq);
> + vhost_discard_desc(vq, headcount);
> continue;
> }
> len = err;
> @@ -279,7 +312,7 @@ static void handle_rx(struct vhost_net *
> break;
> }
> len += hdr_size;
> - vhost_add_used_and_signal(&net->dev, vq, head, len);
> + 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;
> @@ -560,9 +593,14 @@ done:
>
> static int vhost_net_set_features(struct vhost_net *n, u64 features)
> {
> - size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> - sizeof(struct virtio_net_hdr) : 0;
> + size_t hdr_size = 0;
> int i;
> +
> + if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)) {
> + hdr_size = sizeof(struct virtio_net_hdr);
> + if (features & (1 << VIRTIO_NET_F_MRG_RXBUF))
> + hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
My personal style for this would be:
if (!(features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)))
hdr_size = 0
else if (!(features & (1 << VIRTIO_NET_F_MRG_RXBUF)))
hdr_size = sizeof(virtio_net_hdr);
else
hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
which results in more symmetry and less nesting.
> mutex_lock(&n->dev.mutex);
> if ((features & (1 << VHOST_F_LOG_ALL)) &&
> !vhost_log_access_ok(&n->dev)) {
> diff -ruNp net-next-p0/drivers/vhost/vhost.c
> net-next-v3/drivers/vhost/vhost.c
> --- net-next-p0/drivers/vhost/vhost.c 2010-03-22 12:04:38.000000000
> -0700
> +++ net-next-v3/drivers/vhost/vhost.c 2010-04-06 12:57:51.000000000
> -0700
> @@ -856,6 +856,47 @@ static unsigned get_indirect(struct vhos
> return 0;
> }
>
> +/* This is a multi-buffer version of vhost_get_vq_desc
> + * @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, 0 on error
This is unusual. Let's return a negative error code on error.
> + */
> +int vhost_get_desc_n(struct vhost_virtqueue *vq, struct vring_used_elem
> *heads,
> + int datalen, int *iovcount, struct vhost_log *log,
> + unsigned int *log_num)
> +{
> + int out, in;
> + int seg = 0; /* iov index */
> + int hc = 0; /* head count */
> +
> + while (datalen > 0) {
> + if (hc >= VHOST_NET_MAX_SG)
> + goto err;
> + heads[hc].id = vhost_get_desc(vq->dev, vq, vq->iov+seg,
> + ARRAY_SIZE(vq->iov)-seg, &out,
> + &in, log, log_num);
> + if (heads[hc].id == vq->num)
> + goto err;
> + if (out || in <= 0) {
> + vq_err(vq, "unexpected descriptor format for RX: "
> + "out %d, in %d\n", out, in);
> + goto err;
> + }
> + heads[hc].len = iov_length(vq->iov+seg, in);
> + datalen -= heads[hc].len;
This signed/unsigned mix makes me nervuous.
Let's make datalen unsigned, add unsigned total_len, and
while (datalen < total_len).
> + hc++;
> + seg += in;
> + }
> + *iovcount = seg;
> + return hc;
> +err:
> + vhost_discard_desc(vq, hc);
> + return 0;
> +}
> +
> /* This looks in the virtqueue and for the first available buffer, and
> converts
> * it to an iovec for convenient access. Since descriptors consist of
> some
> * number of output then some number of input descriptors, it's
> actually two
> @@ -863,7 +904,7 @@ static unsigned get_indirect(struct vhos
> *
> * 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,
> +unsigned vhost_get_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)
> @@ -981,31 +1022,42 @@ unsigned vhost_get_vq_desc(struct vhost_
> }
>
> /* Reverse the effect of vhost_get_vq_desc. Useful for error handling.
> */
> -void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
> +void vhost_discard_desc(struct vhost_virtqueue *vq, int n)
> {
> - vq->last_avail_idx--;
> + vq->last_avail_idx -= n;
> }
>
> /* After we've used one of their buffers, we tell them about it. We'll
> then
> * want to notify the guest, using eventfd. */
> -int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int
> len)
> +int vhost_add_used(struct vhost_virtqueue *vq, struct vring_used_elem
> *heads,
> + int count)
I think we are better off with vhost_add_used and vhost_add_used_n:
the version with _n has a lot of extra complexity, and tx always
adds them 1 by one.
> {
> struct vring_used_elem *used;
> + int start, n;
> +
> + if (count <= 0)
> + return -EINVAL;
>
> - /* The virtqueue contains a ring of used buffers. Get a pointer to
> the
> - * next entry in that used ring. */
> - used = &vq->used->ring[vq->last_used_idx % vq->num];
> - if (put_user(head, &used->id)) {
> - vq_err(vq, "Failed to write used id");
> + start = vq->last_used_idx % vq->num;
> + if (vq->num - start < count)
> + n = vq->num - start;
> + else
> + n = count;
use min?
> + used = vq->used->ring + start;
> + if (copy_to_user(used, heads, sizeof(heads[0])*n)) {
> + vq_err(vq, "Failed to write used");
> return -EFAULT;
> }
> - if (put_user(len, &used->len)) {
> - vq_err(vq, "Failed to write used len");
> - return -EFAULT;
> + if (n < count) { /* wrapped the ring */
> + used = vq->used->ring;
> + if (copy_to_user(used, heads+n, sizeof(heads[0])*(count-n))) {
> + vq_err(vq, "Failed to write used");
> + return -EFAULT;
> + }
> }
> /* Make sure buffer is written before we update index. */
> smp_wmb();
> - if (put_user(vq->last_used_idx + 1, &vq->used->idx)) {
> + if (put_user(vq->last_used_idx+count, &vq->used->idx)) {
I am a bit confused ... will this write a 32 or 16 bit value?
count is 32 bit ... Maybe we are better off with
u16 idx = vq->last_used_idx + count
put_user(idx, &vq->used->idx)
vq->last_used_idx = idx
> vq_err(vq, "Failed to increment used idx");
> return -EFAULT;
> }
> @@ -1023,7 +1075,7 @@ int vhost_add_used(struct vhost_virtqueu
> if (vq->log_ctx)
> eventfd_signal(vq->log_ctx, 1);
> }
> - vq->last_used_idx++;
> + vq->last_used_idx += count;
> return 0;
> }
>
> @@ -1049,10 +1101,23 @@ void vhost_signal(struct vhost_dev *dev,
>
> /* And here's the combo meal deal. Supersize me! */
> void vhost_add_used_and_signal(struct vhost_dev *dev,
> - struct vhost_virtqueue *vq,
> - unsigned int head, int len)
> + struct vhost_virtqueue *vq, unsigned int id,
> + int len)
> +{
> + struct vring_used_elem head;
> +
> + head.id = id;
> + head.len = len;
> + vhost_add_used(vq, &head, 1);
> + vhost_signal(dev, vq);
> +}
> +
> +/* multi-buffer version of vhost_add_used_and_signal */
> +void vhost_add_used_and_signal_n(struct vhost_dev *dev,
> + struct vhost_virtqueue *vq,
> + struct vring_used_elem *heads, int count)
> {
> - vhost_add_used(vq, head, len);
> + vhost_add_used(vq, heads, count);
> vhost_signal(dev, vq);
> }
>
> diff -ruNp net-next-p0/drivers/vhost/vhost.h
> net-next-v3/drivers/vhost/vhost.h
> --- net-next-p0/drivers/vhost/vhost.h 2010-03-22 12:04:38.000000000
> -0700
> +++ net-next-v3/drivers/vhost/vhost.h 2010-04-05 20:33:57.000000000
> -0700
> @@ -85,6 +85,7 @@ struct vhost_virtqueue {
> struct iovec iov[VHOST_NET_MAX_SG];
> struct iovec hdr[VHOST_NET_MAX_SG];
> size_t hdr_size;
> + struct vring_used_elem heads[VHOST_NET_MAX_SG];
> /* We use a kind of RCU to access private pointer.
> * All readers access it from workqueue, which makes it possible to
> * flush the workqueue instead of synchronize_rcu. Therefore readers
> do
> @@ -120,16 +121,22 @@ long vhost_dev_ioctl(struct vhost_dev *,
> 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
> *,
> +int vhost_get_desc_n(struct vhost_virtqueue *, struct vring_used_elem
> *heads,
> + int datalen, int *iovcount, struct vhost_log *log,
> + unsigned int *log_num);
> +unsigned vhost_get_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 *);
> +void vhost_discard_desc(struct vhost_virtqueue *, int);
>
> -int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int
> len);
> -void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
> +int vhost_add_used(struct vhost_virtqueue *, struct vring_used_elem
> *heads,
> + int count);
> void vhost_add_used_and_signal(struct vhost_dev *, struct
> vhost_virtqueue *,
> - unsigned int head, int len);
> + unsigned int id, int len);
> +void vhost_add_used_and_signal_n(struct vhost_dev *, struct
> vhost_virtqueue *,
> + struct vring_used_elem *heads, int count);
> +void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
> void vhost_disable_notify(struct vhost_virtqueue *);
> bool vhost_enable_notify(struct vhost_virtqueue *);
>
> @@ -149,7 +156,8 @@ enum {
> VHOST_FEATURES = (1 << VIRTIO_F_NOTIFY_ON_EMPTY) |
> (1 << VIRTIO_RING_F_INDIRECT_DESC) |
> (1 << VHOST_F_LOG_ALL) |
> - (1 << VHOST_NET_F_VIRTIO_NET_HDR),
> + (1 << VHOST_NET_F_VIRTIO_NET_HDR) |
> + (1 << VIRTIO_NET_F_MRG_RXBUF),
> };
>
> static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
>
^ permalink raw reply
* [RFC] vhost-blk implementation (v2)
From: Badari Pulavarty @ 2010-04-07 0:36 UTC (permalink / raw)
To: kvm, virtualization, qemu-devel
In-Reply-To: <20100405195912.GA17589@infradead.org>
Hi All,
Here is the latest version of vhost-blk implementation.
Major difference from my previous implementation is that, I
now merge all contiguous requests (both read and write), before
submitting them. This significantly improved IO performance.
I am still collecting performance numbers, I will be posting
in next few days.
Comments ?
Todo:
- Address hch's comments on annontations
- Implement per device read/write queues
- Finish up error handling
Thanks,
Badari
---
drivers/vhost/blk.c | 445 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 445 insertions(+)
Index: net-next/drivers/vhost/blk.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ net-next/drivers/vhost/blk.c 2010-04-06 16:38:03.563847905 -0400
@@ -0,0 +1,445 @@
+ /*
+ * virtio-block server in host kernel.
+ * Inspired by vhost-net and shamlessly ripped code from it :)
+ */
+
+#include <linux/compat.h>
+#include <linux/eventfd.h>
+#include <linux/vhost.h>
+#include <linux/virtio_net.h>
+#include <linux/virtio_blk.h>
+#include <linux/mmu_context.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/workqueue.h>
+#include <linux/rcupdate.h>
+#include <linux/file.h>
+
+#include "vhost.h"
+
+#define VHOST_BLK_VQ_MAX 1
+#define SECTOR_SHIFT 9
+
+struct vhost_blk {
+ struct vhost_dev dev;
+ struct vhost_virtqueue vqs[VHOST_BLK_VQ_MAX];
+ struct vhost_poll poll[VHOST_BLK_VQ_MAX];
+};
+
+struct vhost_blk_io {
+ struct list_head list;
+ struct work_struct work;
+ struct vhost_blk *blk;
+ struct file *file;
+ int head;
+ uint32_t type;
+ uint32_t nvecs;
+ uint64_t sector;
+ uint64_t len;
+ struct iovec iov[0];
+};
+
+static struct workqueue_struct *vblk_workqueue;
+static LIST_HEAD(write_queue);
+static LIST_HEAD(read_queue);
+
+static void handle_io_work(struct work_struct *work)
+{
+ struct vhost_blk_io *vbio, *entry;
+ struct vhost_virtqueue *vq;
+ struct vhost_blk *blk;
+ struct list_head single, *head, *node, *tmp;
+
+ int i, need_free, ret = 0;
+ loff_t pos;
+ uint8_t status = 0;
+
+ vbio = container_of(work, struct vhost_blk_io, work);
+ blk = vbio->blk;
+ vq = &blk->dev.vqs[0];
+ pos = vbio->sector << 8;
+
+ use_mm(blk->dev.mm);
+ if (vbio->type & VIRTIO_BLK_T_FLUSH) {
+ ret = vfs_fsync(vbio->file, vbio->file->f_path.dentry, 1);
+ } else if (vbio->type & VIRTIO_BLK_T_OUT) {
+ ret = vfs_writev(vbio->file, vbio->iov, vbio->nvecs, &pos);
+ } else {
+ ret = vfs_readv(vbio->file, vbio->iov, vbio->nvecs, &pos);
+ }
+ status = (ret < 0) ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
+ if (vbio->head != -1) {
+ INIT_LIST_HEAD(&single);
+ list_add(&vbio->list, &single);
+ head = &single;
+ need_free = 0;
+ } else {
+ head = &vbio->list;
+ need_free = 1;
+ }
+ list_for_each_entry(entry, head, list) {
+ copy_to_user(entry->iov[entry->nvecs].iov_base, &status, sizeof status);
+ }
+ mutex_lock(&vq->mutex);
+ list_for_each_safe(node, tmp, head) {
+ entry = list_entry(node, struct vhost_blk_io, list);
+ vhost_add_used_and_signal(&blk->dev, vq, entry->head, ret);
+ list_del(node);
+ kfree(entry);
+ }
+ mutex_unlock(&vq->mutex);
+ unuse_mm(blk->dev.mm);
+ if (need_free)
+ kfree(vbio);
+}
+
+static struct vhost_blk_io *allocate_vbio(int nvecs)
+{
+ struct vhost_blk_io *vbio;
+ int size = sizeof(struct vhost_blk_io) + nvecs * sizeof(struct iovec);
+ vbio = kmalloc(size, GFP_KERNEL);
+ if (vbio) {
+ INIT_WORK(&vbio->work, handle_io_work);
+ INIT_LIST_HEAD(&vbio->list);
+ }
+ return vbio;
+}
+
+static void merge_and_handoff_work(struct list_head *queue)
+{
+ struct vhost_blk_io *vbio, *entry;
+ int nvecs = 0;
+ int entries = 0;
+
+ list_for_each_entry(entry, queue, list) {
+ nvecs += entry->nvecs;
+ entries++;
+ }
+
+ if (entries == 1) {
+ vbio = list_first_entry(queue, struct vhost_blk_io, list);
+ list_del(&vbio->list);
+ queue_work(vblk_workqueue, &vbio->work);
+ return;
+ }
+
+ vbio = allocate_vbio(nvecs);
+ if (!vbio) {
+ /* Unable to allocate memory - submit IOs individually */
+ list_for_each_entry(vbio, queue, list) {
+ queue_work(vblk_workqueue, &vbio->work);
+ }
+ INIT_LIST_HEAD(queue);
+ return;
+ }
+
+ entry = list_first_entry(queue, struct vhost_blk_io, list);
+ vbio->nvecs = nvecs;
+ vbio->blk = entry->blk;
+ vbio->file = entry->file;
+ vbio->type = entry->type;
+ vbio->sector = entry->sector;
+ vbio->head = -1;
+ vbio->len = 0;
+ nvecs = 0;
+
+ list_for_each_entry(entry, queue, list) {
+ memcpy(&vbio->iov[nvecs], entry->iov, entry->nvecs * sizeof(struct iovec));
+ nvecs += entry->nvecs;
+ vbio->len += entry->len;
+ }
+ list_replace_init(queue, &vbio->list);
+ queue_work(vblk_workqueue, &vbio->work);
+}
+
+static void start_io(struct list_head *queue)
+{
+ struct list_head start;
+ struct vhost_blk_io *vbio = NULL, *entry;
+
+ if (list_empty(queue))
+ return;
+
+ list_for_each_entry(entry, queue, list) {
+ if (!vbio) {
+ vbio = entry;
+ continue;
+ }
+ if (vbio->sector + (vbio->len >> SECTOR_SHIFT) == entry->sector) {
+ vbio = entry;
+ } else {
+ INIT_LIST_HEAD(&start);
+ list_cut_position(&start, queue, &vbio->list);
+ merge_and_handoff_work(&start);
+ vbio = entry;
+ }
+ }
+ if (!list_empty(queue))
+ merge_and_handoff_work(queue);
+}
+
+static uint64_t calculate_len(struct iovec *iov, int nvecs)
+{
+ uint64_t len = 0;
+ int i;
+
+ for (i=0; i<nvecs; i++)
+ len += iov[i].iov_len;
+ return len;
+}
+
+static void insert_to_queue(struct vhost_blk_io *vbio,
+ struct list_head *queue)
+{
+ struct vhost_blk_io *entry;
+
+ list_for_each_entry(entry, queue, list) {
+ if (entry->sector > vbio->sector)
+ break;
+ }
+ list_add_tail(&vbio->list, &entry->list);
+}
+
+static int handoff_io(struct vhost_blk *blk, int head,
+ uint32_t type, uint64_t sector,
+ struct iovec *iov, int nvecs)
+{
+ struct vhost_virtqueue *vq = &blk->dev.vqs[0];
+ struct vhost_blk_io *vbio;
+
+ vbio = allocate_vbio(nvecs+1);
+ if (!vbio) {
+ return -ENOMEM;
+ }
+ vbio->blk = blk;
+ vbio->head = head;
+ vbio->file = vq->private_data;
+ vbio->type = type;
+ vbio->sector = sector;
+ vbio->nvecs = nvecs;
+ vbio->len = calculate_len(iov, nvecs);
+ memcpy(vbio->iov, iov, (nvecs + 1) * sizeof(struct iovec));
+
+ if (vbio->type & VIRTIO_BLK_T_FLUSH) {
+#if 0
+ /* Sync called - do I need to submit IOs in the queue ? */
+ start_io(&read_queue);
+ start_io(&write_queue);
+#endif
+ queue_work(vblk_workqueue, &vbio->work);
+ } else if (vbio->type & VIRTIO_BLK_T_OUT) {
+ insert_to_queue(vbio, &write_queue);
+ } else {
+ insert_to_queue(vbio, &read_queue);
+ }
+ return 0;
+}
+
+
+static void handle_blk(struct vhost_blk *blk)
+{
+ struct vhost_virtqueue *vq = &blk->dev.vqs[0];
+ unsigned head, out, in;
+ struct virtio_blk_outhdr hdr;
+ int nvecs;
+
+ use_mm(blk->dev.mm);
+ mutex_lock(&vq->mutex);
+
+ vhost_disable_notify(vq);
+
+ for (;;) {
+ head = vhost_get_vq_desc(&blk->dev, vq, vq->iov,
+ ARRAY_SIZE(vq->iov),
+ &out, &in, NULL, NULL);
+ if (head == vq->num) {
+ if (unlikely(vhost_enable_notify(vq))) {
+ vhost_disable_notify(vq);
+ continue;
+ }
+ start_io(&read_queue);
+ start_io(&write_queue);
+ break;
+ }
+
+ BUG_ON(vq->iov[0].iov_len != 16);
+
+ if (copy_from_user(&hdr, vq->iov[0].iov_base, sizeof hdr)) {
+ vhost_discard_vq_desc(vq);
+ continue;
+ }
+
+ nvecs = out - 1;
+ if (hdr.type == VIRTIO_BLK_T_IN)
+ nvecs = in - 1;
+
+ BUG_ON(vq->iov[nvecs+1].iov_len != 1);
+ if (handoff_io(blk, head, hdr.type, hdr.sector, &vq->iov[1], nvecs) < 0) {
+ vhost_discard_vq_desc(vq);
+ continue;
+ }
+ }
+ mutex_unlock(&vq->mutex);
+ unuse_mm(blk->dev.mm);
+}
+
+static void vhost_blk_flush(struct vhost_blk *n)
+{
+ vhost_poll_flush(n->poll);
+ vhost_poll_flush(&n->dev.vqs[0].poll);
+}
+
+static void handle_blk_kick(struct work_struct *work)
+{
+ struct vhost_virtqueue *vq;
+ struct vhost_blk *blk;
+ vq = container_of(work, struct vhost_virtqueue, poll.work);
+ blk = container_of(vq->dev, struct vhost_blk, dev);
+ handle_blk(blk);
+}
+
+static void handle_rq_blk(struct work_struct *work)
+{
+ struct vhost_blk *blk;
+ blk = container_of(work, struct vhost_blk, poll[0].work);
+ handle_blk(blk);
+}
+
+static int vhost_blk_open(struct inode *inode, struct file *f)
+{
+ struct vhost_blk *n = kmalloc(sizeof *n, GFP_KERNEL);
+ int r;
+ if (!n)
+ return -ENOMEM;
+ n->vqs[0].handle_kick = handle_blk_kick;
+ r = vhost_dev_init(&n->dev, n->vqs, VHOST_BLK_VQ_MAX);
+ if (r < 0) {
+ kfree(n);
+ return r;
+ }
+
+ vhost_poll_init(n->poll, handle_rq_blk, POLLOUT|POLLIN);
+ f->private_data = n;
+ return 0;
+}
+
+static int vhost_blk_release(struct inode *inode, struct file *f)
+{
+ struct vhost_blk *n = f->private_data;
+
+ fput(n->vqs->private_data);
+ kfree(n);
+ return 0;
+}
+
+static long vhost_blk_set_backend(struct vhost_blk *n, unsigned index, int fd)
+{
+ struct file *file;
+ struct vhost_virtqueue *vq;
+
+ if (index >= VHOST_BLK_VQ_MAX)
+ return -ENOBUFS;
+
+ file = fget(fd);
+ if (!file)
+ return -EBADF;
+
+ vq = n->vqs + index;
+ mutex_lock(&vq->mutex);
+ rcu_assign_pointer(vq->private_data, file);
+ mutex_unlock(&vq->mutex);
+ return 0;
+}
+
+
+static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
+ unsigned long arg)
+{
+ struct vhost_blk *n = f->private_data;
+ void __user *argp = (void __user *)arg;
+ struct vhost_vring_file backend;
+ int r;
+
+ switch (ioctl) {
+ case VHOST_NET_SET_BACKEND:
+ r = copy_from_user(&backend, argp, sizeof backend);
+ if (r < 0)
+ return r;
+ return vhost_blk_set_backend(n, backend.index, backend.fd);
+ default:
+ mutex_lock(&n->dev.mutex);
+ r = vhost_dev_ioctl(&n->dev, ioctl, arg);
+ vhost_blk_flush(n);
+ mutex_unlock(&n->dev.mutex);
+ return r;
+ }
+}
+
+const static struct file_operations vhost_blk_fops = {
+ .owner = THIS_MODULE,
+ .release = vhost_blk_release,
+ .open = vhost_blk_open,
+ .unlocked_ioctl = vhost_blk_ioctl,
+};
+
+static struct miscdevice vhost_blk_misc = {
+ 234,
+ "vhost-blk",
+ &vhost_blk_fops,
+};
+
+static int vhost_blk_init(void)
+{
+ int r = vhost_init();
+ if (r)
+ goto err_init;
+
+ vblk_workqueue = create_workqueue("vblk");
+ if (!vblk_workqueue) {
+ r = -ENOMEM;
+ goto err_vblk;
+ }
+
+ r = misc_register(&vhost_blk_misc);
+ if (r)
+ goto err_reg;
+ return 0;
+err_reg:
+ destroy_workqueue(vblk_workqueue);
+err_vblk:
+ vhost_cleanup();
+err_init:
+ return r;
+
+}
+module_init(vhost_blk_init);
+
+static void vhost_blk_exit(void)
+{
+ misc_deregister(&vhost_blk_misc);
+ destroy_workqueue(vblk_workqueue);
+ vhost_cleanup();
+}
+module_exit(vhost_blk_exit);
+
+MODULE_VERSION("0.0.2");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Host kernel accelerator for virtio blk");
^ permalink raw reply
* [PATCH v3] Add Mergeable receive buffer support to vhost_net
From: David L Stevens @ 2010-04-06 20:32 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, netdev, rusty, virtualization
This patch adds support for the Mergeable Receive Buffers feature to
vhost_net.
+-DLS
Changes from previous revision:
1) renamed:
vhost_discard_vq_desc -> vhost_discard_desc
vhost_get_heads -> vhost_get_desc_n
vhost_get_vq_desc -> vhost_get_desc
2) added heads as argument to ghost_get_desc_n
3) changed "vq->heads" from iovec to vring_used_elem, removed casts
4) changed vhost_add_used to do multiple elements in a single
copy_to_user,
or two when we wrap the ring.
5) removed rxmaxheadcount and available buffer checks in favor of
running until
an allocation failure, but making sure we break the loop if we get
two in a row, indicating we have at least 1 buffer, but not enough
for the current receive packet
6) restore non-vnet header handling
Signed-Off-By: David L Stevens <dlstevens@us.ibm.com>
diff -ruNp net-next-p0/drivers/vhost/net.c
net-next-v3/drivers/vhost/net.c
--- net-next-p0/drivers/vhost/net.c 2010-03-22 12:04:38.000000000 -0700
+++ net-next-v3/drivers/vhost/net.c 2010-04-06 12:54:56.000000000 -0700
@@ -130,9 +130,8 @@ static void handle_tx(struct vhost_net *
hdr_size = vq->hdr_size;
for (;;) {
- head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
- ARRAY_SIZE(vq->iov),
- &out, &in,
+ 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) {
@@ -167,8 +166,15 @@ static void handle_tx(struct vhost_net *
/* 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);
- tx_poll_start(net, sock);
+ if (err == -EAGAIN) {
+ vhost_discard_desc(vq, 1);
+ tx_poll_start(net, sock);
+ } else {
+ vq_err(vq, "sendmsg: errno %d\n", -err);
+ /* drop packet; do not discard/resend */
+ vhost_add_used_and_signal(&net->dev, vq, head,
+ 0);
+ }
break;
}
if (err != len)
@@ -186,12 +192,25 @@ static void handle_tx(struct vhost_net *
unuse_mm(net->dev.mm);
}
+static int vhost_head_len(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;
+ 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,
@@ -202,13 +221,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
};
+ int retries = 0;
size_t len, total_len = 0;
- int err;
+ int err, headcount, datalen;
size_t hdr_size;
struct socket *sock = rcu_dereference(vq->private_data);
if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
@@ -222,31 +242,25 @@ static void handle_rx(struct vhost_net *
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(sock->sk))) {
+ headcount = vhost_get_desc_n(vq, vq->heads, datalen, &in,
+ vq_log, &log);
/* OK, now we need to know about added descriptors. */
- if (head == vq->num) {
- if (unlikely(vhost_enable_notify(vq))) {
+ if (!headcount) {
+ if (retries == 0 && unlikely(vhost_enable_notify(vq))) {
/* They have slipped one in as we were
* doing that: check again. */
vhost_disable_notify(vq);
+ retries++;
continue;
}
+ retries = 0;
/* Nothing new? Wait for eventfd to tell us
* they refilled. */
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. */
+ /* Skip header. TODO: support TSO. */
s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
msg.msg_iovlen = in;
len = iov_length(vq->iov, in);
@@ -261,14 +275,33 @@ static void handle_rx(struct vhost_net *
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 (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF)) {
+ struct virtio_net_hdr_mrg_rxbuf *vhdr =
+ (struct virtio_net_hdr_mrg_rxbuf *)
+ vq->iov[0].iov_base;
+ /* add num_buffers */
+ if (vhost_has_feature(&net->dev,
+ VHOST_NET_F_VIRTIO_NET_HDR))
+ hdr.num_buffers = headcount;
+ else if (vq->iov[0].iov_len < sizeof(*vhdr)) {
+ vq_err(vq, "tiny buffers < %d unsupported",
+ vq->iov[0].iov_len);
+ vhost_discard_desc(vq, headcount);
+ break;
+ } else if (put_user(headcount, &vhdr->num_buffers)) {
+ vq_err(vq, "Failed num_buffers write");
+ vhost_discard_desc(vq, headcount);
+ break;
+ }
+ }
if (err > len) {
pr_err("Discarded truncated rx packet: "
" len %d > %zd\n", err, len);
- vhost_discard_vq_desc(vq);
+ vhost_discard_desc(vq, headcount);
continue;
}
len = err;
@@ -279,7 +312,7 @@ static void handle_rx(struct vhost_net *
break;
}
len += hdr_size;
- vhost_add_used_and_signal(&net->dev, vq, head, len);
+ 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;
@@ -560,9 +593,14 @@ done:
static int vhost_net_set_features(struct vhost_net *n, u64 features)
{
- size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
- sizeof(struct virtio_net_hdr) : 0;
+ size_t hdr_size = 0;
int i;
+
+ if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)) {
+ hdr_size = sizeof(struct virtio_net_hdr);
+ if (features & (1 << VIRTIO_NET_F_MRG_RXBUF))
+ hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
+ }
mutex_lock(&n->dev.mutex);
if ((features & (1 << VHOST_F_LOG_ALL)) &&
!vhost_log_access_ok(&n->dev)) {
diff -ruNp net-next-p0/drivers/vhost/vhost.c
net-next-v3/drivers/vhost/vhost.c
--- net-next-p0/drivers/vhost/vhost.c 2010-03-22 12:04:38.000000000
-0700
+++ net-next-v3/drivers/vhost/vhost.c 2010-04-06 12:57:51.000000000
-0700
@@ -856,6 +856,47 @@ static unsigned get_indirect(struct vhos
return 0;
}
+/* This is a multi-buffer version of vhost_get_vq_desc
+ * @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, 0 on error
+ */
+int vhost_get_desc_n(struct vhost_virtqueue *vq, struct vring_used_elem
*heads,
+ int datalen, int *iovcount, struct vhost_log *log,
+ unsigned int *log_num)
+{
+ int out, in;
+ int seg = 0; /* iov index */
+ int hc = 0; /* head count */
+
+ while (datalen > 0) {
+ if (hc >= VHOST_NET_MAX_SG)
+ goto err;
+ heads[hc].id = vhost_get_desc(vq->dev, vq, vq->iov+seg,
+ ARRAY_SIZE(vq->iov)-seg, &out,
+ &in, log, log_num);
+ if (heads[hc].id == vq->num)
+ goto err;
+ if (out || in <= 0) {
+ vq_err(vq, "unexpected descriptor format for RX: "
+ "out %d, in %d\n", out, in);
+ goto err;
+ }
+ heads[hc].len = iov_length(vq->iov+seg, in);
+ datalen -= heads[hc].len;
+ hc++;
+ seg += in;
+ }
+ *iovcount = seg;
+ return hc;
+err:
+ vhost_discard_desc(vq, hc);
+ return 0;
+}
+
/* This looks in the virtqueue and for the first available buffer, and
converts
* it to an iovec for convenient access. Since descriptors consist of
some
* number of output then some number of input descriptors, it's
actually two
@@ -863,7 +904,7 @@ static unsigned get_indirect(struct vhos
*
* 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,
+unsigned vhost_get_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)
@@ -981,31 +1022,42 @@ unsigned vhost_get_vq_desc(struct vhost_
}
/* Reverse the effect of vhost_get_vq_desc. Useful for error handling.
*/
-void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
+void vhost_discard_desc(struct vhost_virtqueue *vq, int n)
{
- vq->last_avail_idx--;
+ vq->last_avail_idx -= n;
}
/* After we've used one of their buffers, we tell them about it. We'll
then
* want to notify the guest, using eventfd. */
-int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int
len)
+int vhost_add_used(struct vhost_virtqueue *vq, struct vring_used_elem
*heads,
+ int count)
{
struct vring_used_elem *used;
+ int start, n;
+
+ if (count <= 0)
+ return -EINVAL;
- /* The virtqueue contains a ring of used buffers. Get a pointer to
the
- * next entry in that used ring. */
- used = &vq->used->ring[vq->last_used_idx % vq->num];
- if (put_user(head, &used->id)) {
- vq_err(vq, "Failed to write used id");
+ start = vq->last_used_idx % vq->num;
+ if (vq->num - start < count)
+ n = vq->num - start;
+ else
+ n = count;
+ used = vq->used->ring + start;
+ if (copy_to_user(used, heads, sizeof(heads[0])*n)) {
+ vq_err(vq, "Failed to write used");
return -EFAULT;
}
- if (put_user(len, &used->len)) {
- vq_err(vq, "Failed to write used len");
- return -EFAULT;
+ if (n < count) { /* wrapped the ring */
+ used = vq->used->ring;
+ if (copy_to_user(used, heads+n, sizeof(heads[0])*(count-n))) {
+ vq_err(vq, "Failed to write used");
+ return -EFAULT;
+ }
}
/* Make sure buffer is written before we update index. */
smp_wmb();
- if (put_user(vq->last_used_idx + 1, &vq->used->idx)) {
+ if (put_user(vq->last_used_idx+count, &vq->used->idx)) {
vq_err(vq, "Failed to increment used idx");
return -EFAULT;
}
@@ -1023,7 +1075,7 @@ int vhost_add_used(struct vhost_virtqueu
if (vq->log_ctx)
eventfd_signal(vq->log_ctx, 1);
}
- vq->last_used_idx++;
+ vq->last_used_idx += count;
return 0;
}
@@ -1049,10 +1101,23 @@ void vhost_signal(struct vhost_dev *dev,
/* And here's the combo meal deal. Supersize me! */
void vhost_add_used_and_signal(struct vhost_dev *dev,
- struct vhost_virtqueue *vq,
- unsigned int head, int len)
+ struct vhost_virtqueue *vq, unsigned int id,
+ int len)
+{
+ struct vring_used_elem head;
+
+ head.id = id;
+ head.len = len;
+ vhost_add_used(vq, &head, 1);
+ vhost_signal(dev, vq);
+}
+
+/* multi-buffer version of vhost_add_used_and_signal */
+void vhost_add_used_and_signal_n(struct vhost_dev *dev,
+ struct vhost_virtqueue *vq,
+ struct vring_used_elem *heads, int count)
{
- vhost_add_used(vq, head, len);
+ vhost_add_used(vq, heads, count);
vhost_signal(dev, vq);
}
diff -ruNp net-next-p0/drivers/vhost/vhost.h
net-next-v3/drivers/vhost/vhost.h
--- net-next-p0/drivers/vhost/vhost.h 2010-03-22 12:04:38.000000000
-0700
+++ net-next-v3/drivers/vhost/vhost.h 2010-04-05 20:33:57.000000000
-0700
@@ -85,6 +85,7 @@ struct vhost_virtqueue {
struct iovec iov[VHOST_NET_MAX_SG];
struct iovec hdr[VHOST_NET_MAX_SG];
size_t hdr_size;
+ struct vring_used_elem heads[VHOST_NET_MAX_SG];
/* We use a kind of RCU to access private pointer.
* All readers access it from workqueue, which makes it possible to
* flush the workqueue instead of synchronize_rcu. Therefore readers
do
@@ -120,16 +121,22 @@ long vhost_dev_ioctl(struct vhost_dev *,
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
*,
+int vhost_get_desc_n(struct vhost_virtqueue *, struct vring_used_elem
*heads,
+ int datalen, int *iovcount, struct vhost_log *log,
+ unsigned int *log_num);
+unsigned vhost_get_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 *);
+void vhost_discard_desc(struct vhost_virtqueue *, int);
-int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int
len);
-void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
+int vhost_add_used(struct vhost_virtqueue *, struct vring_used_elem
*heads,
+ int count);
void vhost_add_used_and_signal(struct vhost_dev *, struct
vhost_virtqueue *,
- unsigned int head, int len);
+ unsigned int id, int len);
+void vhost_add_used_and_signal_n(struct vhost_dev *, struct
vhost_virtqueue *,
+ struct vring_used_elem *heads, int count);
+void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
void vhost_disable_notify(struct vhost_virtqueue *);
bool vhost_enable_notify(struct vhost_virtqueue *);
@@ -149,7 +156,8 @@ enum {
VHOST_FEATURES = (1 << VIRTIO_F_NOTIFY_ON_EMPTY) |
(1 << VIRTIO_RING_F_INDIRECT_DESC) |
(1 << VHOST_F_LOG_ALL) |
- (1 << VHOST_NET_F_VIRTIO_NET_HDR),
+ (1 << VHOST_NET_F_VIRTIO_NET_HDR) |
+ (1 << VIRTIO_NET_F_MRG_RXBUF),
};
static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
^ permalink raw reply
* Re: [PATCH 10/10] virtio: console: Add support for nonblocking write()s
From: Amit Shah @ 2010-04-06 14:54 UTC (permalink / raw)
To: Rusty Russell; +Cc: Virtualization List, Michael S. Tsirkin
In-Reply-To: <201004061242.58840.rusty@rustcorp.com.au>
On (Tue) Apr 06 2010 [12:42:58], Rusty Russell wrote:
> On Mon, 5 Apr 2010 11:24:14 pm Amit Shah wrote:
> > If the host port is not open, a write() should either just return if the
> > file is opened in non-blocking mode, or block till the host port is
> > opened.
> >
> > Also, don't spin till host consumes data for nonblocking ports. For
> > non-blocking ports, we can do away with the spinning and reclaim the
> > buffers consumed by the host on the next write call or on the condition
> > that'll make poll return.
>
> I'm only reading the patch so I might have missed it, but what's the
> locking going on here?
>
> Can we race thinking we're full or not full incorrectly?
Turns out just this much on top of this patch should be sufficient.
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 0c632ee..6541f76 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -158,6 +158,9 @@ struct port {
*/
spinlock_t inbuf_lock;
+ /* Protect the operations on the out_vq. */
+ spinlock_t outbuf_lock;
+
/* The IO vqs for this port */
struct virtqueue *in_vq, *out_vq;
@@ -409,12 +412,15 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
static void reclaim_consumed_buffers(struct port *port)
{
void *buf;
+ unsigned long flags;
unsigned int len;
+ spin_lock_irqsave(&port->outbuf_lock, flags);
while ((buf = port->out_vq->vq_ops->get_buf(port->out_vq, &len))) {
kfree(buf);
port->outvq_full = false;
}
+ spin_unlock_irqrestore(&port->outbuf_lock, flags);
}
static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
@@ -423,6 +429,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
struct scatterlist sg[1];
struct virtqueue *out_vq;
ssize_t ret;
+ unsigned long flags;
unsigned int len;
out_vq = port->out_vq;
@@ -430,6 +437,9 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
reclaim_consumed_buffers(port);
sg_init_one(sg, in_buf, in_count);
+
+ spin_lock_irqsave(&port->outbuf_lock, flags);
+
ret = out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, in_buf);
/* Tell Host to go! */
@@ -437,14 +447,14 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
if (ret < 0) {
in_count = 0;
- goto fail;
+ goto done;
}
if (ret == 0)
port->outvq_full = true;
if (nonblock)
- return in_count;
+ goto done;
/*
* Wait till the host acknowledges it pushed out the data we
@@ -454,7 +464,8 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
*/
while (!out_vq->vq_ops->get_buf(out_vq, &len))
cpu_relax();
-fail:
+done:
+ spin_unlock_irqrestore(&port->outbuf_lock, flags);
/*
* We're expected to return the amount of data we wrote -- all
* of it
@@ -1009,6 +1020,7 @@ static int add_port(struct ports_device *portdev, u32 id)
}
spin_lock_init(&port->inbuf_lock);
+ spin_lock_init(&port->outbuf_lock);
init_waitqueue_head(&port->waitqueue);
/* Fill the in_vq with buffers so the host can send us data. */
^ permalink raw reply related
* Re: [PATCH 10/10] virtio: console: Add support for nonblocking write()s
From: Amit Shah @ 2010-04-06 3:54 UTC (permalink / raw)
To: Rusty Russell; +Cc: Virtualization List, Michael S. Tsirkin
In-Reply-To: <201004061242.58840.rusty@rustcorp.com.au>
On (Tue) Apr 06 2010 [12:42:58], Rusty Russell wrote:
> On Mon, 5 Apr 2010 11:24:14 pm Amit Shah wrote:
> > If the host port is not open, a write() should either just return if the
> > file is opened in non-blocking mode, or block till the host port is
> > opened.
> >
> > Also, don't spin till host consumes data for nonblocking ports. For
> > non-blocking ports, we can do away with the spinning and reclaim the
> > buffers consumed by the host on the next write call or on the condition
> > that'll make poll return.
>
> I'm only reading the patch so I might have missed it, but what's the
> locking going on here?
>
> Can we race thinking we're full or not full incorrectly?
Yes, we can. I forgot to mention that in the changelog, I'm working on
adding (and testing) the locking.
Basically I wanted to send out the series in this stage for a review --
only the locking remains to be done as of now.
> But the basic approach of this series seems good!
Thanks!
Amit
^ permalink raw reply
* Re: [PATCH 10/10] virtio: console: Add support for nonblocking write()s
From: Rusty Russell @ 2010-04-06 3:12 UTC (permalink / raw)
To: Amit Shah; +Cc: Michael S. Tsirkin, Virtualization List
In-Reply-To: <1270475654-399-11-git-send-email-amit.shah@redhat.com>
On Mon, 5 Apr 2010 11:24:14 pm Amit Shah wrote:
> If the host port is not open, a write() should either just return if the
> file is opened in non-blocking mode, or block till the host port is
> opened.
>
> Also, don't spin till host consumes data for nonblocking ports. For
> non-blocking ports, we can do away with the spinning and reclaim the
> buffers consumed by the host on the next write call or on the condition
> that'll make poll return.
I'm only reading the patch so I might have missed it, but what's the
locking going on here?
Can we race thinking we're full or not full incorrectly?
But the basic approach of this series seems good!
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH 07/10] virtio: console: Use a control message to add ports
From: Rusty Russell @ 2010-04-06 3:09 UTC (permalink / raw)
To: Amit Shah; +Cc: Michael S. Tsirkin, Virtualization List
In-Reply-To: <1270475654-399-8-git-send-email-amit.shah@redhat.com>
On Mon, 5 Apr 2010 11:24:11 pm Amit Shah wrote:
> @@ -1431,13 +1414,20 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
> err = -ENOMEM;
> goto free_vqs;
> }
> +
> }
Minor nit: gratuitous whitespace addition.
Cheers,
Rusty.
^ permalink raw reply
* Re: A clocksource driver for HyperV
From: Greg KH @ 2010-04-05 22:03 UTC (permalink / raw)
To: Jeremy Fitzhardinge; +Cc: devel, Virtualization, Ky Srinivasan, Haiyang Zhang
In-Reply-To: <4BBA57FB.2000406@goop.org>
On Mon, Apr 05, 2010 at 02:36:59PM -0700, Jeremy Fitzhardinge wrote:
> >+static struct dmi_system_id __initconst
> >+hv_timesource_dmi_table[] __maybe_unused = {
> >+ {
> >+ .ident = "Hyper-V",
> >+ .matches = {
> >+ DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> >+ DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
> >+ DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
> >+ },
> >+ },
> >+ { },
> >+};
> >+MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
>
> So you use the DMI signatures to determine whether the module is
> needed, but cpuid to work out if the feature is present?
The DMI and PCI stuff is to handle module autoloading. Ideally we would
just use the DMI matching, but some distro installers can't handle that,
so we also add the PCI stuff. The hyperv core already has this in it.
> >+
> >+static struct pci_device_id __initconst
> >+hv_timesource_pci_table[] __maybe_unused = {
> >+ { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
> >+ { 0 }
> >+};
> >+MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
>
> And/or PCI?
>
> Seems a bit... ad-hoc? Is this the official way to determine the
> presence of Hyper-V?
Yeah, it is :(
thanks,
greg k-h
^ permalink raw reply
* Re: A clocksource driver for HyperV
From: Jeremy Fitzhardinge @ 2010-04-05 21:36 UTC (permalink / raw)
To: Ky Srinivasan; +Cc: devel, Virtualization, Haiyang Zhang, Greg KH
In-Reply-To: <4BB9F408020000300008287C@sinclair.provo.novell.com>
On 04/05/2010 01:30 PM, Ky Srinivasan wrote:
> +static cycle_t read_hv_clock(struct clocksource *arg)
> +{
> + cycle_t current_tick;
> + /*
> + * Read the partition counter to get the current tick count. This count
> + * is set to 0 when the partition is created and is incremented in
> + * 100 nanosecond units.
> + */
> + rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
> + return current_tick;
> +}
> +
> +static struct clocksource clocksource_hyperv = {
> + .name = "hyperv_clocksource",
>
Seems like a redundantly long name; any use of this string is going to
be in a context where it is obviously a clocksource. How about just
"hyperv"?
> + .rating = 400, /* use this when running on Hyperv*/
> + .read = read_hv_clock,
> + .mask = CLOCKSOURCE_MASK(64),
> + .shift = HV_CLOCK_SHIFT,
> +};
> +
> +static struct dmi_system_id __initconst
> +hv_timesource_dmi_table[] __maybe_unused = {
> + {
> + .ident = "Hyper-V",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
> + DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
> + },
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
>
So you use the DMI signatures to determine whether the module is needed,
but cpuid to work out if the feature is present?
> +
> +static struct pci_device_id __initconst
> +hv_timesource_pci_table[] __maybe_unused = {
> + { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
> + { 0 }
> +};
> +MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
>
And/or PCI?
Seems a bit... ad-hoc? Is this the official way to determine the
presence of Hyper-V?
> +
> +
> +static int __init hv_detect_hyperv(void)
>
This looks generally useful. Should it be hidden away in the
clocksource driver, or in some common hyper-v code? Do other hyper-v
drivers have versions of this?
> +{
> + u32 eax, ebx, ecx, edx;
> + static char hyp_signature[20];
>
20? static?
> +
> + cpuid(1,&eax,&ebx,&ecx,&edx);
> + if (!(ecx& HV_HYPERVISOR_PRESENT_BIT)) {
> + printk(KERN_WARNING
> + "Not on a Hypervisor\n");
>
This just looks like noise, especially since it doesn't identify what is
generating the message. And if you compile this code in as =y
(non-modular) then it will complain every boot.
> + return 1;
> + }
> + cpuid(HV_CPUID_SIGNATURE,&eax,&ebx,&ecx,&edx);
> + *(u32 *)(hyp_signature + 0) = ebx;
> + *(u32 *)(hyp_signature + 4) = ecx;
> + *(u32 *)(hyp_signature + 8) = edx;
> + hyp_signature[12] = 0;
> +
> + if ((eax< HV_CPUID_MIN) || (strcmp("Microsoft Hv", hyp_signature))) {
>
memcmp, surely?
> + printk(KERN_WARNING
> + "Not on HyperV; signature %s, eax %x\n",
> + hyp_signature, eax);
> + return 1;
> + }
> + /*
> + * Extract the features, recommendations etc.
> + */
> + cpuid(HV_CPUID_FEATURES,&eax,&ebx,&ecx,&edx);
> + if (!(eax& 0x10)) {
> + printk(KERN_WARNING "HyperV Time Ref Counter not available!\n");
> + return 1;
> + }
> +
> + cpuid(HV_CPUID_RECOMMENDATIONS,&eax,&ebx,&ecx,&edx);
> + printk(KERN_INFO "HyperV recommendations: %x\n", eax);
> + printk(KERN_INFO "HyperV spin count: %x\n", ebx);
> + return 0;
> +}
> +
> +
> +static int __init init_hv_clocksource(void)
> +{
> + if (hv_detect_hyperv())
> + return -ENODEV;
> + /*
> + * The time ref counter in HyperV is in 100ns units.
> + * The definition of mult is:
> + * mult/2^shift = ns/cyc = 100
> + * mult = (100<< shift)
> + */
> + clocksource_hyperv.mult = (100<< HV_CLOCK_SHIFT);
>
Why not initialize this in the structure? It's just 100<<22 isn't it?
> + printk(KERN_INFO "Registering HyperV clock source\n");
> + return clocksource_register(&clocksource_hyperv);
> +}
> +
> +module_init(init_hv_clocksource);
> +MODULE_DESCRIPTION("HyperV based clocksource");
> +MODULE_AUTHOR("K. Y. Srinivasan<ksrinivasan@novell.com>");
> +MODULE_LICENSE("GPL");
> Index: linux/drivers/staging/hv/Makefile
> ===================================================================
> --- linux.orig/drivers/staging/hv/Makefile 2010-04-05 13:02:06.000000000 -0600
> +++ linux/drivers/staging/hv/Makefile 2010-04-05 13:02:13.000000000 -0600
> @@ -1,4 +1,4 @@
> -obj-$(CONFIG_HYPERV) += hv_vmbus.o
> +obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_timesource.o
> obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o
> obj-$(CONFIG_HYPERV_BLOCK) += hv_blkvsc.o
> obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
>
^ permalink raw reply
* A clocksource driver for HyperV
From: Ky Srinivasan @ 2010-04-05 20:30 UTC (permalink / raw)
To: Greg KH; +Cc: devel, Virtualization, Haiyang Zhang, Ky Srinivasan
[-- Attachment #1: Type: text/plain, Size: 115 bytes --]
I am attaching a clocksource driver for HyperV.
Signed-off-by: K. Y. Srinivasan <ksrinivasan@novell.com>
[-- Attachment #2: hyperv_clocksource.patch --]
[-- Type: text/plain, Size: 5617 bytes --]
From: K. Y. Srinivasan <ksrinivasan@novell.com>
Subject: A clocksource for Linux guests hosted on HyperV.
References: None
Patch-mainline:
This patch is a clocksource implementation suitable for guests hosted on HyperV.
Time keeping in Linux guests hosted on HyperV is unstable. This clocksource
driver fixes the problem.
Signed-off-by: K. Y. Srinivasan <ksrinivasan@novell.com>
Index: linux/drivers/staging/hv/hv_timesource.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux/drivers/staging/hv/hv_timesource.c 2010-04-05 13:33:36.000000000 -0600
@@ -0,0 +1,148 @@
+/*
+ * A clocksource for Linux running on HyperV.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan <ksrinivasan@novell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include <linux/version.h>
+#include <linux/clocksource.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/dmi.h>
+
+#define HV_CLOCK_SHIFT 22
+/*
+ * HyperV defined synthetic CPUID leaves:
+ */
+#define HV_CPUID_SIGNATURE 0x40000000
+#define HV_CPUID_MIN 0x40000005
+#define HV_HYPERVISOR_PRESENT_BIT 0x80000000
+#define HV_CPUID_FEATURES 0x40000003
+#define HV_CPUID_RECOMMENDATIONS 0x40000004
+
+/*
+ * HyperV defined synthetic MSRs
+ */
+
+#define HV_X64_MSR_TIME_REF_COUNT 0x40000020
+
+
+static cycle_t read_hv_clock(struct clocksource *arg)
+{
+ cycle_t current_tick;
+ /*
+ * Read the partition counter to get the current tick count. This count
+ * is set to 0 when the partition is created and is incremented in
+ * 100 nanosecond units.
+ */
+ rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
+ return current_tick;
+}
+
+static struct clocksource clocksource_hyperv = {
+ .name = "hyperv_clocksource",
+ .rating = 400, /* use this when running on Hyperv*/
+ .read = read_hv_clock,
+ .mask = CLOCKSOURCE_MASK(64),
+ .shift = HV_CLOCK_SHIFT,
+};
+
+static struct dmi_system_id __initconst
+hv_timesource_dmi_table[] __maybe_unused = {
+ {
+ .ident = "Hyper-V",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
+ DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
+ },
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
+
+static struct pci_device_id __initconst
+hv_timesource_pci_table[] __maybe_unused = {
+ { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
+ { 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
+
+
+static int __init hv_detect_hyperv(void)
+{
+ u32 eax, ebx, ecx, edx;
+ static char hyp_signature[20];
+
+ cpuid(1, &eax, &ebx, &ecx, &edx);
+ if (!(ecx & HV_HYPERVISOR_PRESENT_BIT)) {
+ printk(KERN_WARNING
+ "Not on a Hypervisor\n");
+ return 1;
+ }
+ cpuid(HV_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
+ *(u32 *)(hyp_signature + 0) = ebx;
+ *(u32 *)(hyp_signature + 4) = ecx;
+ *(u32 *)(hyp_signature + 8) = edx;
+ hyp_signature[12] = 0;
+
+ if ((eax < HV_CPUID_MIN) || (strcmp("Microsoft Hv", hyp_signature))) {
+ printk(KERN_WARNING
+ "Not on HyperV; signature %s, eax %x\n",
+ hyp_signature, eax);
+ return 1;
+ }
+ /*
+ * Extract the features, recommendations etc.
+ */
+ cpuid(HV_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
+ if (!(eax & 0x10)) {
+ printk(KERN_WARNING "HyperV Time Ref Counter not available!\n");
+ return 1;
+ }
+
+ cpuid(HV_CPUID_RECOMMENDATIONS, &eax, &ebx, &ecx, &edx);
+ printk(KERN_INFO "HyperV recommendations: %x\n", eax);
+ printk(KERN_INFO "HyperV spin count: %x\n", ebx);
+ return 0;
+}
+
+
+static int __init init_hv_clocksource(void)
+{
+ if (hv_detect_hyperv())
+ return -ENODEV;
+ /*
+ * The time ref counter in HyperV is in 100ns units.
+ * The definition of mult is:
+ * mult/2^shift = ns/cyc = 100
+ * mult = (100 << shift)
+ */
+ clocksource_hyperv.mult = (100 << HV_CLOCK_SHIFT);
+ printk(KERN_INFO "Registering HyperV clock source\n");
+ return clocksource_register(&clocksource_hyperv);
+}
+
+module_init(init_hv_clocksource);
+MODULE_DESCRIPTION("HyperV based clocksource");
+MODULE_AUTHOR("K. Y. Srinivasan <ksrinivasan@novell.com>");
+MODULE_LICENSE("GPL");
Index: linux/drivers/staging/hv/Makefile
===================================================================
--- linux.orig/drivers/staging/hv/Makefile 2010-04-05 13:02:06.000000000 -0600
+++ linux/drivers/staging/hv/Makefile 2010-04-05 13:02:13.000000000 -0600
@@ -1,4 +1,4 @@
-obj-$(CONFIG_HYPERV) += hv_vmbus.o
+obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_timesource.o
obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o
obj-$(CONFIG_HYPERV_BLOCK) += hv_blkvsc.o
obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
[-- Attachment #3: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* A clocksource driver for HyperV
From: Ky Srinivasan @ 2010-04-05 20:30 UTC (permalink / raw)
To: Greg KH; +Cc: devel, Virtualization, Haiyang Zhang, Ky Srinivasan
[-- Attachment #1: Type: text/plain, Size: 115 bytes --]
I am attaching a clocksource driver for HyperV.
Signed-off-by: K. Y. Srinivasan <ksrinivasan@novell.com>
[-- Attachment #2: hyperv_clocksource.patch --]
[-- Type: text/plain, Size: 5617 bytes --]
From: K. Y. Srinivasan <ksrinivasan@novell.com>
Subject: A clocksource for Linux guests hosted on HyperV.
References: None
Patch-mainline:
This patch is a clocksource implementation suitable for guests hosted on HyperV.
Time keeping in Linux guests hosted on HyperV is unstable. This clocksource
driver fixes the problem.
Signed-off-by: K. Y. Srinivasan <ksrinivasan@novell.com>
Index: linux/drivers/staging/hv/hv_timesource.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux/drivers/staging/hv/hv_timesource.c 2010-04-05 13:33:36.000000000 -0600
@@ -0,0 +1,148 @@
+/*
+ * A clocksource for Linux running on HyperV.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan <ksrinivasan@novell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include <linux/version.h>
+#include <linux/clocksource.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/dmi.h>
+
+#define HV_CLOCK_SHIFT 22
+/*
+ * HyperV defined synthetic CPUID leaves:
+ */
+#define HV_CPUID_SIGNATURE 0x40000000
+#define HV_CPUID_MIN 0x40000005
+#define HV_HYPERVISOR_PRESENT_BIT 0x80000000
+#define HV_CPUID_FEATURES 0x40000003
+#define HV_CPUID_RECOMMENDATIONS 0x40000004
+
+/*
+ * HyperV defined synthetic MSRs
+ */
+
+#define HV_X64_MSR_TIME_REF_COUNT 0x40000020
+
+
+static cycle_t read_hv_clock(struct clocksource *arg)
+{
+ cycle_t current_tick;
+ /*
+ * Read the partition counter to get the current tick count. This count
+ * is set to 0 when the partition is created and is incremented in
+ * 100 nanosecond units.
+ */
+ rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
+ return current_tick;
+}
+
+static struct clocksource clocksource_hyperv = {
+ .name = "hyperv_clocksource",
+ .rating = 400, /* use this when running on Hyperv*/
+ .read = read_hv_clock,
+ .mask = CLOCKSOURCE_MASK(64),
+ .shift = HV_CLOCK_SHIFT,
+};
+
+static struct dmi_system_id __initconst
+hv_timesource_dmi_table[] __maybe_unused = {
+ {
+ .ident = "Hyper-V",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
+ DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
+ },
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
+
+static struct pci_device_id __initconst
+hv_timesource_pci_table[] __maybe_unused = {
+ { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
+ { 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
+
+
+static int __init hv_detect_hyperv(void)
+{
+ u32 eax, ebx, ecx, edx;
+ static char hyp_signature[20];
+
+ cpuid(1, &eax, &ebx, &ecx, &edx);
+ if (!(ecx & HV_HYPERVISOR_PRESENT_BIT)) {
+ printk(KERN_WARNING
+ "Not on a Hypervisor\n");
+ return 1;
+ }
+ cpuid(HV_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
+ *(u32 *)(hyp_signature + 0) = ebx;
+ *(u32 *)(hyp_signature + 4) = ecx;
+ *(u32 *)(hyp_signature + 8) = edx;
+ hyp_signature[12] = 0;
+
+ if ((eax < HV_CPUID_MIN) || (strcmp("Microsoft Hv", hyp_signature))) {
+ printk(KERN_WARNING
+ "Not on HyperV; signature %s, eax %x\n",
+ hyp_signature, eax);
+ return 1;
+ }
+ /*
+ * Extract the features, recommendations etc.
+ */
+ cpuid(HV_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
+ if (!(eax & 0x10)) {
+ printk(KERN_WARNING "HyperV Time Ref Counter not available!\n");
+ return 1;
+ }
+
+ cpuid(HV_CPUID_RECOMMENDATIONS, &eax, &ebx, &ecx, &edx);
+ printk(KERN_INFO "HyperV recommendations: %x\n", eax);
+ printk(KERN_INFO "HyperV spin count: %x\n", ebx);
+ return 0;
+}
+
+
+static int __init init_hv_clocksource(void)
+{
+ if (hv_detect_hyperv())
+ return -ENODEV;
+ /*
+ * The time ref counter in HyperV is in 100ns units.
+ * The definition of mult is:
+ * mult/2^shift = ns/cyc = 100
+ * mult = (100 << shift)
+ */
+ clocksource_hyperv.mult = (100 << HV_CLOCK_SHIFT);
+ printk(KERN_INFO "Registering HyperV clock source\n");
+ return clocksource_register(&clocksource_hyperv);
+}
+
+module_init(init_hv_clocksource);
+MODULE_DESCRIPTION("HyperV based clocksource");
+MODULE_AUTHOR("K. Y. Srinivasan <ksrinivasan@novell.com>");
+MODULE_LICENSE("GPL");
Index: linux/drivers/staging/hv/Makefile
===================================================================
--- linux.orig/drivers/staging/hv/Makefile 2010-04-05 13:02:06.000000000 -0600
+++ linux/drivers/staging/hv/Makefile 2010-04-05 13:02:13.000000000 -0600
@@ -1,4 +1,4 @@
-obj-$(CONFIG_HYPERV) += hv_vmbus.o
+obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_timesource.o
obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o
obj-$(CONFIG_HYPERV_BLOCK) += hv_blkvsc.o
obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
[-- Attachment #3: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH 10/10] virtio: console: Add support for nonblocking write()s
From: Amit Shah @ 2010-04-05 13:54 UTC (permalink / raw)
To: Virtualization List; +Cc: Amit Shah, Michael S. Tsirkin
In-Reply-To: <1270475654-399-10-git-send-email-amit.shah@redhat.com>
If the host port is not open, a write() should either just return if the
file is opened in non-blocking mode, or block till the host port is
opened.
Also, don't spin till host consumes data for nonblocking ports. For
non-blocking ports, we can do away with the spinning and reclaim the
buffers consumed by the host on the next write call or on the condition
that'll make poll return.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 82 +++++++++++++++++++++++++++++++++++++++--
1 files changed, 78 insertions(+), 4 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 680ea25..d70efda 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -183,6 +183,8 @@ struct port {
/* The 'id' to identify the port with the Host */
u32 id;
+ bool outvq_full;
+
/* Is the host device open */
bool host_connected;
@@ -404,7 +406,19 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
return __send_control_msg(port->portdev, port->id, event, value);
}
-static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
+static void reclaim_consumed_buffers(struct port *port)
+{
+ void *buf;
+ unsigned int len;
+
+ while ((buf = port->out_vq->vq_ops->get_buf(port->out_vq, &len))) {
+ kfree(buf);
+ port->outvq_full = false;
+ }
+}
+
+static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
+ bool nonblock)
{
struct scatterlist sg[1];
struct virtqueue *out_vq;
@@ -413,6 +427,8 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
out_vq = port->out_vq;
+ reclaim_consumed_buffers(port);
+
sg_init_one(sg, in_buf, in_count);
ret = out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, in_buf);
@@ -424,6 +440,12 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
goto fail;
}
+ if (ret == 0)
+ port->outvq_full = true;
+
+ if (nonblock)
+ return in_count;
+
/*
* Wait till the host acknowledges it pushed out the data we
* sent. This is done for ports in blocking mode or for data
@@ -491,6 +513,20 @@ static bool will_read_block(struct port *port)
return !port_has_data(port) && port->host_connected;
}
+static bool will_write_block(struct port *port)
+{
+ if (!port->host_connected)
+ return true;
+
+ /*
+ * Check if the Host has consumed any buffers since we last
+ * sent data (this is only applicable for nonblocking ports).
+ */
+ reclaim_consumed_buffers(port);
+
+ return port->outvq_full;
+}
+
static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
size_t count, loff_t *offp)
{
@@ -537,9 +573,22 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
struct port *port;
char *buf;
ssize_t ret;
+ bool nonblock;
port = filp->private_data;
+ nonblock = filp->f_flags & O_NONBLOCK;
+
+ if (will_write_block(port)) {
+ if (nonblock)
+ return -EAGAIN;
+
+ ret = wait_event_interruptible(port->waitqueue,
+ !will_write_block(port));
+ if (ret < 0)
+ return ret;
+ }
+
count = min((size_t)(32 * 1024), count);
buf = kmalloc(count, GFP_KERNEL);
@@ -552,9 +601,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
goto free_buf;
}
- ret = send_buf(port, buf, count);
+ ret = send_buf(port, buf, count, nonblock);
+
+ if (nonblock && ret > 0)
+ goto out;
+
free_buf:
kfree(buf);
+out:
return ret;
}
@@ -569,7 +623,7 @@ static unsigned int port_fops_poll(struct file *filp, poll_table *wait)
ret = 0;
if (port->inbuf)
ret |= POLLIN | POLLRDNORM;
- if (port->host_connected)
+ if (!will_write_block(port))
ret |= POLLOUT;
if (!port->host_connected)
ret |= POLLHUP;
@@ -592,6 +646,7 @@ static int port_fops_release(struct inode *inode, struct file *filp)
discard_port_data(port);
spin_unlock_irq(&port->inbuf_lock);
+ reclaim_consumed_buffers(port);
return 0;
}
@@ -621,6 +676,13 @@ static int port_fops_open(struct inode *inode, struct file *filp)
port->guest_connected = true;
spin_unlock_irq(&port->inbuf_lock);
+ /*
+ * There might be a chance that we missed reclaiming a few
+ * buffers in the window of the port getting previously closed
+ * and opening now.
+ */
+ reclaim_consumed_buffers(port);
+
/* Notify host of port being opened */
send_control_msg(filp->private_data, VIRTIO_CONSOLE_PORT_OPEN, 1);
@@ -661,7 +723,7 @@ static int put_chars(u32 vtermno, const char *buf, int count)
if (!port)
return -EPIPE;
- return send_buf(port, (void *)buf, count);
+ return send_buf(port, (void *)buf, count, false);
}
/*
@@ -853,6 +915,8 @@ static ssize_t debugfs_read(struct file *filp, char __user *ubuf,
out_offset += snprintf(buf + out_offset, out_count - out_offset,
"host_connected: %d\n", port->host_connected);
out_offset += snprintf(buf + out_offset, out_count - out_offset,
+ "outvq_full: %d\n", port->outvq_full);
+ out_offset += snprintf(buf + out_offset, out_count - out_offset,
"is_console: %s\n",
is_console_port(port) ? "yes" : "no");
out_offset += snprintf(buf + out_offset, out_count - out_offset,
@@ -919,6 +983,8 @@ static int add_port(struct ports_device *portdev, u32 id)
port->host_connected = port->guest_connected = false;
+ port->outvq_full = false;
+
port->in_vq = portdev->in_vqs[port->id];
port->out_vq = portdev->out_vqs[port->id];
@@ -1036,6 +1102,8 @@ static int remove_port(struct port *port)
/* Remove unused data this port might have received. */
discard_port_data(port);
+ reclaim_consumed_buffers(port);
+
/* Remove buffers we queued up for the Host to send us data in. */
while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
free_buf(buf);
@@ -1107,6 +1175,12 @@ static void handle_control_message(struct ports_device *portdev,
case VIRTIO_CONSOLE_PORT_OPEN:
port->host_connected = cpkt->value;
wake_up_interruptible(&port->waitqueue);
+ /*
+ * If the host port got closed and the host had any
+ * unconsumed buffers, we'll be able to reclaim them
+ * now.
+ */
+ reclaim_unconsumed_buffers(port);
break;
case VIRTIO_CONSOLE_PORT_NAME:
/*
--
1.6.2.5
^ permalink raw reply related
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