* [PATCH 2/9] virtio-serial-bus: save/load: Ensure nr_ports on src and dest are same.
From: Amit Shah @ 2010-03-19 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, quintela, mst, virtualization
In-Reply-To: <1268999926-29560-2-git-send-email-amit.shah@redhat.com>
The number of ports on the source as well as the destination machines
should match. If they don't, it means some ports that got hotplugged on
the source aren't instantiated on the destination. Or that ports that
were hot-unplugged on the source are created on the destination.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reported-by: Juan Quintela <quintela@redhat.com>
---
hw/virtio-serial-bus.c | 18 ++++++++++++++++--
1 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 36985a1..f43d1fc 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -401,7 +401,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
VirtIOSerial *s = opaque;
VirtIOSerialPort *port;
- uint32_t max_nr_ports, nr_active_ports;
+ uint32_t max_nr_ports, nr_active_ports, nr_ports;
unsigned int i;
if (version_id > 2) {
@@ -418,7 +418,21 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
/* The config space */
qemu_get_be16s(f, &s->config.cols);
qemu_get_be16s(f, &s->config.rows);
- s->config.nr_ports = qemu_get_be32(f);
+ nr_ports = qemu_get_be32(f);
+
+ if (nr_ports != s->config.nr_ports) {
+ /*
+ * Source hot-plugged/unplugged ports and we don't have all of
+ * them here.
+ *
+ * Note: This condition cannot check for all hotplug/unplug
+ * events: eg, if one port was hot-plugged and one was
+ * unplugged, the nr_ports remains the same but the port id's
+ * would have changed and we won't catch it here. A later
+ * check for !find_port_by_id() will confirm if this happened.
+ */
+ return -EINVAL;
+ }
/* Items in struct VirtIOSerial */
--
1.6.2.5
^ permalink raw reply related
* [PATCH 1/9] virtio-serial-bus: save/load: Ensure target has enough ports
From: Amit Shah @ 2010-03-19 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, quintela, mst, virtualization
In-Reply-To: <1268999926-29560-1-git-send-email-amit.shah@redhat.com>
The target could be started with max_nr_ports for a virtio-serial device
lesser than what was available on the source machine. Fail the migration
in such a case.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reported-by: Juan Quintela <quintela@redhat.com>
---
hw/virtio-serial-bus.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 17c1ec1..36985a1 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -374,6 +374,8 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
/* Items in struct VirtIOSerial */
+ qemu_put_be32s(f, &s->bus->max_nr_ports);
+
/* Do this because we might have hot-unplugged some ports */
nr_active_ports = 0;
QTAILQ_FOREACH(port, &s->ports, next)
@@ -399,7 +401,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
VirtIOSerial *s = opaque;
VirtIOSerialPort *port;
- uint32_t nr_active_ports;
+ uint32_t max_nr_ports, nr_active_ports;
unsigned int i;
if (version_id > 2) {
@@ -420,6 +422,12 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
/* Items in struct VirtIOSerial */
+ qemu_get_be32s(f, &max_nr_ports);
+ if (max_nr_ports > s->bus->max_nr_ports) {
+ /* Source could have more ports than us. Fail migration. */
+ return -EINVAL;
+ }
+
qemu_get_be32s(f, &nr_active_ports);
/* Items in struct VirtIOSerialPort */
--
1.6.2.5
^ permalink raw reply related
* [PATCH 0/9] virtio-serial fixes, ABI updates
From: Amit Shah @ 2010-03-19 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, quintela, mst, virtualization
Hello,
This series fixes a few issues pointed out by Avi and Juan. Avi
pointed out we should do full scatter/gather processing of guest data
even if current (well-behaved) guests don't send multiple iovs per
element.
Juan pointed out a few migration-related bugs.
In handling the migration fixes, I noticed hot-plug/unplug isn't
handled perfectly for the migration case: ports are enumerated and the
port numbering has to be consistent with the guest's numbering. If
there's a mismatch, control messages meant for one port could be
interpreted for another.
To solve this issue, I go back to maintaining a bitmap in the config
space for active ports. Hot-plug and unplug can be added easily via
the config space as a result.
The kernel driver has to be changed as well so that the changes are in
sync with the changes here.
I've tested these patches on my test suite that tests for correctness
and also hot-plug/unplug cases and fixes presented here.
Amit Shah (9):
virtio-serial-bus: save/load: Ensure target has enough ports
virtio-serial-bus: save/load: Ensure nr_ports on src and dest are
same.
virtio-serial: save/load: Ensure we have hot-plugged ports
instantiated
virtio-serial: Handle scatter-gather buffers for control messages
virtio-serial: Handle scatter/gather input from the guest
virtio-serial: Remove redundant check for 0-sized write request
virtio-serial: Update copyright year to 2010
virtio-serial-bus: Use a bitmap in virtio config space for active
ports
virtio-serial-bus: Let the guest know of host connection changes
after migration
hw/virtio-console.c | 4 +-
hw/virtio-serial-bus.c | 205 ++++++++++++++++++++++++++++++++++++------------
hw/virtio-serial.h | 8 +-
3 files changed, 161 insertions(+), 56 deletions(-)
^ permalink raw reply
* [GIT PULL] vhost-net fixes for issues in 2.6.34-rc1
From: Michael S. Tsirkin @ 2010-03-18 9:53 UTC (permalink / raw)
To: David Miller
Cc: samudrala, kvm, Juan Quintela, netdev, linux-kernel,
Laurent Chavey, virtualization, Unai Uribarri, Jeff Dike,
Jiri Slaby
David,
The following tree includes patches fixing issues with vhost-net in
2.6.34-rc1. Please pull them for 2.6.34.
Thanks!
The following changes since commit a3d3203e4bb40f253b1541e310dc0f9305be7c84:
Merge branch 'release' of git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux-acpi-2.6 (2010-03-14 20:29:21 -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: fix error path in vhost_net_set_backend
Michael S. Tsirkin (2):
vhost: fix interrupt mitigation with raw sockets
vhost: fix error handling in vring ioctls
drivers/vhost/net.c | 10 ++++++----
drivers/vhost/vhost.c | 18 ++++++++++++------
2 files changed, 18 insertions(+), 10 deletions(-)
^ permalink raw reply
* Re: [PATCH] vhost: fix error handling in vring ioctls
From: Laurent Chavey @ 2010-03-17 18:40 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jiri Slaby, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <97949e3e1003171054q3bee27d5yce0d92ae33e5b5ca@mail.gmail.com>
Acked-by: Laurent Chavey <chavey@google.com>
On Wed, Mar 17, 2010 at 10:54 AM, Laurent Chavey <chavey@google.com> wrote:
> Acked-by: chavey@google.com
>
>
> On Wed, Mar 17, 2010 at 7:42 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> Stanse found a locking problem in vhost_set_vring:
>> several returns from VHOST_SET_VRING_KICK, VHOST_SET_VRING_CALL,
>> VHOST_SET_VRING_ERR with the vq->mutex held.
>> Fix these up.
>>
>> Reported-by: Jiri Slaby <jirislaby@gmail.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> drivers/vhost/vhost.c | 18 ++++++++++++------
>> 1 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index 7cd55e0..7bd7a1e 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -476,8 +476,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
>> if (r < 0)
>> break;
>> eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
>> - if (IS_ERR(eventfp))
>> - return PTR_ERR(eventfp);
>> + if (IS_ERR(eventfp)) {
>> + r = PTR_ERR(eventfp);
>> + break;
>> + }
>> if (eventfp != vq->kick) {
>> pollstop = filep = vq->kick;
>> pollstart = vq->kick = eventfp;
>> @@ -489,8 +491,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
>> if (r < 0)
>> break;
>> eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
>> - if (IS_ERR(eventfp))
>> - return PTR_ERR(eventfp);
>> + if (IS_ERR(eventfp)) {
>> + r = PTR_ERR(eventfp);
>> + break;
>> + }
>> if (eventfp != vq->call) {
>> filep = vq->call;
>> ctx = vq->call_ctx;
>> @@ -505,8 +509,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
>> if (r < 0)
>> break;
>> eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
>> - if (IS_ERR(eventfp))
>> - return PTR_ERR(eventfp);
>> + if (IS_ERR(eventfp)) {
>> + r = PTR_ERR(eventfp);
>> + break;
>> + }
>> if (eventfp != vq->error) {
>> filep = vq->error;
>> vq->error = eventfp;
>> --
>> 1.7.0.18.g0d53a5
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
^ permalink raw reply
* Re: [PATCH] vhost: fix error handling in vring ioctls
From: Laurent Chavey @ 2010-03-17 17:54 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jiri Slaby, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20100317144249.GA3984@redhat.com>
Acked-by: chavey@google.com
On Wed, Mar 17, 2010 at 7:42 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> Stanse found a locking problem in vhost_set_vring:
> several returns from VHOST_SET_VRING_KICK, VHOST_SET_VRING_CALL,
> VHOST_SET_VRING_ERR with the vq->mutex held.
> Fix these up.
>
> Reported-by: Jiri Slaby <jirislaby@gmail.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/vhost.c | 18 ++++++++++++------
> 1 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 7cd55e0..7bd7a1e 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -476,8 +476,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
> if (r < 0)
> break;
> eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
> - if (IS_ERR(eventfp))
> - return PTR_ERR(eventfp);
> + if (IS_ERR(eventfp)) {
> + r = PTR_ERR(eventfp);
> + break;
> + }
> if (eventfp != vq->kick) {
> pollstop = filep = vq->kick;
> pollstart = vq->kick = eventfp;
> @@ -489,8 +491,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
> if (r < 0)
> break;
> eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
> - if (IS_ERR(eventfp))
> - return PTR_ERR(eventfp);
> + if (IS_ERR(eventfp)) {
> + r = PTR_ERR(eventfp);
> + break;
> + }
> if (eventfp != vq->call) {
> filep = vq->call;
> ctx = vq->call_ctx;
> @@ -505,8 +509,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
> if (r < 0)
> break;
> eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
> - if (IS_ERR(eventfp))
> - return PTR_ERR(eventfp);
> + if (IS_ERR(eventfp)) {
> + r = PTR_ERR(eventfp);
> + break;
> + }
> if (eventfp != vq->error) {
> filep = vq->error;
> vq->error = eventfp;
> --
> 1.7.0.18.g0d53a5
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [PATCH] vhost: fix interrupt mitigation with raw sockets
From: Michael S. Tsirkin @ 2010-03-17 14:43 UTC (permalink / raw)
To: Michael S. Tsirkin, Juan Quintela, Unai Uribarri, kvm,
virtualization
A thinko in code means we never trigger interrupt
mitigation. Fix this.
Reported-by: Juan Quintela <quintela@redhat.com>
Reported-by: Unai Uribarri <unai.uribarri@optenet.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/net.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index fcafb6b..a6a88df 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -125,7 +125,7 @@ static void handle_tx(struct vhost_net *net)
mutex_lock(&vq->mutex);
vhost_disable_notify(vq);
- if (wmem < sock->sk->sk_sndbuf * 2)
+ if (wmem < sock->sk->sk_sndbuf / 2)
tx_poll_stop(net);
hdr_size = vq->hdr_size;
--
1.7.0.18.g0d53a5
^ permalink raw reply related
* [PATCH] vhost: fix error handling in vring ioctls
From: Michael S. Tsirkin @ 2010-03-17 14:42 UTC (permalink / raw)
To: Michael S. Tsirkin, Jiri Slaby, kvm, virtualization, netdev,
linux-kernel
Stanse found a locking problem in vhost_set_vring:
several returns from VHOST_SET_VRING_KICK, VHOST_SET_VRING_CALL,
VHOST_SET_VRING_ERR with the vq->mutex held.
Fix these up.
Reported-by: Jiri Slaby <jirislaby@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/vhost.c | 18 ++++++++++++------
1 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 7cd55e0..7bd7a1e 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -476,8 +476,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
if (r < 0)
break;
eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
- if (IS_ERR(eventfp))
- return PTR_ERR(eventfp);
+ if (IS_ERR(eventfp)) {
+ r = PTR_ERR(eventfp);
+ break;
+ }
if (eventfp != vq->kick) {
pollstop = filep = vq->kick;
pollstart = vq->kick = eventfp;
@@ -489,8 +491,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
if (r < 0)
break;
eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
- if (IS_ERR(eventfp))
- return PTR_ERR(eventfp);
+ if (IS_ERR(eventfp)) {
+ r = PTR_ERR(eventfp);
+ break;
+ }
if (eventfp != vq->call) {
filep = vq->call;
ctx = vq->call_ctx;
@@ -505,8 +509,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
if (r < 0)
break;
eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
- if (IS_ERR(eventfp))
- return PTR_ERR(eventfp);
+ if (IS_ERR(eventfp)) {
+ r = PTR_ERR(eventfp);
+ break;
+ }
if (eventfp != vq->error) {
filep = vq->error;
vq->error = eventfp;
--
1.7.0.18.g0d53a5
^ permalink raw reply related
* [PATCH 2/2] xen: events: Fix checkpatch issues
From: Bruno Bigras @ 2010-03-11 4:00 UTC (permalink / raw)
To: jeremy; +Cc: virtualization, xen-devel, linux-kernel, Bruno Bigras
In-Reply-To: <1268280054-16170-1-git-send-email-bigras.bruno@gmail.com>
drivers/xen/events.c:31: WARNING: Use #include <linux/ptrace.h> instead of <asm/ptrace.h>
drivers/xen/events.c:76: ERROR: open brace '{' following struct go on the same line
drivers/xen/events.c:509: WARNING: line over 80 characters
drivers/xen/events.c:581: ERROR: space required before the open parenthesis '('
drivers/xen/events.c:585: ERROR: space required before the open parenthesis '('
drivers/xen/events.c:590: ERROR: space required before the open parenthesis '('
drivers/xen/events.c:595: ERROR: space required before the open parenthesis '('
drivers/xen/events.c:625: ERROR: code indent should use tabs where possible
drivers/xen/events.c:625: WARNING: please, no space before tabs
drivers/xen/events.c:666: ERROR: space required before the open parenthesis '('
drivers/xen/events.c:802: ERROR: do not use assignment in if condition
drivers/xen/events.c:831: ERROR: do not use assignment in if condition
Signed-off-by: Bruno Bigras <bigras.bruno@gmail.com>
---
drivers/xen/events.c | 26 ++++++++++++++------------
1 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 2f84137..e86841c 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -27,8 +27,8 @@
#include <linux/module.h>
#include <linux/string.h>
#include <linux/bootmem.h>
+#include <linux/ptrace.h>
-#include <asm/ptrace.h>
#include <asm/irq.h>
#include <asm/idle.h>
#include <asm/sync_bitops.h>
@@ -72,8 +72,7 @@ enum xen_irq_type {
* IPI - IPI vector
* EVTCHN -
*/
-struct irq_info
-{
+struct irq_info {
enum xen_irq_type type; /* type */
unsigned short evtchn; /* event channel */
unsigned short cpu; /* cpu bound */
@@ -506,7 +505,8 @@ EXPORT_SYMBOL_GPL(bind_evtchn_to_irqhandler);
int bind_virq_to_irqhandler(unsigned int virq, unsigned int cpu,
irq_handler_t handler,
- unsigned long irqflags, const char *devname, void *dev_id)
+ unsigned long irqflags, const char *devname,
+ void *dev_id)
{
unsigned int irq;
int retval;
@@ -578,21 +578,21 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
v->evtchn_pending_sel);
}
printk("pending:\n ");
- for(i = ARRAY_SIZE(sh->evtchn_pending)-1; i >= 0; i--)
+ for (i = ARRAY_SIZE(sh->evtchn_pending)-1; i >= 0; i--)
printk("%08lx%s", sh->evtchn_pending[i],
i % 8 == 0 ? "\n " : " ");
printk("\nmasks:\n ");
- for(i = ARRAY_SIZE(sh->evtchn_mask)-1; i >= 0; i--)
+ for (i = ARRAY_SIZE(sh->evtchn_mask)-1; i >= 0; i--)
printk("%08lx%s", sh->evtchn_mask[i],
i % 8 == 0 ? "\n " : " ");
printk("\nunmasked:\n ");
- for(i = ARRAY_SIZE(sh->evtchn_mask)-1; i >= 0; i--)
+ for (i = ARRAY_SIZE(sh->evtchn_mask)-1; i >= 0; i--)
printk("%08lx%s", sh->evtchn_pending[i] & ~sh->evtchn_mask[i],
i % 8 == 0 ? "\n " : " ");
printk("\npending list:\n");
- for(i = 0; i < NR_EVENT_CHANNELS; i++) {
+ for (i = 0; i < NR_EVENT_CHANNELS; i++) {
if (sync_test_bit(i, sh->evtchn_pending)) {
printk(" %d: event %d -> irq %d\n",
cpu_from_evtchn(i), i,
@@ -622,7 +622,7 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
struct pt_regs *old_regs = set_irq_regs(regs);
struct shared_info *s = HYPERVISOR_shared_info;
struct vcpu_info *vcpu_info = __get_cpu_var(xen_vcpu);
- unsigned count;
+ unsigned count;
exit_idle();
irq_enter();
@@ -663,7 +663,7 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
count = __get_cpu_var(xed_nesting_count);
__get_cpu_var(xed_nesting_count) = 0;
- } while(count != 1);
+ } while (count != 1);
out:
irq_exit();
@@ -799,7 +799,8 @@ static void restore_cpu_virqs(unsigned int cpu)
int virq, irq, evtchn;
for (virq = 0; virq < NR_VIRQS; virq++) {
- if ((irq = per_cpu(virq_to_irq, cpu)[virq]) == -1)
+ irq = per_cpu(virq_to_irq, cpu)[virq];
+ if (irq == -1)
continue;
BUG_ON(virq_from_irq(irq) != virq);
@@ -828,7 +829,8 @@ static void restore_cpu_ipis(unsigned int cpu)
int ipi, irq, evtchn;
for (ipi = 0; ipi < XEN_NR_IPIS; ipi++) {
- if ((irq = per_cpu(ipi_to_irq, cpu)[ipi]) == -1)
+ irq = per_cpu(ipi_to_irq, cpu)[ipi];
+ if (irq == -1)
continue;
BUG_ON(ipi_from_irq(irq) != ipi);
--
1.7.0.2
^ permalink raw reply related
* [PATCH 1/2] xen: balloon: Fix checkpatch issues
From: Bruno Bigras @ 2010-03-11 4:00 UTC (permalink / raw)
To: jeremy; +Cc: virtualization, xen-devel, linux-kernel, Bruno Bigras
drivers/xen/balloon.c:50: WARNING: Use #include <linux/uaccess.h> instead of <asm/uaccess.h>
drivers/xen/balloon.c:158: ERROR: else should follow close brace '}'
drivers/xen/balloon.c:277: ERROR: do not use assignment in if condition
drivers/xen/balloon.c:293: ERROR: code indent should use tabs where possible
drivers/xen/balloon.c:364: ERROR: that open brace { should be on the previous line
drivers/xen/balloon.c:463: WARNING: line over 80 characters
drivers/xen/balloon.c:491: WARNING: line over 80 characters
Signed-off-by: Bruno Bigras <bigras.bruno@gmail.com>
---
drivers/xen/balloon.c | 21 ++++++++++-----------
1 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index f6738d8..93538a8 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -43,11 +43,11 @@
#include <linux/mutex.h>
#include <linux/list.h>
#include <linux/sysdev.h>
+#include <linux/uaccess.h>
#include <asm/page.h>
#include <asm/pgalloc.h>
#include <asm/pgtable.h>
-#include <asm/uaccess.h>
#include <asm/tlb.h>
#include <asm/xen/hypervisor.h>
@@ -154,8 +154,7 @@ static struct page *balloon_retrieve(void)
if (PageHighMem(page)) {
balloon_stats.balloon_high--;
inc_totalhigh_pages();
- }
- else
+ } else
balloon_stats.balloon_low--;
totalram_pages++;
@@ -274,7 +273,8 @@ static int decrease_reservation(unsigned long nr_pages)
nr_pages = ARRAY_SIZE(frame_list);
for (i = 0; i < nr_pages; i++) {
- if ((page = alloc_page(GFP_BALLOON)) == NULL) {
+ page = alloc_page(GFP_BALLOON);
+ if (!page) {
nr_pages = i;
need_sleep = 1;
break;
@@ -290,7 +290,7 @@ static int decrease_reservation(unsigned long nr_pages)
(unsigned long)__va(pfn << PAGE_SHIFT),
__pte_ma(0), 0);
BUG_ON(ret);
- }
+ }
}
@@ -360,8 +360,7 @@ static void balloon_set_new_target(unsigned long target)
schedule_work(&balloon_worker);
}
-static struct xenbus_watch target_watch =
-{
+static struct xenbus_watch target_watch = {
.node = "memory/target"
};
@@ -460,8 +459,8 @@ BALLOON_SHOW(low_kb, "%lu\n", PAGES2KB(balloon_stats.balloon_low));
BALLOON_SHOW(high_kb, "%lu\n", PAGES2KB(balloon_stats.balloon_high));
BALLOON_SHOW(driver_kb, "%lu\n", PAGES2KB(balloon_stats.driver_pages));
-static ssize_t show_target_kb(struct sys_device *dev, struct sysdev_attribute *attr,
- char *buf)
+static ssize_t show_target_kb(struct sys_device *dev,
+ struct sysdev_attribute *attr, char *buf)
{
return sprintf(buf, "%lu\n", PAGES2KB(balloon_stats.target_pages));
}
@@ -488,8 +487,8 @@ static SYSDEV_ATTR(target_kb, S_IRUGO | S_IWUSR,
show_target_kb, store_target_kb);
-static ssize_t show_target(struct sys_device *dev, struct sysdev_attribute *attr,
- char *buf)
+static ssize_t show_target(struct sys_device *dev,
+ struct sysdev_attribute *attr, char *buf)
{
return sprintf(buf, "%llu\n",
(unsigned long long)balloon_stats.target_pages
--
1.7.0.2
^ permalink raw reply related
* Re: [RFC][ PATCH 1/3] vhost-net: support multiple buffer heads in receiver
From: David Stevens @ 2010-03-10 22:17 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <20100308074533.GA7482@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote on 03/07/2010 11:45:33 PM:
> > > > +static int skb_head_len(struct sk_buff_head *skq)
> > > > +{
> > > > + struct sk_buff *head;
> > > > +
> > > > + head = skb_peek(skq);
> > > > + if (head)
> > > > + return head->len;
> > > > + return 0;
> > > > +}
> > > > +
> > >
> > > This is done without locking, which I think can crash
> > > if skb is consumed after we peek it but before we read the
> > > length.
> >
> > This thread is the only legitimate consumer, right? But
> > qemu has the file descriptor and I guess we shouldn't trust
> > that it won't give it to someone else; it'd break vhost, but
> > a crash would be inappropriate, of course. I'd like to avoid
> > the lock, but I have another idea here, so will investigate.
I looked at this some more and actually, I'm not sure I
see a crash here.
First, without qemu, or something it calls, being broken
as root, nothing else should ever read from the socket, in which
case the length will be exactly right for the next packet we
read. No problem.
But if by some error this skb is freed, we'll have valid
memory address that isn't the length field of the next packet
we'll read.
If the length is negative or more than available in the
vring, we'll fail the buffer allocation, exit the loop, and
get the new head length of the receive queue the next time
around -- no problem.
If the length field is 0, we'll exit the loop even
though we have data to read, but will get that packet the
next time we get in here, again, with the right length.
No problem.
If the length field is big enough to allocate buffer
space for it, but smaller than the new head we have to read,
the recvmsg will fail with EMSGSIZE, drop the packet, exit
the loop and be back in business with the next packet. No
problem.
Otherwise, the packet will fit and be delivered.
I don't much like the notion of using skb->head when
it's garbage, but that can only happen if qemu has broken,
and I don't see a crash unless the skb is not only freed
but no longer a valid memory address for reading at all,
and all within the race window.
Since the code handles other failure cases (not
enough ring buffers or packet not fitting in the allocated
buffers), the actual length value only matters in the
sense that it prevents us from using buffers unnecessarily--
something that isn't all that relevant if it's hosed enough
to have unauthorized readers on the socket.
Is this case worth the performance penalty we'll no
doubt pay for either locking the socket or always allocating
for a max-sized packet? I'll experiment with a couple
solutions here, but unless I've missed something, we might
be better off just leaving it as-is.
+-DLS
^ permalink raw reply
* Re: [PATCH 0/2] virtio: console: Trivial fixes based on review comments
From: Amit Shah @ 2010-03-09 6:36 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization, quintela
In-Reply-To: <20100309062849.GA3814@redhat.com>
On (Tue) Mar 09 2010 [08:28:50], Michael S. Tsirkin wrote:
> On Tue, Mar 09, 2010 at 09:25:56AM +0530, Amit Shah wrote:
> > On (Mon) Mar 08 2010 [23:23:20], Michael S. Tsirkin wrote:
> > > On Mon, Mar 08, 2010 at 02:16:58PM +0530, Amit Shah wrote:
> > > > Hello,
> > > >
> > > > Here are a couple of small fixes for the virtio_console code, it's mostly
> > > > stylistic fixes.
> > > >
> > > > Michael, can you push these to Linus? Thanks.
> > >
> > > We are in fixes only mode now. Does the first patch fix a bug,
> > > or is it cosmetic only?
> >
> > No, it's not a bug fix. The value written to 'len' is write-only, we
> > never read from it.
>
> Then IMO we are better off queueing it up for 2.6.35,
> no need to create unnecessary noise.
I'm fine with that too.
Amit
^ permalink raw reply
* Re: [PATCH 0/2] virtio: console: Trivial fixes based on review comments
From: Michael S. Tsirkin @ 2010-03-09 6:28 UTC (permalink / raw)
To: Amit Shah; +Cc: virtualization, quintela
In-Reply-To: <20100309035556.GA18898@amit-x200.redhat.com>
On Tue, Mar 09, 2010 at 09:25:56AM +0530, Amit Shah wrote:
> On (Mon) Mar 08 2010 [23:23:20], Michael S. Tsirkin wrote:
> > On Mon, Mar 08, 2010 at 02:16:58PM +0530, Amit Shah wrote:
> > > Hello,
> > >
> > > Here are a couple of small fixes for the virtio_console code, it's mostly
> > > stylistic fixes.
> > >
> > > Michael, can you push these to Linus? Thanks.
> >
> > We are in fixes only mode now. Does the first patch fix a bug,
> > or is it cosmetic only?
>
> No, it's not a bug fix. The value written to 'len' is write-only, we
> never read from it.
>
> Amit
Then IMO we are better off queueing it up for 2.6.35,
no need to create unnecessary noise.
--
MST
^ permalink raw reply
* Re: [PATCH 0/2] virtio: console: Trivial fixes based on review comments
From: Amit Shah @ 2010-03-09 3:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization, quintela
In-Reply-To: <20100308212319.GA13886@redhat.com>
On (Mon) Mar 08 2010 [23:23:20], Michael S. Tsirkin wrote:
> On Mon, Mar 08, 2010 at 02:16:58PM +0530, Amit Shah wrote:
> > Hello,
> >
> > Here are a couple of small fixes for the virtio_console code, it's mostly
> > stylistic fixes.
> >
> > Michael, can you push these to Linus? Thanks.
>
> We are in fixes only mode now. Does the first patch fix a bug,
> or is it cosmetic only?
No, it's not a bug fix. The value written to 'len' is write-only, we
never read from it.
Amit
^ permalink raw reply
* Re: [PATCH 0/2] virtio: console: Trivial fixes based on review comments
From: Michael S. Tsirkin @ 2010-03-08 21:23 UTC (permalink / raw)
To: Amit Shah; +Cc: virtualization, quintela
In-Reply-To: <1268038020-12539-1-git-send-email-amit.shah@redhat.com>
On Mon, Mar 08, 2010 at 02:16:58PM +0530, Amit Shah wrote:
> Hello,
>
> Here are a couple of small fixes for the virtio_console code, it's mostly
> stylistic fixes.
>
> Michael, can you push these to Linus? Thanks.
We are in fixes only mode now. Does the first patch fix a bug,
or is it cosmetic only?
> Amit Shah (2):
> virtio: console: Fix type of 'len' as unsigned int
> virtio: console: Use better variable names for fill_queue operation
>
> drivers/char/virtio_console.c | 29 ++++++++++++++++-------------
> 1 files changed, 16 insertions(+), 13 deletions(-)
^ permalink raw reply
* Re: [PATCH 0/2] virtio: console: Trivial fixes based on review comments
From: Juan Quintela @ 2010-03-08 10:34 UTC (permalink / raw)
To: Amit Shah; +Cc: virtualization, mst
In-Reply-To: <1268038020-12539-1-git-send-email-amit.shah@redhat.com>
Amit Shah <amit.shah@redhat.com> wrote:
> Hello,
>
> Here are a couple of small fixes for the virtio_console code, it's mostly
> stylistic fixes.
>
> Michael, can you push these to Linus? Thanks.
>
> Amit Shah (2):
> virtio: console: Fix type of 'len' as unsigned int
> virtio: console: Use better variable names for fill_queue operation
>
> drivers/char/virtio_console.c | 29 ++++++++++++++++-------------
> 1 files changed, 16 insertions(+), 13 deletions(-)
Acked-by: Juan Quintela <quintela@redhat.com>
^ permalink raw reply
* [PATCH 2/2] virtio: console: Use better variable names for fill_queue operation
From: Amit Shah @ 2010-03-08 8:47 UTC (permalink / raw)
To: mst; +Cc: Amit Shah, virtualization, quintela
In-Reply-To: <1268038020-12539-2-git-send-email-amit.shah@redhat.com>
We want to keep track of the number of buffers added to a vq. Use
nr_added_bufs instead of 'ret'.
Also, the users of fill_queue() overloaded a local 'err' variable to
check the numbers of buffers allocated. Use nr_added_bufs instead of
err.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reported-by: Juan Quintela <quintela@redhat.com>
---
drivers/char/virtio_console.c | 27 +++++++++++++++------------
1 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 2bd6a9c..f404ccf 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1071,27 +1071,27 @@ static void config_intr(struct virtio_device *vdev)
static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
{
struct port_buffer *buf;
- unsigned int ret;
- int err;
+ unsigned int nr_added_bufs;
+ int ret;
- ret = 0;
+ nr_added_bufs = 0;
do {
buf = alloc_buf(PAGE_SIZE);
if (!buf)
break;
spin_lock_irq(lock);
- err = add_inbuf(vq, buf);
- if (err < 0) {
+ ret = add_inbuf(vq, buf);
+ if (ret < 0) {
spin_unlock_irq(lock);
free_buf(buf);
break;
}
- ret++;
+ nr_added_bufs++;
spin_unlock_irq(lock);
- } while (err > 0);
+ } while (ret > 0);
- return ret;
+ return nr_added_bufs;
}
static int add_port(struct ports_device *portdev, u32 id)
@@ -1100,6 +1100,7 @@ static int add_port(struct ports_device *portdev, u32 id)
struct port *port;
struct port_buffer *buf;
dev_t devt;
+ unsigned int nr_added_bufs;
int err;
port = kmalloc(sizeof(*port), GFP_KERNEL);
@@ -1144,8 +1145,8 @@ static int add_port(struct ports_device *portdev, u32 id)
init_waitqueue_head(&port->waitqueue);
/* Fill the in_vq with buffers so the host can send us data. */
- err = fill_queue(port->in_vq, &port->inbuf_lock);
- if (!err) {
+ nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock);
+ if (!nr_added_bufs) {
dev_err(port->dev, "Error allocating inbufs\n");
err = -ENOMEM;
goto free_device;
@@ -1442,12 +1443,14 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
INIT_LIST_HEAD(&portdev->ports);
if (multiport) {
+ unsigned int nr_added_bufs;
+
spin_lock_init(&portdev->cvq_lock);
INIT_WORK(&portdev->control_work, &control_work_handler);
INIT_WORK(&portdev->config_work, &config_work_handler);
- err = fill_queue(portdev->c_ivq, &portdev->cvq_lock);
- if (!err) {
+ nr_added_bufs = fill_queue(portdev->c_ivq, &portdev->cvq_lock);
+ if (!nr_added_bufs) {
dev_err(&vdev->dev,
"Error allocating buffers for control queue\n");
err = -ENOMEM;
--
1.6.2.5
^ permalink raw reply related
* [PATCH 1/2] virtio: console: Fix type of 'len' as unsigned int
From: Amit Shah @ 2010-03-08 8:46 UTC (permalink / raw)
To: mst; +Cc: Amit Shah, virtualization, quintela
In-Reply-To: <1268038020-12539-1-git-send-email-amit.shah@redhat.com>
We declare 'len' as int type but it should be 'unsigned int', as
get_buf() wants it to be.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reported-by: Juan Quintela <quintela@redhat.com>
---
drivers/char/virtio_console.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 213373b..2bd6a9c 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -379,7 +379,7 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
struct scatterlist sg[1];
struct virtio_console_control cpkt;
struct virtqueue *vq;
- int len;
+ unsigned int len;
if (!use_multiport(port->portdev))
return 0;
--
1.6.2.5
^ permalink raw reply related
* [PATCH 0/2] virtio: console: Trivial fixes based on review comments
From: Amit Shah @ 2010-03-08 8:46 UTC (permalink / raw)
To: mst; +Cc: Amit Shah, virtualization, quintela
Hello,
Here are a couple of small fixes for the virtio_console code, it's mostly
stylistic fixes.
Michael, can you push these to Linus? Thanks.
Amit Shah (2):
virtio: console: Fix type of 'len' as unsigned int
virtio: console: Use better variable names for fill_queue operation
drivers/char/virtio_console.c | 29 ++++++++++++++++-------------
1 files changed, 16 insertions(+), 13 deletions(-)
^ permalink raw reply
* Re: [RFC][ PATCH 3/3] vhost-net: Add mergeable RX buffer support to vhost-net
From: Michael S. Tsirkin @ 2010-03-08 8:07 UTC (permalink / raw)
To: David Stevens; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <OF8D4A9381.FF5A483F-ON882576E0.0008221D-882576E0.000B9D4A@us.ibm.com>
On Sun, Mar 07, 2010 at 06:06:51PM -0800, David Stevens wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote on 03/07/2010 08:26:33 AM:
>
> > On Tue, Mar 02, 2010 at 05:20:34PM -0700, David Stevens wrote:
> > > This patch glues them all together and makes sure we
> > > notify whenever we don't have enough buffers to receive
> > > a max-sized packet, and adds the feature bit.
> > >
> > > Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
> >
> > Maybe split this up?
>
> I can. I was looking mostly at size (and this is the smallest
> of the bunch). But the feature requires all of them together, of course.
> This last one is just "everything left over" from the other two.
>
> > > @@ -110,6 +90,7 @@
> > > size_t len, total_len = 0;
> > > int err, wmem;
> > > struct socket *sock = rcu_dereference(vq->private_data);
> > > +
> >
> > I tend not to add empty lines if line below it is already short.
>
> This leaves no blank line between the declarations and the start
> of code. It's habit for me-- not sure of kernel coding standards address
> that or not, but I don't think I've seen it anywhere else.
>
> >
> > > if (!sock)
> > > return;
> > >
> > > @@ -166,11 +147,11 @@
> > > /* Skip header. TODO: support TSO. */
> > > msg.msg_iovlen = out;
> > > head.iov_len = len = iov_length(vq->iov, out);
> > > +
> >
> > I tend not to add empty lines if line below it is a comment.
>
> I added this to separate the logical "skip header" block from
> the next, unrelated piece. Not important to me, though.
>
> >
> > > /* Sanity check */
> > > if (!len) {
> > > vq_err(vq, "Unexpected header len for TX: "
> > > - "%zd expected %zd\n",
> > > - len, vq->guest_hlen);
> > > + "%zd expected %zd\n", len,
> vq->guest_hlen);
> > > break;
> > > }
> > > /* TODO: Check specific error and bomb out unless
> ENOBUFS?
> > > */
>
>
> > > /* 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_bufs */
> > > + vq->iov[0].iov_len = vq->guest_hlen;
> > > + vhdr->num_buffers = headcount;
> >
> > I don't understand this. iov_base is a userspace pointer, isn't it.
> > How can you assign values to it like that?
> > Rusty also commented earlier that it's not a good idea to assume
> > specific layout, such as first chunk being large enough to
> > include virtio_net_hdr_mrg_rxbuf.
> >
> > I think we need to use memcpy to/from iovec etc.
>
> I guess you mean put_user() or copy_to_user(); yes, I suppose
> it could be paged since we read it.
> The code doesn't assume that it'll fit so much as arranged for
> it to fit. We allocate guest_hlen bytes in the buffer, but set the
> iovec to the (smaller) sock_hlen; do the read, then this code adds
> back the 2 bytes in the middle that we didn't read into (where
> num_buffers goes). But the allocator does require that guest_hlen
> will fit in a single buffer (and reports error if it doesn't). The
> alternative is significantly more complicated,
I'm not sure why. Can't we just call memcpy_from_iovec
and then read the structure as usual?
> and only fails if
> the guest doesn't give us at least the buffer size the guest header
> requires (a truly lame guest). I'm not sure it's worth a lot of
> complexity in vhost to support the guest giving us <12 byte buffers;
> those guests don't exist now and maybe they never should?
>
>
> > > /* This actually signals the guest, using eventfd. */
> > > void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> > > {
> > > __u16 flags = 0;
> > > +
> >
> > I tend not to add empty lines if a line above it is already short.
>
> Again, separating declarations from code-- never seen different
> in any other kernel code.
>
> >
> > > if (get_user(flags, &vq->avail->flags)) {
> > > vq_err(vq, "Failed to get flags");
> > > return;
> > > @@ -1125,7 +1140,7 @@
> > >
> > > /* If they don't want an interrupt, don't signal, unless
> empty. */
> > > if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
> > > - (vq->avail_idx != vq->last_avail_idx ||
> > > + (vhost_available(vq) > vq->maxheadcount ||
> >
> > I don't understand this change. It seems to make
> > code not match the comments.
>
> It redefines "empty". Without mergeable buffers, we can empty
> the ring down to nothing before we require notification. With
> mergeable buffers, if the packet requires, say, 3 buffers, and we
> have only 2 left, we are empty and require notification and new
> buffers to read anything. In both cases, we notify when we can't
> read another packet without more buffers.
I don't really see how you can find this out from just the number of
heads. A head can address buffer of any size. Need to think
about this code some more.
> I can think about changing the comment to reflect this more
> clearly.
Also, this is all for rx only, so names should reflect this I think.
> >
> > > !vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
> > > return;
> > >
> > > diff -ruN net-next-p2/drivers/vhost/vhost.h
> > > net-next-p3/drivers/vhost/vhost.h
> > > --- net-next-p2/drivers/vhost/vhost.h 2010-03-02 13:02:03.000000000
> > > -0800
> > > +++ net-next-p3/drivers/vhost/vhost.h 2010-03-02 14:29:44.000000000
> > > -0800
> > > @@ -85,6 +85,7 @@
> > > struct iovec iov[VHOST_NET_MAX_SG+1]; /* an extra for vnet hdr
> */
> > > struct iovec heads[VHOST_NET_MAX_SG];
> > > size_t guest_hlen, sock_hlen;
> > > + int maxheadcount;
> >
> > I don't completely understand what does this field does.
> > It seems to be only set on rx? Maybe name should reflect this?
>
> This is a way for me to dynamically guess how many heads I need
> for a
> max-sized packet for whatever the MTU/GRO settings are without waiting to
> detect the need for more buffers until a read fails. Without mergeable
> buffers,
> we can always fit a max-sized packet in 1 head, but with, we need more
> than
> one head to do the read.
>
> I didn't want to hard-code 64K (which it usually is, but not always), and
> I didn't want to wait until a read fails every time the ring is near full.
> I played with re-enabling notify on 1/4 available or some such, but that
> delays
> reads unnecessarily, so I came up with this method: use maxheadcount to
> track
> the biggest packet we've ever seen and always make sure we have at least
> that
> many available for the next read. If it increases, we may fail the read,
> which'll
> notify, but this allows us to notify before we try and fail in normal
> operation,
> while still not doing a notify on every read.
>
> +-DLS
Hmm, yes. One of the horrors of the mergeable buffer hack.
Not sure all this is worth the complexity though: I don't think this
covers all cases whether the size would be < 64K, anyway: you don't get
notified when user disables GRO on physical NIC, for example. So maybe
just go ahead with hard-coding 64K.
In any case, number of heads does not necessarily tell us much either,
does it?
Would interrupt when we actually don't have room on RX work better? I'll
think about it some more.
--
MST
^ permalink raw reply
* Re: [RFC][ PATCH 2/3] vhost-net: handle vnet_hdr processing for MRG_RX_BUF
From: Michael S. Tsirkin @ 2010-03-08 7:54 UTC (permalink / raw)
To: David Stevens; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <OF8A4807F2.DE42C086-ON882576E0.00033A96-882576E0.00080FCA@us.ibm.com>
On Sun, Mar 07, 2010 at 05:28:02PM -0800, David Stevens wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote on 03/07/2010 08:12:29 AM:
>
> > On Tue, Mar 02, 2010 at 05:20:26PM -0700, David Stevens wrote:
> > > This patch adds vnet_hdr processing for mergeable buffer
> > > support to vhost-net.
> > >
> > > Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
> > >
> > > diff -ruN net-next-p1/drivers/vhost/net.c
> net-next-p2/drivers/vhost/net.c
> >
> > Could you please add -p to diff flags so that it's easier
> > to figure out which function is changes?
>
> Sure.
>
>
> > > @@ -148,25 +146,45 @@
> > > "out %d, int %d\n", out, in);
> > > break;
> > > }
> > > + if (vq->guest_hlen > vq->sock_hlen) {
> > > + if (msg.msg_iov[0].iov_len == vq->guest_hlen)
> > > + msg.msg_iov[0].iov_len =
> vq->sock_hlen;
> > > + else if (out == ARRAY_SIZE(vq->iov))
> > > + vq_err(vq, "handle_tx iov overflow!");
> > > + else {
> > > + int i;
> > > +
> > > + /* give header its own iov */
> > > + for (i=out; i>0; ++i)
> > > + msg.msg_iov[i+1] =
> msg.msg_iov[i];
> > > + msg.msg_iov[0].iov_len =
> vq->sock_hlen;
> > > + msg.msg_iov[1].iov_base +=
> vq->guest_hlen;
> > > + msg.msg_iov[1].iov_len -=
> vq->guest_hlen;
> > > + out++;
> >
> > We seem to spend a fair bit of code here and elsewhere trying to cover
> > the diff between header size in guest and host. In hindsight, it was
> > not a good idea to put new padding between data and the header:
> > virtio-net should have added it *before*. But it is what it is.
> >
> > Wouldn't it be easier to just add an ioctl to tap so that
> > vnet header size is made bigger by 4 bytes?
>
> I'd be ok with that, but if we support raw sockets, we have
> some of the issues (easier, since it's 0). "num_buffers" is only
> 16 bits, so just add 2 bytes, but yeah-- pushing it to tap would
> be easier for vhost.
>
>
> > > /* TODO: Check specific error and bomb out unless
> ENOBUFS?
> > > */
> > > err = sock->ops->sendmsg(NULL, sock, &msg, len);
> > > if (unlikely(err < 0)) {
> > > - vhost_discard(vq, 1);
> > > - tx_poll_start(net, sock);
> > > + if (err == -EAGAIN) {
> >
> > The comment mentions ENOBUFS. Are you sure it's EAGAIN?
>
> The comment's from the original -- the code changes should do
> the "TODO", so probably should remove the comment.
> >
> > > + tx_poll_start(net, sock);
> >
> > Don't we need to call discard to move the last avail header back?
>
> Yes, I think you're right. We might consider dropping the packet
> in the EAGAIN case here instead, though. It's not clear retrying the
> same packet with a delay here is better than getting new ones when the
> socket buffer is full (esp. with queues on both sides already).
You mean already full? Well, this one triggers even if queue
on guest side is not full. TCP might be ok with it, but I suspect
packet drops would be very bad for UDP.
> Maybe
> we should fold EAGAIN into the other error cases? Otherwise, you're
> right, it looks like it should have a discard here.
>
> >
> > > + } else {
> > > + vq_err(vq, "sendmsg: errno %d\n",
> -err);
> > > + /* drop packet; do not discard/resend
> */
> > > + vhost_add_used_and_signal(&net->dev,vq,&head,1);
> > > + }
> > > break;
> > > - }
> > > - if (err != len)
> > > + } else if (err != len)
> >
> >
> > Previous if ends with break so no need for else here.
>
> Yes.
>
>
> > > @@ -232,25 +243,18 @@
> > > headcount = vhost_get_heads(vq, datalen, &in, vq_log,
> > > &log);
> > > /* OK, now we need to know about added descriptors. */
> > > if (!headcount) {
> > > - if (unlikely(vhost_enable_notify(vq))) {
> > > - /* They have slipped one in as we were
> > > - * doing that: check again. */
> > > - vhost_disable_notify(vq);
> > > - continue;
> > > - }
> > > - /* Nothing new? Wait for eventfd to tell us
> > > - * they refilled. */
> > > + vhost_enable_notify(vq);
> >
> >
> > You don't think this race can happen?
>
> The notify case has to allow for multiple heads, not just the one,
> so if he added just one head, it doesn't mean we're ok to continue-- the
> packet may require multiple. Instead, the notifier now checks for enough
> to handle a max-sized packet (whether or not NONOTIFY is set). I'll think
> about this some more, but I concluded it wasn't needed at the time. :-)
>
Hmm, need to think about it as well.
> > > @@ -519,6 +524,20 @@
> > >
> > > vhost_net_disable_vq(n, vq);
> > > rcu_assign_pointer(vq->private_data, sock);
> > > +
> > > + if (sock && sock->sk) {
> > > + if (!vhost_sock_is_raw(sock) ||
> >
> > I dislike this backend-specific code, it ties us with specifics of
> > backend implementations, which change without notice. Ideally all
> > backend-specific stuff should live in userspace, userspace programs
> > vhost device appropriately.
>
> We could do that; we need to know whether the socket has a
> vnet header on it or not and the existing flags don't tell us. If
> qemu sets a flag telling us the socket is has or doesn't have vnet,
> that'd be equivalent.
qemu tells us how much of the header to skip before passing
it to socket.
> > > + vhost_has_feature(&n->dev,
> > > VHOST_NET_F_VIRTIO_NET_HDR)) {
> > > + vq->sock_hlen = sizeof(struct virtio_net_hdr);
> > > + if (vhost_has_feature(&n->dev,
> > > VIRTIO_NET_F_MRG_RXBUF))
> > > + vq->guest_hlen =
> > > + sizeof(struct
> > > virtio_net_hdr_mrg_rxbuf);
> > > + else
> > > + vq->guest_hlen = sizeof(struct
> > > virtio_net_hdr);
> > > + } else
> > > + vq->guest_hlen = vq->sock_hlen = 0;
> > > + } else
> > > + vq_err(vq, "vhost_net_set_backend: sock->sk is NULL");
> >
> > As proposed above, IMO it would be nicer to add an ioctl in tap
> > that let us program header size.
>
> Do you know if Herbert or Dave have an opinion on that solution?
I'l post a patch, and we'll see.
> > > +static int
> > > +vhost_get_hdr(struct vhost_virtqueue *vq, int *in, struct vhost_log
> *log,
> > > + int *log_num)
> > > +{
> > > + struct iovec *heads = vq->heads;
> > > + struct iovec *iov = vq->iov;
> > > + int out;
> > > +
> > > + *in = 0;
> > > + iov[0].iov_len = 0;
> > > +
> > > + /* get buffer, starting from iov[1] */
> > > + heads[0].iov_base = (void *)vhost_get_vq_desc(vq->dev, vq,
> > > + vq->iov+1, ARRAY_SIZE(vq->iov)-1, &out, in, log,
> log_num);
> > > + if (out || *in <= 0) {
> > > + vq_err(vq, "unexpected descriptor format for RX: out
> %d, "
> > > + "in %d\n", out, *in);
> > > + return 0;
> > > + }
> > > + if (heads[0].iov_base == (void *)vq->num)
> > > + return 0;
> > > +
> > > + /* make iov[0] the header */
> > > + if (!vq->guest_hlen) {
> > > + if (vq->sock_hlen) {
> > > + static struct virtio_net_hdr junk; /* bit
> bucket
> > > */
> > > +
> > > + iov[0].iov_base = &junk;
> > > + iov[0].iov_len = sizeof(junk);
> > > + } else
> > > + iov[0].iov_len = 0;
> > > + }
> > > + if (vq->sock_hlen < vq->guest_hlen) {
> > > + iov[0].iov_base = iov[1].iov_base;
> > > + iov[0].iov_len = vq->sock_hlen;
> > > +
> > > + if (iov[1].iov_len < vq->sock_hlen) {
> > > + vq_err(vq, "can't fit header in one buffer!");
> > > + vhost_discard(vq, 1);
> > > + return 0;
> > > + }
> > > + if (!vq->sock_hlen) {
> > > + static const struct virtio_net_hdr_mrg_rxbuf
> hdr =
> > > {
> > > + .hdr.flags = 0,
> > > + .hdr.gso_type =
> VIRTIO_NET_HDR_GSO_NONE
> > > + };
> > > + memcpy(iov[0].iov_base, &hdr, vq->guest_hlen);
> > > + }
> > > + iov[1].iov_base += vq->guest_hlen;
> > > + iov[1].iov_len -= vq->guest_hlen;
> > > + }
> > > + return 1;
> >
> > The above looks kind of scary, lots of special-casing. I'll send a
> > patch for tap to set vnet header size, let's see if it makes life easier
> > for us.
>
> Yes, I try to handle all combinations of differing guest and
> socket
> header lengths (including 0-lengths). If we don't support raw sockets
> and/or
> we make the tap vnet header bigger in the MRXB case, it'll simplify this
> code.
Let's see about tap patch then.
> >
> > > +}
> > > +
> > > unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
>
> > > *iovcount,
> > > struct vhost_log *log, unsigned int *log_num)
> > > {
> > > struct iovec *heads = vq->heads;
> > > - int out, in;
> > > + int out, in = 0;
> > > + int seg = 0;
> > > int hc = 0;
> >
> > I think it's better to stick to simple names like i
> > for index variables, alternatively give it a meaningful
> > name like "count".
>
> I'm not sure which one you're talking about. "in & out"
> are inherited from the original, "seg" is an iovec segment index, and
> "hc" is a head counter. "headcount" or "count" (if that's the
> one you mean) instead of "hc" would probably cause some line wraps.
> I think "i" is too generic here, but if you mean "hc", I could stick
> a comment on the declaration:
>
> int hc = 0; /* head count */
>
> Does that address your concern, or do you mean something else?
I mean hc. Let's call it count and live with line wraps if you don't
like i.
> >
> > >
> > > + if (vq->guest_hlen != vq->sock_hlen) {
> >
> > Sticking guest_hlen/sock_hlen in vhost is somewhat ugly.
>
> Do you mean in the struct? This essentially splits the
> original "hdrsize" into the two (possibly different) header
> sizes and allows removing the "hdr" iovec and reading the
> (partial) header directly into the extended header mergeable
> buffers needs.
Yes. hdrsize is kind of generic, socket is obviously net specific ...
> Unless we remove raw socket support and add the
> ioctl to tap you suggested, I'm not sure it can be much cleaner.
> For me, the ugliness comes from tap, raw and guest headers not
> being the same.
>
> +-DLS
--
MST
^ permalink raw reply
* Re: [RFC][ PATCH 1/3] vhost-net: support multiple buffer heads in receiver
From: Michael S. Tsirkin @ 2010-03-08 7:45 UTC (permalink / raw)
To: David Stevens; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <OFEEE8B4AB.0C708EB8-ON882576DF.00831519-882576E0.000329C4@us.ibm.com>
On Sun, Mar 07, 2010 at 04:34:32PM -0800, David Stevens wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote on 03/07/2010 07:31:30 AM:
>
> > On Tue, Mar 02, 2010 at 05:20:15PM -0700, David Stevens wrote:
> > > This patch generalizes buffer handling functions to
> NULL, NULL);
> > > + head.iov_base = (void *)vhost_get_vq_desc(&net->dev,
> vq,
> > > + vq->iov, ARRAY_SIZE(vq->iov), &out, &in, NULL,
>
> > > NULL);
> >
> > Should type for head be changed so that we do not need to cast?
> >
> > I also prefer aligning descendants on the opening (.
>
> Yes, that's probably better; the indentation with the cast would
> still wrap a lot, but I'll see what I can do here.
>
>
> > > err = sock->ops->sendmsg(NULL, sock, &msg, len);
> > > if (unlikely(err < 0)) {
> > > - vhost_discard_vq_desc(vq);
> > > + vhost_discard(vq, 1);
> >
> > Isn't the original name a bit more descriptive?
>
> During development, I had both and I generally like
> shorter names, but I can change it back.
>
> > > +static int skb_head_len(struct sk_buff_head *skq)
> > > +{
> > > + struct sk_buff *head;
> > > +
> > > + head = skb_peek(skq);
> > > + if (head)
> > > + return head->len;
> > > + return 0;
> > > +}
> > > +
> >
> > This is done without locking, which I think can crash
> > if skb is consumed after we peek it but before we read the
> > length.
>
> This thread is the only legitimate consumer, right? But
> qemu has the file descriptor and I guess we shouldn't trust
> that it won't give it to someone else; it'd break vhost, but
> a crash would be inappropriate, of course. I'd like to avoid
> the lock, but I have another idea here, so will investigate.
>
> >
> >
> > > /* 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,
> > > @@ -204,10 +213,11 @@
> > > };
> > >
> > > 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))
> > > +
> > > + if (!sock || !skb_head_len(&sock->sk->sk_receive_queue))
> > > return;
> > >
> >
> > Isn't this equivalent? Do you expect zero len skbs in socket?
> > If yes, maybe we should discard these, not stop processing ...
>
> A zero return means "no skb's". They are equivalent; I
> changed this call just to make it identical to the loop check,
> but I don't have a strong attachment to this.
>
>
> > > 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 = skb_head_len(&sock->sk->sk_receive_queue)))
> {
> >
> > This peeks in the queue to figure out how much data we have.
> > It's a neat trick, but it does introduce new failure modes
> > where an skb is consumed after we did the peek:
> > - new skb could be shorter, we should return the unconsumed part
> > - new skb could be longer, this will drop a packet.
> > maybe this last is not an issue as the race should be rare in practice
>
> As before, this loop is the only legitimate consumer of skb's; if
> anyone else is reading the socket, it's broken. But since the file
> descriptor is available to qemu, it's probably trusting qemu too much.
> I don't believe there is a race at all on a working system; the head
> can't change until we read it after this test, but I'll see if I can
> come up with something better here. Closing the fd for qemu so vhost
> has exclusive access might do it, but otherwise we could just get a
> max-sized packet worth of buffers and then return them after the read.
> That'd force us to wait with small packets when the ring is nearly
> full, where we don't have to now, but I expect locking to read the head
> length will hurt performance in all cases. Will try these ideas out.k
>
> >
> > > + headcount = vhost_get_heads(vq, datalen, &in, vq_log,
> > > &log);
> > > /* OK, now we need to know about added descriptors. */
> > > - if (head == vq->num) {
> > > + if (!headcount) {
> > > if (unlikely(vhost_enable_notify(vq))) {
> > > /* They have slipped one in as we were
> > > * doing that: check again. */
> > > @@ -235,13 +242,6 @@
> > > * they refilled. */
> > > break;
> > > }
> > > - /* We don't need to be notified again. */
> >
> > you find this comment unhelpful?
>
> This code is changed in the later patches; the comment was
> removed with that, but I got it in the wrong patch on the split.
> I guess the comment is ok to stay, anyway, but notification may
> require multiple buffers to be available; I had fixed that here, but
> the final patch pushes that into the notify code, so yes, this can
> go back in.
>
> > > - if (out) {
> > > - vq_err(vq, "Unexpected descriptor format for
> RX: "
> > > - "out %d, int %d\n",
> > > - out, in);
> > > - break;
> > > - }
> >
> >
> > we still need this check, don't we?
>
> It's done in vhost_get_heads(); "out" isn't visible in handle_rx()
> anymore.
>
> > > +unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
>
> > > *iovcount,
> > > + struct vhost_log *log, unsigned int *log_num)
> >
> > Could you please document this function's parameters? It's not obvious
> > what iovcount, log, log_num are.
>
> Yes. To answer the question, they are the same as vhost_get_vq_desc(),
> except "iovcount" replaces "in" (and "out" is not needed in the caller).
>
> > > +{
> > > + struct iovec *heads = vq->heads;
> > > + int out, in;
> > > + int hc = 0;
> > > +
> > > + while (datalen > 0) {
> > > + if (hc >= VHOST_NET_MAX_SG) {
> > > + vhost_discard(vq, hc);
> > > + return 0;
> > > + }
> > > + heads[hc].iov_base = (void
> *)vhost_get_vq_desc(vq->dev,
> > > vq,
> > > + vq->iov, ARRAY_SIZE(vq->iov), &out, &in, log,
> > > log_num);
> > > + if (heads[hc].iov_base == (void *)vq->num) {
> > > + vhost_discard(vq, hc);
> > > + return 0;
> > > + }
> > > + if (out || in <= 0) {
> > > + vq_err(vq, "unexpected descriptor format for
> RX: "
> > > + "out %d, in %d\n", out, in);
> > > + vhost_discard(vq, hc);
> > > + return 0;
> > > + }
> > > + heads[hc].iov_len = iov_length(vq->iov, in);
> > > + hc++;
> > > + datalen -= heads[hc].iov_len;
> > > + }
> > > + *iovcount = in;
> >
> >
> > Only the last value?
>
> In this part of the split, it only goes through once; this is
> changed to the accumulated value "seg" in a later patch. So, split
> artifact.
>
> {
> > > struct vring_used_elem *used;
> > > + int i;
> > >
> > > - /* 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");
> > > - return -EFAULT;
> > > - }
> > > - if (put_user(len, &used->len)) {
> > > - vq_err(vq, "Failed to write used len");
> > > - return -EFAULT;
> > > + for (i=0; i<count; i++, vq->last_used_idx++) {
> >
> > whitespace damage: I prefer space around =, <.
> > I also use ++i, etc in this driver, better be consistent?
> > Also for clarity, I prefer to put vq->last_used_idx inside the loop.
>
> OK.
> >
> > > + used = &vq->used->ring[vq->last_used_idx % vq->num];
> > > + if (put_user((unsigned)heads[i].iov_base, &used->id))
> {
> > > + vq_err(vq, "Failed to write used id");
> > > + return -EFAULT;
> > > + }
> > > + if (put_user(heads[i].iov_len, &used->len)) {
> > > + vq_err(vq, "Failed to write used len");
> > > + return -EFAULT;
> > > + }
> >
> > If this fails, last_used_idx will still be incremented, which I think is
> wrong.
>
> True, but if these fail, aren't we dead? I don't think we can
> recover
> from an EFAULT in any of these; I didn't test those paths from the
> original,
> but I think we need to bail out entirely for these cases, right?
>
> +-DLS
Yes, we stop on error, but host can fix uo the vq and redo a kick.
That's why there's errorfd.
--
MST
^ permalink raw reply
* Re: [RFC][ PATCH 3/3] vhost-net: Add mergeable RX buffer support to vhost-net
From: David Stevens @ 2010-03-08 2:06 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <20100307162633.GC24997@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote on 03/07/2010 08:26:33 AM:
> On Tue, Mar 02, 2010 at 05:20:34PM -0700, David Stevens wrote:
> > This patch glues them all together and makes sure we
> > notify whenever we don't have enough buffers to receive
> > a max-sized packet, and adds the feature bit.
> >
> > Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
>
> Maybe split this up?
I can. I was looking mostly at size (and this is the smallest
of the bunch). But the feature requires all of them together, of course.
This last one is just "everything left over" from the other two.
> > @@ -110,6 +90,7 @@
> > size_t len, total_len = 0;
> > int err, wmem;
> > struct socket *sock = rcu_dereference(vq->private_data);
> > +
>
> I tend not to add empty lines if line below it is already short.
This leaves no blank line between the declarations and the start
of code. It's habit for me-- not sure of kernel coding standards address
that or not, but I don't think I've seen it anywhere else.
>
> > if (!sock)
> > return;
> >
> > @@ -166,11 +147,11 @@
> > /* Skip header. TODO: support TSO. */
> > msg.msg_iovlen = out;
> > head.iov_len = len = iov_length(vq->iov, out);
> > +
>
> I tend not to add empty lines if line below it is a comment.
I added this to separate the logical "skip header" block from
the next, unrelated piece. Not important to me, though.
>
> > /* Sanity check */
> > if (!len) {
> > vq_err(vq, "Unexpected header len for TX: "
> > - "%zd expected %zd\n",
> > - len, vq->guest_hlen);
> > + "%zd expected %zd\n", len,
vq->guest_hlen);
> > break;
> > }
> > /* TODO: Check specific error and bomb out unless
ENOBUFS?
> > */
> > /* 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_bufs */
> > + vq->iov[0].iov_len = vq->guest_hlen;
> > + vhdr->num_buffers = headcount;
>
> I don't understand this. iov_base is a userspace pointer, isn't it.
> How can you assign values to it like that?
> Rusty also commented earlier that it's not a good idea to assume
> specific layout, such as first chunk being large enough to
> include virtio_net_hdr_mrg_rxbuf.
>
> I think we need to use memcpy to/from iovec etc.
I guess you mean put_user() or copy_to_user(); yes, I suppose
it could be paged since we read it.
The code doesn't assume that it'll fit so much as arranged for
it to fit. We allocate guest_hlen bytes in the buffer, but set the
iovec to the (smaller) sock_hlen; do the read, then this code adds
back the 2 bytes in the middle that we didn't read into (where
num_buffers goes). But the allocator does require that guest_hlen
will fit in a single buffer (and reports error if it doesn't). The
alternative is significantly more complicated, and only fails if
the guest doesn't give us at least the buffer size the guest header
requires (a truly lame guest). I'm not sure it's worth a lot of
complexity in vhost to support the guest giving us <12 byte buffers;
those guests don't exist now and maybe they never should?
> > /* This actually signals the guest, using eventfd. */
> > void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> > {
> > __u16 flags = 0;
> > +
>
> I tend not to add empty lines if a line above it is already short.
Again, separating declarations from code-- never seen different
in any other kernel code.
>
> > if (get_user(flags, &vq->avail->flags)) {
> > vq_err(vq, "Failed to get flags");
> > return;
> > @@ -1125,7 +1140,7 @@
> >
> > /* If they don't want an interrupt, don't signal, unless
empty. */
> > if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
> > - (vq->avail_idx != vq->last_avail_idx ||
> > + (vhost_available(vq) > vq->maxheadcount ||
>
> I don't understand this change. It seems to make
> code not match the comments.
It redefines "empty". Without mergeable buffers, we can empty
the ring down to nothing before we require notification. With
mergeable buffers, if the packet requires, say, 3 buffers, and we
have only 2 left, we are empty and require notification and new
buffers to read anything. In both cases, we notify when we can't
read another packet without more buffers.
I can think about changing the comment to reflect this more
clearly.
>
> > !vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
> > return;
> >
> > diff -ruN net-next-p2/drivers/vhost/vhost.h
> > net-next-p3/drivers/vhost/vhost.h
> > --- net-next-p2/drivers/vhost/vhost.h 2010-03-02 13:02:03.000000000
> > -0800
> > +++ net-next-p3/drivers/vhost/vhost.h 2010-03-02 14:29:44.000000000
> > -0800
> > @@ -85,6 +85,7 @@
> > struct iovec iov[VHOST_NET_MAX_SG+1]; /* an extra for vnet hdr
*/
> > struct iovec heads[VHOST_NET_MAX_SG];
> > size_t guest_hlen, sock_hlen;
> > + int maxheadcount;
>
> I don't completely understand what does this field does.
> It seems to be only set on rx? Maybe name should reflect this?
This is a way for me to dynamically guess how many heads I need
for a
max-sized packet for whatever the MTU/GRO settings are without waiting to
detect the need for more buffers until a read fails. Without mergeable
buffers,
we can always fit a max-sized packet in 1 head, but with, we need more
than
one head to do the read.
I didn't want to hard-code 64K (which it usually is, but not always), and
I didn't want to wait until a read fails every time the ring is near full.
I played with re-enabling notify on 1/4 available or some such, but that
delays
reads unnecessarily, so I came up with this method: use maxheadcount to
track
the biggest packet we've ever seen and always make sure we have at least
that
many available for the next read. If it increases, we may fail the read,
which'll
notify, but this allows us to notify before we try and fail in normal
operation,
while still not doing a notify on every read.
+-DLS
^ permalink raw reply
* Re: [RFC][ PATCH 2/3] vhost-net: handle vnet_hdr processing for MRG_RX_BUF
From: David Stevens @ 2010-03-08 1:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <20100307161229.GB24997@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote on 03/07/2010 08:12:29 AM:
> On Tue, Mar 02, 2010 at 05:20:26PM -0700, David Stevens wrote:
> > This patch adds vnet_hdr processing for mergeable buffer
> > support to vhost-net.
> >
> > Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
> >
> > diff -ruN net-next-p1/drivers/vhost/net.c
net-next-p2/drivers/vhost/net.c
>
> Could you please add -p to diff flags so that it's easier
> to figure out which function is changes?
Sure.
> > @@ -148,25 +146,45 @@
> > "out %d, int %d\n", out, in);
> > break;
> > }
> > + if (vq->guest_hlen > vq->sock_hlen) {
> > + if (msg.msg_iov[0].iov_len == vq->guest_hlen)
> > + msg.msg_iov[0].iov_len =
vq->sock_hlen;
> > + else if (out == ARRAY_SIZE(vq->iov))
> > + vq_err(vq, "handle_tx iov overflow!");
> > + else {
> > + int i;
> > +
> > + /* give header its own iov */
> > + for (i=out; i>0; ++i)
> > + msg.msg_iov[i+1] =
msg.msg_iov[i];
> > + msg.msg_iov[0].iov_len =
vq->sock_hlen;
> > + msg.msg_iov[1].iov_base +=
vq->guest_hlen;
> > + msg.msg_iov[1].iov_len -=
vq->guest_hlen;
> > + out++;
>
> We seem to spend a fair bit of code here and elsewhere trying to cover
> the diff between header size in guest and host. In hindsight, it was
> not a good idea to put new padding between data and the header:
> virtio-net should have added it *before*. But it is what it is.
>
> Wouldn't it be easier to just add an ioctl to tap so that
> vnet header size is made bigger by 4 bytes?
I'd be ok with that, but if we support raw sockets, we have
some of the issues (easier, since it's 0). "num_buffers" is only
16 bits, so just add 2 bytes, but yeah-- pushing it to tap would
be easier for vhost.
> > /* TODO: Check specific error and bomb out unless
ENOBUFS?
> > */
> > err = sock->ops->sendmsg(NULL, sock, &msg, len);
> > if (unlikely(err < 0)) {
> > - vhost_discard(vq, 1);
> > - tx_poll_start(net, sock);
> > + if (err == -EAGAIN) {
>
> The comment mentions ENOBUFS. Are you sure it's EAGAIN?
The comment's from the original -- the code changes should do
the "TODO", so probably should remove the comment.
>
> > + tx_poll_start(net, sock);
>
> Don't we need to call discard to move the last avail header back?
Yes, I think you're right. We might consider dropping the packet
in the EAGAIN case here instead, though. It's not clear retrying the
same packet with a delay here is better than getting new ones when the
socket buffer is full (esp. with queues on both sides already). Maybe
we should fold EAGAIN into the other error cases? Otherwise, you're
right, it looks like it should have a discard here.
>
> > + } else {
> > + vq_err(vq, "sendmsg: errno %d\n",
-err);
> > + /* drop packet; do not discard/resend
*/
> > + vhost_add_used_and_signal(&net->dev,vq,&head,1);
> > + }
> > break;
> > - }
> > - if (err != len)
> > + } else if (err != len)
>
>
> Previous if ends with break so no need for else here.
Yes.
> > @@ -232,25 +243,18 @@
> > headcount = vhost_get_heads(vq, datalen, &in, vq_log,
> > &log);
> > /* OK, now we need to know about added descriptors. */
> > if (!headcount) {
> > - if (unlikely(vhost_enable_notify(vq))) {
> > - /* They have slipped one in as we were
> > - * doing that: check again. */
> > - vhost_disable_notify(vq);
> > - continue;
> > - }
> > - /* Nothing new? Wait for eventfd to tell us
> > - * they refilled. */
> > + vhost_enable_notify(vq);
>
>
> You don't think this race can happen?
The notify case has to allow for multiple heads, not just the one,
so if he added just one head, it doesn't mean we're ok to continue-- the
packet may require multiple. Instead, the notifier now checks for enough
to handle a max-sized packet (whether or not NONOTIFY is set). I'll think
about this some more, but I concluded it wasn't needed at the time. :-)
> > @@ -519,6 +524,20 @@
> >
> > vhost_net_disable_vq(n, vq);
> > rcu_assign_pointer(vq->private_data, sock);
> > +
> > + if (sock && sock->sk) {
> > + if (!vhost_sock_is_raw(sock) ||
>
> I dislike this backend-specific code, it ties us with specifics of
> backend implementations, which change without notice. Ideally all
> backend-specific stuff should live in userspace, userspace programs
> vhost device appropriately.
We could do that; we need to know whether the socket has a
vnet header on it or not and the existing flags don't tell us. If
qemu sets a flag telling us the socket is has or doesn't have vnet,
that'd be equivalent.
> > + vhost_has_feature(&n->dev,
> > VHOST_NET_F_VIRTIO_NET_HDR)) {
> > + vq->sock_hlen = sizeof(struct virtio_net_hdr);
> > + if (vhost_has_feature(&n->dev,
> > VIRTIO_NET_F_MRG_RXBUF))
> > + vq->guest_hlen =
> > + sizeof(struct
> > virtio_net_hdr_mrg_rxbuf);
> > + else
> > + vq->guest_hlen = sizeof(struct
> > virtio_net_hdr);
> > + } else
> > + vq->guest_hlen = vq->sock_hlen = 0;
> > + } else
> > + vq_err(vq, "vhost_net_set_backend: sock->sk is NULL");
>
> As proposed above, IMO it would be nicer to add an ioctl in tap
> that let us program header size.
Do you know if Herbert or Dave have an opinion on that solution?
> > +static int
> > +vhost_get_hdr(struct vhost_virtqueue *vq, int *in, struct vhost_log
*log,
> > + int *log_num)
> > +{
> > + struct iovec *heads = vq->heads;
> > + struct iovec *iov = vq->iov;
> > + int out;
> > +
> > + *in = 0;
> > + iov[0].iov_len = 0;
> > +
> > + /* get buffer, starting from iov[1] */
> > + heads[0].iov_base = (void *)vhost_get_vq_desc(vq->dev, vq,
> > + vq->iov+1, ARRAY_SIZE(vq->iov)-1, &out, in, log,
log_num);
> > + if (out || *in <= 0) {
> > + vq_err(vq, "unexpected descriptor format for RX: out
%d, "
> > + "in %d\n", out, *in);
> > + return 0;
> > + }
> > + if (heads[0].iov_base == (void *)vq->num)
> > + return 0;
> > +
> > + /* make iov[0] the header */
> > + if (!vq->guest_hlen) {
> > + if (vq->sock_hlen) {
> > + static struct virtio_net_hdr junk; /* bit
bucket
> > */
> > +
> > + iov[0].iov_base = &junk;
> > + iov[0].iov_len = sizeof(junk);
> > + } else
> > + iov[0].iov_len = 0;
> > + }
> > + if (vq->sock_hlen < vq->guest_hlen) {
> > + iov[0].iov_base = iov[1].iov_base;
> > + iov[0].iov_len = vq->sock_hlen;
> > +
> > + if (iov[1].iov_len < vq->sock_hlen) {
> > + vq_err(vq, "can't fit header in one buffer!");
> > + vhost_discard(vq, 1);
> > + return 0;
> > + }
> > + if (!vq->sock_hlen) {
> > + static const struct virtio_net_hdr_mrg_rxbuf
hdr =
> > {
> > + .hdr.flags = 0,
> > + .hdr.gso_type =
VIRTIO_NET_HDR_GSO_NONE
> > + };
> > + memcpy(iov[0].iov_base, &hdr, vq->guest_hlen);
> > + }
> > + iov[1].iov_base += vq->guest_hlen;
> > + iov[1].iov_len -= vq->guest_hlen;
> > + }
> > + return 1;
>
> The above looks kind of scary, lots of special-casing. I'll send a
> patch for tap to set vnet header size, let's see if it makes life easier
> for us.
Yes, I try to handle all combinations of differing guest and
socket
header lengths (including 0-lengths). If we don't support raw sockets
and/or
we make the tap vnet header bigger in the MRXB case, it'll simplify this
code.
>
> > +}
> > +
> > unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
> > *iovcount,
> > struct vhost_log *log, unsigned int *log_num)
> > {
> > struct iovec *heads = vq->heads;
> > - int out, in;
> > + int out, in = 0;
> > + int seg = 0;
> > int hc = 0;
>
> I think it's better to stick to simple names like i
> for index variables, alternatively give it a meaningful
> name like "count".
I'm not sure which one you're talking about. "in & out"
are inherited from the original, "seg" is an iovec segment index, and
"hc" is a head counter. "headcount" or "count" (if that's the
one you mean) instead of "hc" would probably cause some line wraps.
I think "i" is too generic here, but if you mean "hc", I could stick
a comment on the declaration:
int hc = 0; /* head count */
Does that address your concern, or do you mean something else?
>
> >
> > + if (vq->guest_hlen != vq->sock_hlen) {
>
> Sticking guest_hlen/sock_hlen in vhost is somewhat ugly.
Do you mean in the struct? This essentially splits the
original "hdrsize" into the two (possibly different) header
sizes and allows removing the "hdr" iovec and reading the
(partial) header directly into the extended header mergeable
buffers needs. Unless we remove raw socket support and add the
ioctl to tap you suggested, I'm not sure it can be much cleaner.
For me, the ugliness comes from tap, raw and guest headers not
being the same.
+-DLS
^ permalink raw reply
* Re: [RFC][ PATCH 1/3] vhost-net: support multiple buffer heads in receiver
From: David Stevens @ 2010-03-08 0:34 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <20100307153130.GA24997@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote on 03/07/2010 07:31:30 AM:
> On Tue, Mar 02, 2010 at 05:20:15PM -0700, David Stevens wrote:
> > This patch generalizes buffer handling functions to
NULL, NULL);
> > + head.iov_base = (void *)vhost_get_vq_desc(&net->dev,
vq,
> > + vq->iov, ARRAY_SIZE(vq->iov), &out, &in, NULL,
> > NULL);
>
> Should type for head be changed so that we do not need to cast?
>
> I also prefer aligning descendants on the opening (.
Yes, that's probably better; the indentation with the cast would
still wrap a lot, but I'll see what I can do here.
> > err = sock->ops->sendmsg(NULL, sock, &msg, len);
> > if (unlikely(err < 0)) {
> > - vhost_discard_vq_desc(vq);
> > + vhost_discard(vq, 1);
>
> Isn't the original name a bit more descriptive?
During development, I had both and I generally like
shorter names, but I can change it back.
> > +static int skb_head_len(struct sk_buff_head *skq)
> > +{
> > + struct sk_buff *head;
> > +
> > + head = skb_peek(skq);
> > + if (head)
> > + return head->len;
> > + return 0;
> > +}
> > +
>
> This is done without locking, which I think can crash
> if skb is consumed after we peek it but before we read the
> length.
This thread is the only legitimate consumer, right? But
qemu has the file descriptor and I guess we shouldn't trust
that it won't give it to someone else; it'd break vhost, but
a crash would be inappropriate, of course. I'd like to avoid
the lock, but I have another idea here, so will investigate.
>
>
> > /* 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,
> > @@ -204,10 +213,11 @@
> > };
> >
> > 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))
> > +
> > + if (!sock || !skb_head_len(&sock->sk->sk_receive_queue))
> > return;
> >
>
> Isn't this equivalent? Do you expect zero len skbs in socket?
> If yes, maybe we should discard these, not stop processing ...
A zero return means "no skb's". They are equivalent; I
changed this call just to make it identical to the loop check,
but I don't have a strong attachment to this.
> > 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 = skb_head_len(&sock->sk->sk_receive_queue)))
{
>
> This peeks in the queue to figure out how much data we have.
> It's a neat trick, but it does introduce new failure modes
> where an skb is consumed after we did the peek:
> - new skb could be shorter, we should return the unconsumed part
> - new skb could be longer, this will drop a packet.
> maybe this last is not an issue as the race should be rare in practice
As before, this loop is the only legitimate consumer of skb's; if
anyone else is reading the socket, it's broken. But since the file
descriptor is available to qemu, it's probably trusting qemu too much.
I don't believe there is a race at all on a working system; the head
can't change until we read it after this test, but I'll see if I can
come up with something better here. Closing the fd for qemu so vhost
has exclusive access might do it, but otherwise we could just get a
max-sized packet worth of buffers and then return them after the read.
That'd force us to wait with small packets when the ring is nearly
full, where we don't have to now, but I expect locking to read the head
length will hurt performance in all cases. Will try these ideas out.k
>
> > + headcount = vhost_get_heads(vq, datalen, &in, vq_log,
> > &log);
> > /* OK, now we need to know about added descriptors. */
> > - if (head == vq->num) {
> > + if (!headcount) {
> > if (unlikely(vhost_enable_notify(vq))) {
> > /* They have slipped one in as we were
> > * doing that: check again. */
> > @@ -235,13 +242,6 @@
> > * they refilled. */
> > break;
> > }
> > - /* We don't need to be notified again. */
>
> you find this comment unhelpful?
This code is changed in the later patches; the comment was
removed with that, but I got it in the wrong patch on the split.
I guess the comment is ok to stay, anyway, but notification may
require multiple buffers to be available; I had fixed that here, but
the final patch pushes that into the notify code, so yes, this can
go back in.
> > - if (out) {
> > - vq_err(vq, "Unexpected descriptor format for
RX: "
> > - "out %d, int %d\n",
> > - out, in);
> > - break;
> > - }
>
>
> we still need this check, don't we?
It's done in vhost_get_heads(); "out" isn't visible in handle_rx()
anymore.
> > +unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
> > *iovcount,
> > + struct vhost_log *log, unsigned int *log_num)
>
> Could you please document this function's parameters? It's not obvious
> what iovcount, log, log_num are.
Yes. To answer the question, they are the same as vhost_get_vq_desc(),
except "iovcount" replaces "in" (and "out" is not needed in the caller).
> > +{
> > + struct iovec *heads = vq->heads;
> > + int out, in;
> > + int hc = 0;
> > +
> > + while (datalen > 0) {
> > + if (hc >= VHOST_NET_MAX_SG) {
> > + vhost_discard(vq, hc);
> > + return 0;
> > + }
> > + heads[hc].iov_base = (void
*)vhost_get_vq_desc(vq->dev,
> > vq,
> > + vq->iov, ARRAY_SIZE(vq->iov), &out, &in, log,
> > log_num);
> > + if (heads[hc].iov_base == (void *)vq->num) {
> > + vhost_discard(vq, hc);
> > + return 0;
> > + }
> > + if (out || in <= 0) {
> > + vq_err(vq, "unexpected descriptor format for
RX: "
> > + "out %d, in %d\n", out, in);
> > + vhost_discard(vq, hc);
> > + return 0;
> > + }
> > + heads[hc].iov_len = iov_length(vq->iov, in);
> > + hc++;
> > + datalen -= heads[hc].iov_len;
> > + }
> > + *iovcount = in;
>
>
> Only the last value?
In this part of the split, it only goes through once; this is
changed to the accumulated value "seg" in a later patch. So, split
artifact.
{
> > struct vring_used_elem *used;
> > + int i;
> >
> > - /* 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");
> > - return -EFAULT;
> > - }
> > - if (put_user(len, &used->len)) {
> > - vq_err(vq, "Failed to write used len");
> > - return -EFAULT;
> > + for (i=0; i<count; i++, vq->last_used_idx++) {
>
> whitespace damage: I prefer space around =, <.
> I also use ++i, etc in this driver, better be consistent?
> Also for clarity, I prefer to put vq->last_used_idx inside the loop.
OK.
>
> > + used = &vq->used->ring[vq->last_used_idx % vq->num];
> > + if (put_user((unsigned)heads[i].iov_base, &used->id))
{
> > + vq_err(vq, "Failed to write used id");
> > + return -EFAULT;
> > + }
> > + if (put_user(heads[i].iov_len, &used->len)) {
> > + vq_err(vq, "Failed to write used len");
> > + return -EFAULT;
> > + }
>
> If this fails, last_used_idx will still be incremented, which I think is
wrong.
True, but if these fail, aren't we dead? I don't think we can
recover
from an EFAULT in any of these; I didn't test those paths from the
original,
but I think we need to bail out entirely for these cases, right?
+-DLS
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).