* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Sjur Brændeland @ 2012-09-05 18:15 UTC (permalink / raw)
To: Michael S. Tsirkin, Rusty Russell, Ohad Ben-Cohen
Cc: Amit Shah, Linus Walleij, Sjur Brændeland, linux-kernel,
virtualization
In-Reply-To: <20120905143558.GB10925@redhat.com>
Hi Michael.
>> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
>> when DMA is not supported, virtio will do BUG_ON() from
>> virtio_check_driver_offered_feature().
>>
>> Is this acceptable or should we add a check in virtcons_probe()
>> and let the probing fail instead?
>>
>> E.g:
>> /* Refuse to bind if F_DMA_MEM request cannot be met */
>> if (!VIRTIO_CONSOLE_HAS_DMA &&
>> (vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
>> dev_err(&vdev->dev,
>> "DMA_MEM requested, but arch does not support DMA\n");
>> err = -EINVAL;
>> goto fail;
>> }
>>
>> Regards,
>> Sjur
>
> Failing probe would be cleaner. But there is still a problem:
> old driver will happily bind to that device and then
> fail to work, right?
Not just fail to work, the kernel will panic on the BUG_ON().
Remoteproc gets the virtio configuration from firmware loaded
from user space. So this type of problem might be triggered
for other virtio drivers as well.
> virtio pci has revision id for this, but remoteproc doesn't
> seem to have anything similar. Or did I miss it?
No there are currently no sanity check of
virtio type and feature bits in remoteproc.
One option may be to add this...
> If not -
> we probably need to use a different
> device id, and not a feature bit.
But if I create a new virtio console type, remoteproc
could still call the existing virtio_console with random
bad feature bits, causing kernel panic.
Even if we fix this particular problem, the general problem
still exists: bogus virtio declarations in remoteproc's firmware
may cause BUG_ON(). (Note the fundamental difference
between visualizations and remoteproc. For remoteproc
the virtio configuration comes from binaries loaded from
user space).
So maybe we should look for a more generic solution, e.g.
changing virtio probe functionality so that devices with
bad feature bits will not trigger BUG_ON(), but rather refuse
to bind the driver.
Regards,
Sjur
^ permalink raw reply
* [PATCH] virtio: support reserved vqs
From: Michael S. Tsirkin @ 2012-09-05 18:47 UTC (permalink / raw)
To: Rusty Russell, Ohad Ben-Cohen, Christian Borntraeger,
Cornelia Huck, linux390, Martin Schwidefsky, Heiko Carstens,
Michael S. Tsirkin, Paul Gortmaker, Stratos Psomadakis,
David S. Miller, lguest, linux-kernel, linux-s390, virtualization,
kvm
virtio network device multiqueue support reserves
vq 3 for future use (useful both for future extensions and to make it
pretty - this way receive vqs have even and transmit - odd numbers).
Make it possible to skip initialization for
specific vq numbers by specifying NULL for name.
Document this usage as well as (existing) NULL callback.
Drivers using this not coded up yet, so I simply tested
with virtio-pci and verified that this patch does
not break existing drivers.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/lguest/lguest_device.c | 3 +++
drivers/remoteproc/remoteproc_virtio.c | 3 +++
drivers/s390/kvm/kvm_virtio.c | 3 +++
drivers/virtio/virtio_mmio.c | 3 +++
drivers/virtio/virtio_pci.c | 5 ++++-
include/linux/virtio_config.h | 2 ++
6 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index 9e8388e..e849e12 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -263,6 +263,9 @@ static struct virtqueue *lg_find_vq(struct virtio_device *vdev,
struct virtqueue *vq;
int err;
+ if (!name)
+ return NULL;
+
/* We must have this many virtqueues. */
if (index >= ldev->desc->num_vq)
return ERR_PTR(-ENOENT);
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 3541b44..bda6750 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -84,6 +84,9 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
if (id >= ARRAY_SIZE(rvdev->vring))
return ERR_PTR(-EINVAL);
+ if (!name)
+ return NULL;
+
ret = rproc_alloc_vring(rvdev, id);
if (ret)
return ERR_PTR(ret);
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index 47cccd5..6e842d1 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -190,6 +190,9 @@ static struct virtqueue *kvm_find_vq(struct virtio_device *vdev,
if (index >= kdev->desc->num_vq)
return ERR_PTR(-ENOENT);
+ if (!name)
+ return NULL;
+
config = kvm_vq_config(kdev->desc)+index;
err = vmem_add_mapping(config->address,
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 453db0c..b1f342e 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -309,6 +309,9 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
unsigned long flags, size;
int err;
+ if (!name)
+ return NULL;
+
/* Select the queue we're interested in */
writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 2e03d41..92809ba 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -542,7 +542,10 @@ static int vp_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
vp_dev->per_vq_vectors = per_vq_vectors;
allocated_vectors = vp_dev->msix_used_vectors;
for (i = 0; i < nvqs; ++i) {
- if (!callbacks[i] || !vp_dev->msix_enabled)
+ if (!names[i]) {
+ vqs[i] = NULL;
+ continue;
+ } else if (!callbacks[i] || !vp_dev->msix_enabled)
msix_vec = VIRTIO_MSI_NO_VECTOR;
else if (vp_dev->per_vq_vectors)
msix_vec = allocated_vectors++;
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index fc457f4..07c0c69 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -84,7 +84,9 @@
* nvqs: the number of virtqueues to find
* vqs: on success, includes new virtqueues
* callbacks: array of callbacks, for each virtqueue
+ * include a NULL entry for vqs that do not need a callback
* names: array of virtqueue names (mainly for debugging)
+ * include a NULL entry for vqs unused by driver
* Returns 0 on success or error status
* @del_vqs: free virtqueues found by find_vqs().
* @get_features: get the array of feature bits for this device.
--
MST
^ permalink raw reply related
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Michael S. Tsirkin @ 2012-09-05 19:16 UTC (permalink / raw)
To: Sjur Brændeland
Cc: Linus Walleij, linux-kernel, virtualization, Amit Shah,
Sjur Brændeland
In-Reply-To: <CAJK669bhVfgnxuF6wVtdnzs2bgiJ2HzgwfWw4uZ=EqTdm43Jow@mail.gmail.com>
On Wed, Sep 05, 2012 at 08:15:36PM +0200, Sjur Brændeland wrote:
> Hi Michael.
>
> >> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
> >> when DMA is not supported, virtio will do BUG_ON() from
> >> virtio_check_driver_offered_feature().
> >>
> >> Is this acceptable or should we add a check in virtcons_probe()
> >> and let the probing fail instead?
> >>
> >> E.g:
> >> /* Refuse to bind if F_DMA_MEM request cannot be met */
> >> if (!VIRTIO_CONSOLE_HAS_DMA &&
> >> (vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
> >> dev_err(&vdev->dev,
> >> "DMA_MEM requested, but arch does not support DMA\n");
> >> err = -EINVAL;
> >> goto fail;
> >> }
> >>
> >> Regards,
> >> Sjur
> >
> > Failing probe would be cleaner. But there is still a problem:
> > old driver will happily bind to that device and then
> > fail to work, right?
>
> Not just fail to work, the kernel will panic on the BUG_ON().
> Remoteproc gets the virtio configuration from firmware loaded
> from user space. So this type of problem might be triggered
> for other virtio drivers as well.
how?
>
> > virtio pci has revision id for this, but remoteproc doesn't
> > seem to have anything similar. Or did I miss it?
>
> No there are currently no sanity check of
> virtio type and feature bits in remoteproc.
> One option may be to add this...
you can not fix the past.
> > If not -
> > we probably need to use a different
> > device id, and not a feature bit.
>
> But if I create a new virtio console type, remoteproc
> could still call the existing virtio_console with random
> bad feature bits, causing kernel panic.
cirtio core checks device id - this should not happen.
> Even if we fix this particular problem, the general problem
> still exists: bogus virtio declarations in remoteproc's firmware
> may cause BUG_ON().
which BUG_ON exactly?
> (Note the fundamental difference
> between visualizations and remoteproc. For remoteproc
> the virtio configuration comes from binaries loaded from
> user space).
>
> So maybe we should look for a more generic solution, e.g.
> changing virtio probe functionality so that devices with
> bad feature bits will not trigger BUG_ON(), but rather refuse
> to bind the driver.
>
> Regards,
> Sjur
^ permalink raw reply
* Re: [PATCH 2/5] virtio: introduce an API to set affinity for a virtqueue
From: Rusty Russell @ 2012-09-05 23:32 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel; +Cc: virtualization, kvm, linux-scsi, mst
In-Reply-To: <1346154857-12487-3-git-send-email-pbonzini@redhat.com>
Paolo Bonzini <pbonzini@redhat.com> writes:
> From: Jason Wang <jasowang@redhat.com>
>
> Sometimes, virtio device need to configure irq affinity hint to maximize the
> performance. Instead of just exposing the irq of a virtqueue, this patch
> introduce an API to set the affinity for a virtqueue.
>
> The api is best-effort, the affinity hint may not be set as expected due to
> platform support, irq sharing or irq type. Currently, only pci method were
> implemented and we set the affinity according to:
>
> - if device uses INTX, we just ignore the request
> - if device has per vq vector, we force the affinity hint
> - if the virtqueues share MSI, make the affinity OR over all affinities
> requested
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Applied, thanks.
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH 1/5] virtio-ring: move queue_index to vring_virtqueue
From: Rusty Russell @ 2012-09-05 23:32 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel; +Cc: virtualization, kvm, linux-scsi, mst
In-Reply-To: <1346154857-12487-2-git-send-email-pbonzini@redhat.com>
Paolo Bonzini <pbonzini@redhat.com> writes:
> From: Jason Wang <jasowang@redhat.com>
>
> Instead of storing the queue index in transport-specific virtio structs,
> this patch moves them to vring_virtqueue and introduces an helper to get
> the value. This lets drivers simplify their management and tracing of
> virtqueues.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Sorry for the delay, I was at Kernel Summit and am only now actually
reading (vs skimming) my backlog.
Putting it in vring_virtqueue rather than virtqueue seems weird, though.
But I've applied as-is, we can clean up that later if we want (probably
by merging the two structures, I'll have to think harder on that).
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Rusty Russell @ 2012-09-06 0:02 UTC (permalink / raw)
To: Sasha Levin, Michael S. Tsirkin; +Cc: avi, linux-kernel, kvm, virtualization
In-Reply-To: <503CC904.3050207@gmail.com>
Sasha Levin <levinsasha928@gmail.com> writes:
> On 08/28/2012 03:20 PM, Michael S. Tsirkin wrote:
>> On Tue, Aug 28, 2012 at 03:04:03PM +0200, Sasha Levin wrote:
>>> Currently if VIRTIO_RING_F_INDIRECT_DESC is enabled we will
>>> use indirect descriptors and allocate them using a simple
>>> kmalloc().
>>>
>>> This patch adds a cache which will allow indirect buffers under
>>> a configurable size to be allocated from that cache instead.
>>>
>>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
>>
>> I imagine this helps performance? Any numbers?
>
> I ran benchmarks on the original RFC, I've re-tested it now and got similar
> numbers to the original ones (virtio-net using vhost-net, thresh=16):
>
> Before:
> Recv Send Send
> Socket Socket Message Elapsed
> Size Size Size Time Throughput
> bytes bytes bytes secs. 10^6bits/sec
>
> 87380 16384 16384 10.00 4512.12
>
> After:
> Recv Send Send
> Socket Socket Message Elapsed
> Size Size Size Time Throughput
> bytes bytes bytes secs. 10^6bits/sec
>
> 87380 16384 16384 10.00 5399.18
I have an older patch which adjusts the threshold dynamically, can you
compare? User-adjustable thresholds are statistically never adjusted :(
virtio: use indirect buffers based on demand (heuristic)
virtio_ring uses a ring buffer of descriptors: indirect support allows
a single descriptor to refer to a table of descriptors. This saves
space in the ring, but requires a kmalloc/kfree.
Rather than try to figure out what the right threshold at which to use
indirect buffers, we drop the threshold dynamically when the ring is
under stress.
Note: to stress this, I reduced the ring size to 32 in lguest, and a
1G send reduced the threshold to 9.
Note2: I moved the BUG_ON()s above the indirect test, where they belong
(indirect falls thru on OOM, so the constraints still apply).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
drivers/virtio/virtio_ring.c | 61 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 52 insertions(+), 9 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -89,6 +89,8 @@ struct vring_virtqueue
/* Host supports indirect buffers */
bool indirect;
+ /* Threshold before we go indirect. */
+ unsigned int indirect_threshold;
/* Host publishes avail event idx */
bool event;
@@ -174,6 +176,34 @@ static int vring_add_indirect(struct vri
return head;
}
+static void adjust_threshold(struct vring_virtqueue *vq,
+ unsigned int out, unsigned int in)
+{
+ /* There are really two species of virtqueue, and it matters here.
+ * If there are no output parts, it's a "normally full" receive queue,
+ * otherwise it's a "normally empty" send queue. */
+ if (out) {
+ /* Leave threshold unless we're full. */
+ if (out + in < vq->num_free)
+ return;
+ } else {
+ /* Leave threshold unless we're empty. */
+ if (vq->num_free != vq->vring.num)
+ return;
+ }
+
+ /* Never drop threshold below 1 */
+ vq->indirect_threshold /= 2;
+ vq->indirect_threshold |= 1;
+
+#if 0
+ printk("%s %s: indirect threshold %u (%u+%u vs %u)\n",
+ dev_name(&vq->vq.vdev->dev),
+ vq->vq.name, vq->indirect_threshold,
+ out, in, vq->num_free);
+#endif
+}
+
int virtqueue_get_queue_index(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
@@ -226,17 +256,32 @@ int virtqueue_add_buf(struct virtqueue *
}
#endif
- /* If the host supports indirect descriptor tables, and we have multiple
- * buffers, then go indirect. FIXME: tune this threshold */
- if (vq->indirect && (out + in) > 1 && vq->num_free) {
- head = vring_add_indirect(vq, sg, out, in, gfp);
- if (likely(head >= 0))
- goto add_head;
- }
-
BUG_ON(out + in > vq->vring.num);
BUG_ON(out + in == 0);
+
+ /* If the host supports indirect descriptor tables, consider it. */
+ if (vq->indirect) {
+ bool try_indirect;
+
+ /* We tweak the threshold automatically. */
+ adjust_threshold(vq, out, in);
+
+ /* If we can't fit any at all, fall through. */
+ if (vq->num_free == 0)
+ try_indirect = false;
+ else if (out + in > vq->num_free)
+ try_indirect = true;
+ else
+ try_indirect = (out + in > vq->indirect_threshold);
+
+ if (try_indirect) {
+ head = vring_add_indirect(vq, sg, out, in);
+ if (head != vq->vring.num)
+ goto add_head;
+ }
+ }
+
if (vq->num_free < out + in) {
pr_debug("Can't add buf len %i - avail = %i\n",
out + in, vq->num_free);
@@ -666,6 +711,7 @@ struct virtqueue *vring_new_virtqueue(un
#endif
vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
+ vq->indirect_threshold = num;
vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
/* No callback? Tell other side not to bother us. */
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Rusty Russell @ 2012-09-06 1:02 UTC (permalink / raw)
To: Sasha Levin, Michael S. Tsirkin; +Cc: avi, linux-kernel, kvm, virtualization
In-Reply-To: <503E4873.6060607@gmail.com>
Sasha Levin <levinsasha928@gmail.com> writes:
>> On Wed, Aug 29, 2012 at 05:03:03PM +0200, Sasha Levin wrote:
>>> I've also re-ran it on a IBM server type host instead of my laptop. Here are the
>>> results:
>>>
>>> Vanilla kernel:
>>>
>>> MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.33.1
>>> () port 0 AF_INET
>>> enable_enobufs failed: getprotobyname
>>> Recv Send Send
>>> Socket Socket Message Elapsed
>>> Size Size Size Time Throughput
>>> bytes bytes bytes secs. 10^6bits/sec
>>>
>>> 87380 16384 16384 10.00 7922.72
>>>
>>> Patch 1, with threshold=16:
>>>
>>> MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.33.1
>>> () port 0 AF_INET
>>> enable_enobufs failed: getprotobyname
>>> Recv Send Send
>>> Socket Socket Message Elapsed
>>> Size Size Size Time Throughput
>>> bytes bytes bytes secs. 10^6bits/sec
>>>
>>> 87380 16384 16384 10.00 8415.07
>>>
>>> Patch 2:
>>>
>>> MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.33.1
>>> () port 0 AF_INET
>>> enable_enobufs failed: getprotobyname
>>> Recv Send Send
>>> Socket Socket Message Elapsed
>>> Size Size Size Time Throughput
>>> bytes bytes bytes secs. 10^6bits/sec
>>>
>>> 87380 16384 16384 10.00 8931.05
>>>
>>>
>>> Note that these are simple tests with netperf listening on one end and a simple
>>> 'netperf -H [host]' within the guest. If there are other tests which may be
>>> interesting please let me know.
It might be worth just unconditionally having a cache for the 2
descriptor case. This is what I get with qemu tap, though for some
reason the device features don't have guest or host CSUM, so my setup is
probably screwed:
Queue histogram for virtio0:
Size distribution for input (max=128427):
1: 128427 ################################################################
Size distribution for output (max=256485):
2: 256485 ################################################################
Size distribution for control (max=10):
3: 10 ################################################################
4: 5 ################################
Here's a patch, what do you get (run ifconfig to trigger the dump; yeah,
it's a hack!)
Hack: histogram of buffer sizes for virtio devices.
Currently triggered by a stats query (eg ifconfig) on a net device.
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -727,6 +727,8 @@ static struct rtnl_link_stats64 *virtnet
tot->rx_length_errors = dev->stats.rx_length_errors;
tot->rx_frame_errors = dev->stats.rx_frame_errors;
+ virtio_dev_dump_histogram(vi->vdev);
+
return tot;
}
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -108,6 +108,16 @@ void virtio_check_driver_offered_feature
}
EXPORT_SYMBOL_GPL(virtio_check_driver_offered_feature);
+void virtio_dev_dump_histogram(const struct virtio_device *vdev)
+{
+ const struct virtqueue *vq;
+
+ printk("Queue histogram for %s:\n", dev_name(&vdev->dev));
+ list_for_each_entry(vq, &vdev->vqs, list)
+ virtqueue_dump_histogram(vq);
+}
+EXPORT_SYMBOL_GPL(virtio_dev_dump_histogram);
+
static int virtio_dev_probe(struct device *_d)
{
int err, i;
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -120,6 +120,8 @@ struct vring_virtqueue
ktime_t last_add_time;
#endif
+ unsigned int *histo;
+
/* Tokens for callbacks. */
void *data[];
};
@@ -259,6 +261,8 @@ int virtqueue_add_buf(struct virtqueue *
BUG_ON(out + in > vq->vring.num);
BUG_ON(out + in == 0);
+ vq->histo[out+in]++;
+
/* If the host supports indirect descriptor tables, consider it. */
if (vq->indirect) {
bool try_indirect;
@@ -726,6 +730,7 @@ struct virtqueue *vring_new_virtqueue(un
}
vq->data[i] = NULL;
+ vq->histo = kzalloc(num * sizeof(vq->histo[0]), GFP_KERNEL);
return &vq->vq;
}
EXPORT_SYMBOL_GPL(vring_new_virtqueue);
@@ -772,4 +777,33 @@ unsigned int virtqueue_get_vring_size(st
}
EXPORT_SYMBOL_GPL(virtqueue_get_vring_size);
+void virtqueue_dump_histogram(const struct virtqueue *_vq)
+{
+ const struct vring_virtqueue *vq = to_vvq(_vq);
+ int i, j, start = 0, end = 0, max = 1;
+ char line[120];
+
+ for (i = 0; i < vq->vring.num; i++) {
+ if (!vq->histo[i])
+ continue;
+
+ end = i;
+ if (!vq->histo[start])
+ start = i;
+
+ if (vq->histo[i] > max)
+ max = vq->histo[i];
+ }
+
+ printk("Size distribution for %s (max=%u):\n", _vq->name, max);
+ for (i = start; i <= end; i++) {
+ unsigned int off;
+ off = sprintf(line, "%3u: %-7u ", i, vq->histo[i]);
+ for (j = 0; j < vq->histo[i] * 64 / max; j++)
+ line[off++] = '#';
+ line[off] = '\0';
+ printk("%s\n", line);
+ }
+}
+
MODULE_LICENSE("GPL");
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -52,6 +52,9 @@ unsigned int virtqueue_get_vring_size(st
int virtqueue_get_queue_index(struct virtqueue *vq);
+void virtio_dev_dump_histogram(const struct virtio_device *vdev);
+void virtqueue_dump_histogram(const struct virtqueue *vq);
+
/**
* virtio_device - representation of a device using virtio
* @index: unique position on the virtio bus
^ permalink raw reply
* Re: [PATCH] Add a page cache-backed balloon device driver.
From: Rusty Russell @ 2012-09-06 1:35 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Frank Swiderski, Andrea Arcangeli, Rik van Riel, Rafael Aquini,
kvm, Michael S. Tsirkin, linux-kernel, mikew, Ying Han,
virtualization
In-Reply-To: <50444FAF.7030304@redhat.com>
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 02/07/2012 02:29, Rusty Russell ha scritto:
>> VIRTIO_BALLOON_F_MUST_TELL_HOST
>> implies you should tell the host (eventually). I don't know if any
>> implementations actually care though.
>
> This is indeed broken, because it is a "negative" feature: it tells you
> that "implicit deflate" is _not_ supported.
>
> Right now, QEMU refuses migration if the target does not support all the
> features that were negotiated. But then:
>
> - a migration from non-MUST_TELL_HOST to MUST_TELL_HOST will succeed,
> which is wrong;
>
> - a migration from MUST_TELL_HOST to non-MUST_TELL_HOST will fail, which
> is useless.
>
>> We could add a VIRTIO_BALLOON_F_NEVER_TELL_DEFLATE which would mean the
>> deflate vq need not be used at all.
>
> That would work. At the same time we could deprecate MUST_TELL_HOST.
> Certainly the guest implementations don't care, or we would have
> experienced problems such as the one above. The QEMU implementation
> also does not care but, for example, a Xen implementation would care.
OK; I'm not sure we need to deprecate MUST_TELL_HOST, though since it's
never actually been used there's a good argument.
VIRTIO_BALLOON_F_SILENT_DEFLATE (or whatever it's called) would
obviously mean you couldn't ack VIRTIO_BALLOON_F_MUST_TELL_HOST.
Patches welcome!
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Rusty Russell @ 2012-09-06 1:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Kent Overstreet, linux-kernel, virtualization
In-Reply-To: <20120905061219.GC7517@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
>> Kent Overstreet <koverstreet@google.com> writes:
>>
>> > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
>> > instead.
>>
>> This is a slight mis-understanding. It's supposed to be selected by
>> the particular driver, probably virtio_pci in your case.
>>
>> Cheers,
>> Rusty.
>
> Actually balloon selects VIRTIO, I think it's a bug.
> Also isn't it time we dropped the experimental tag?
> Leaving it in for now.
Thanks, I applied this, and killed the EXPERIMENTAL in a separate patch.
Cheers,
Rusty.
^ permalink raw reply
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Rusty Russell @ 2012-09-06 2:04 UTC (permalink / raw)
To: Michael S. Tsirkin, Sjur Brændeland
Cc: Amit Shah, Linus Walleij, linux-kernel, virtualization
In-Reply-To: <20120904185541.GB3602@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote:
>> Hi Michael,
>>
>> > Exactly. Though if we just fail load it will be much less code.
>> >
>> > Generally, using a feature bit for this is a bit of a problem though:
>> > normally driver is expected to be able to simply ignore
>> > a feature bit. In this case driver is required to
>> > do something so a feature bit is not a good fit.
>> > I am not sure what the right thing to do is.
>>
>> I see - so in order to avoid the binding between driver and device
>> there are two options I guess. Either make virtio_dev_match() or
>> virtcons_probe() fail. Neither of them seems like the obvious choice.
>>
>> Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match
>> between device and driver in virtcons_probe() is the lesser evil?
>>
>> Regards,
>> Sjur
>
> A simplest thing to do is change dev id. rusty?
For generic usage, this is correct. But my opinion is that fallback on
feature non-ack is quality-of-implementation issue: great to have, but
there are cases where you just want to fail with "you're too old".
And in this case, an old system simply will never work. So it's a
question of how graceful the failure is.
Can your userspace loader can refuse to proceed if the driver doesn't
ack the bits? If so, it's simpler than a whole new ID.
Cheers,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] virtio: support reserved vqs
From: Rusty Russell @ 2012-09-06 2:41 UTC (permalink / raw)
To: Michael S. Tsirkin, Ohad Ben-Cohen, Christian Borntraeger,
Cornelia Huck, linux390, Martin Schwidefsky, Heiko Carstens
In-Reply-To: <20120905184745.GA15861@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> virtio network device multiqueue support reserves
> vq 3 for future use (useful both for future extensions and to make it
> pretty - this way receive vqs have even and transmit - odd numbers).
> Make it possible to skip initialization for
> specific vq numbers by specifying NULL for name.
> Document this usage as well as (existing) NULL callback.
>
> Drivers using this not coded up yet, so I simply tested
> with virtio-pci and verified that this patch does
> not break existing drivers.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
This seems sane. Applied.
Thanks,
Rusty.
^ permalink raw reply
* Re: [patch] virtio-blk: fix NULL checking in virtblk_alloc_req()
From: Rusty Russell @ 2012-09-06 2:55 UTC (permalink / raw)
To: Dan Carpenter, Asias He
Cc: virtualization, kernel-janitors, Michael S. Tsirkin
In-Reply-To: <20120905123252.GE6128@elgon.mountain>
Dan Carpenter <dan.carpenter@oracle.com> writes:
> Smatch complains about the inconsistent NULL checking here. Fix it to
> return NULL on failure.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This is only needed in linux-next.
Nice!
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 2edfb5c..457db0c 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -90,10 +90,11 @@ static inline struct virtblk_req *virtblk_alloc_req(struct virtio_blk *vblk,
> struct virtblk_req *vbr;
>
> vbr = mempool_alloc(vblk->pool, gfp_mask);
> - if (vbr && use_bio)
> - sg_init_table(vbr->sg, vblk->sg_elems);
> + if (!vbr)
> + return NULL;
>
> - vbr->vblk = vblk;
> + if (use_bio)
> + sg_init_table(vbr->sg, vblk->sg_elems);
>
> return vbr;
> }
But it turns out that "vbr->vblk = vblk;" assignment is important :)
Fixed and applied,
Rusty.
^ permalink raw reply
* Re: [patch] virtio-blk: fix NULL checking in virtblk_alloc_req()
From: Asias He @ 2012-09-06 3:02 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors, virtualization, Michael S. Tsirkin
In-Reply-To: <20120905123252.GE6128@elgon.mountain>
Hello Dan,
On 09/05/2012 08:32 PM, Dan Carpenter wrote:
> Smatch complains about the inconsistent NULL checking here. Fix it to
> return NULL on failure.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Thanks for catching this.
> ---
> This is only needed in linux-next.
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 2edfb5c..457db0c 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -90,10 +90,11 @@ static inline struct virtblk_req *virtblk_alloc_req(struct virtio_blk *vblk,
> struct virtblk_req *vbr;
>
> vbr = mempool_alloc(vblk->pool, gfp_mask);
> - if (vbr && use_bio)
> - sg_init_table(vbr->sg, vblk->sg_elems);
> + if (!vbr)
> + return NULL;
>
> - vbr->vblk = vblk;
The assignment of vbr->vblk is needed.
> + if (use_bio)
> + sg_init_table(vbr->sg, vblk->sg_elems);
>
> return vbr;
> }
--
Asias
^ permalink raw reply
* Re: [patch] virtio-blk: fix NULL checking in virtblk_alloc_req()
From: Asias He @ 2012-09-06 3:04 UTC (permalink / raw)
To: Rusty Russell
Cc: virtualization, kernel-janitors, Dan Carpenter,
Michael S. Tsirkin
In-Reply-To: <871uifj1vr.fsf@rustcorp.com.au>
On 09/06/2012 10:55 AM, Rusty Russell wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
>
>> Smatch complains about the inconsistent NULL checking here. Fix it to
>> return NULL on failure.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> ---
>> This is only needed in linux-next.
>
> Nice!
>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 2edfb5c..457db0c 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -90,10 +90,11 @@ static inline struct virtblk_req *virtblk_alloc_req(struct virtio_blk *vblk,
>> struct virtblk_req *vbr;
>>
>> vbr = mempool_alloc(vblk->pool, gfp_mask);
>> - if (vbr && use_bio)
>> - sg_init_table(vbr->sg, vblk->sg_elems);
>> + if (!vbr)
>> + return NULL;
>>
>> - vbr->vblk = vblk;
>> + if (use_bio)
>> + sg_init_table(vbr->sg, vblk->sg_elems);
>>
>> return vbr;
>> }
>
> But it turns out that "vbr->vblk = vblk;" assignment is important :)
I was just replying ;-)
> Fixed and applied,
Thanks Rusty!
--
Asias
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Michael S. Tsirkin @ 2012-09-06 5:02 UTC (permalink / raw)
To: Rusty Russell; +Cc: linux-kernel, avi, Sasha Levin, kvm, virtualization
In-Reply-To: <871uigj747.fsf@rustcorp.com.au>
On Thu, Sep 06, 2012 at 10:32:48AM +0930, Rusty Russell wrote:
> Sasha Levin <levinsasha928@gmail.com> writes:
> >> On Wed, Aug 29, 2012 at 05:03:03PM +0200, Sasha Levin wrote:
> >>> I've also re-ran it on a IBM server type host instead of my laptop. Here are the
> >>> results:
> >>>
> >>> Vanilla kernel:
> >>>
> >>> MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.33.1
> >>> () port 0 AF_INET
> >>> enable_enobufs failed: getprotobyname
> >>> Recv Send Send
> >>> Socket Socket Message Elapsed
> >>> Size Size Size Time Throughput
> >>> bytes bytes bytes secs. 10^6bits/sec
> >>>
> >>> 87380 16384 16384 10.00 7922.72
> >>>
> >>> Patch 1, with threshold=16:
> >>>
> >>> MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.33.1
> >>> () port 0 AF_INET
> >>> enable_enobufs failed: getprotobyname
> >>> Recv Send Send
> >>> Socket Socket Message Elapsed
> >>> Size Size Size Time Throughput
> >>> bytes bytes bytes secs. 10^6bits/sec
> >>>
> >>> 87380 16384 16384 10.00 8415.07
> >>>
> >>> Patch 2:
> >>>
> >>> MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.33.1
> >>> () port 0 AF_INET
> >>> enable_enobufs failed: getprotobyname
> >>> Recv Send Send
> >>> Socket Socket Message Elapsed
> >>> Size Size Size Time Throughput
> >>> bytes bytes bytes secs. 10^6bits/sec
> >>>
> >>> 87380 16384 16384 10.00 8931.05
> >>>
> >>>
> >>> Note that these are simple tests with netperf listening on one end and a simple
> >>> 'netperf -H [host]' within the guest. If there are other tests which may be
> >>> interesting please let me know.
>
> It might be worth just unconditionally having a cache for the 2
> descriptor case. This is what I get with qemu tap, though for some
> reason the device features don't have guest or host CSUM, so my setup is
> probably screwed:
Yes without checksum net core always linearizes packets, so yes it is
screwed.
For -net, skb always allocates space for 17 frags + linear part so
it seems sane to do same in virtio core, and allocate, for -net,
up to max_frags + 1 from cache.
We can adjust it: no _SG -> 2 otherwise 18.
Not sure about other drivers, maybe really use 2 there for now.
> Queue histogram for virtio0:
> Size distribution for input (max=128427):
> 1: 128427 ################################################################
> Size distribution for output (max=256485):
> 2: 256485 ################################################################
> Size distribution for control (max=10):
> 3: 10 ################################################################
> 4: 5 ################################
>
> Here's a patch, what do you get (run ifconfig to trigger the dump; yeah,
> it's a hack!)
>
> Hack: histogram of buffer sizes for virtio devices.
>
> Currently triggered by a stats query (eg ifconfig) on a net device.
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -727,6 +727,8 @@ static struct rtnl_link_stats64 *virtnet
> tot->rx_length_errors = dev->stats.rx_length_errors;
> tot->rx_frame_errors = dev->stats.rx_frame_errors;
>
> + virtio_dev_dump_histogram(vi->vdev);
> +
> return tot;
> }
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -108,6 +108,16 @@ void virtio_check_driver_offered_feature
> }
> EXPORT_SYMBOL_GPL(virtio_check_driver_offered_feature);
>
> +void virtio_dev_dump_histogram(const struct virtio_device *vdev)
> +{
> + const struct virtqueue *vq;
> +
> + printk("Queue histogram for %s:\n", dev_name(&vdev->dev));
> + list_for_each_entry(vq, &vdev->vqs, list)
> + virtqueue_dump_histogram(vq);
> +}
> +EXPORT_SYMBOL_GPL(virtio_dev_dump_histogram);
> +
> static int virtio_dev_probe(struct device *_d)
> {
> int err, i;
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -120,6 +120,8 @@ struct vring_virtqueue
> ktime_t last_add_time;
> #endif
>
> + unsigned int *histo;
> +
> /* Tokens for callbacks. */
> void *data[];
> };
> @@ -259,6 +261,8 @@ int virtqueue_add_buf(struct virtqueue *
> BUG_ON(out + in > vq->vring.num);
> BUG_ON(out + in == 0);
>
> + vq->histo[out+in]++;
> +
> /* If the host supports indirect descriptor tables, consider it. */
> if (vq->indirect) {
> bool try_indirect;
> @@ -726,6 +730,7 @@ struct virtqueue *vring_new_virtqueue(un
> }
> vq->data[i] = NULL;
>
> + vq->histo = kzalloc(num * sizeof(vq->histo[0]), GFP_KERNEL);
> return &vq->vq;
> }
> EXPORT_SYMBOL_GPL(vring_new_virtqueue);
> @@ -772,4 +777,33 @@ unsigned int virtqueue_get_vring_size(st
> }
> EXPORT_SYMBOL_GPL(virtqueue_get_vring_size);
>
> +void virtqueue_dump_histogram(const struct virtqueue *_vq)
> +{
> + const struct vring_virtqueue *vq = to_vvq(_vq);
> + int i, j, start = 0, end = 0, max = 1;
> + char line[120];
> +
> + for (i = 0; i < vq->vring.num; i++) {
> + if (!vq->histo[i])
> + continue;
> +
> + end = i;
> + if (!vq->histo[start])
> + start = i;
> +
> + if (vq->histo[i] > max)
> + max = vq->histo[i];
> + }
> +
> + printk("Size distribution for %s (max=%u):\n", _vq->name, max);
> + for (i = start; i <= end; i++) {
> + unsigned int off;
> + off = sprintf(line, "%3u: %-7u ", i, vq->histo[i]);
> + for (j = 0; j < vq->histo[i] * 64 / max; j++)
> + line[off++] = '#';
> + line[off] = '\0';
> + printk("%s\n", line);
> + }
> +}
> +
> MODULE_LICENSE("GPL");
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -52,6 +52,9 @@ unsigned int virtqueue_get_vring_size(st
>
> int virtqueue_get_queue_index(struct virtqueue *vq);
>
> +void virtio_dev_dump_histogram(const struct virtio_device *vdev);
> +void virtqueue_dump_histogram(const struct virtqueue *vq);
> +
> /**
> * virtio_device - representation of a device using virtio
> * @index: unique position on the virtio bus
^ permalink raw reply
* Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
From: Michael S. Tsirkin @ 2012-09-06 5:27 UTC (permalink / raw)
To: Rusty Russell
Cc: Sjur Brændeland, Linus Walleij, linux-kernel, virtualization,
Amit Shah
In-Reply-To: <87k3w7j49i.fsf@rustcorp.com.au>
On Thu, Sep 06, 2012 at 11:34:25AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote:
> >> Hi Michael,
> >>
> >> > Exactly. Though if we just fail load it will be much less code.
> >> >
> >> > Generally, using a feature bit for this is a bit of a problem though:
> >> > normally driver is expected to be able to simply ignore
> >> > a feature bit. In this case driver is required to
> >> > do something so a feature bit is not a good fit.
> >> > I am not sure what the right thing to do is.
> >>
> >> I see - so in order to avoid the binding between driver and device
> >> there are two options I guess. Either make virtio_dev_match() or
> >> virtcons_probe() fail. Neither of them seems like the obvious choice.
> >>
> >> Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match
> >> between device and driver in virtcons_probe() is the lesser evil?
> >>
> >> Regards,
> >> Sjur
> >
> > A simplest thing to do is change dev id. rusty?
>
> For generic usage, this is correct. But my opinion is that fallback on
> feature non-ack is quality-of-implementation issue: great to have, but
> there are cases where you just want to fail with "you're too old".
>
> And in this case, an old system simply will never work. So it's a
> question of how graceful the failure is.
>
> Can your userspace loader can refuse to proceed if the driver doesn't
> ack the bits? If so, it's simpler than a whole new ID.
>
> Cheers,
> Rusty.
Yes but how can it signal guest that it will never proceed?
Also grep for BUG_ON in core found this:
drv->remove(dev);
/* Driver should have reset device. */
BUG_ON(dev->config->get_status(dev));
I think below is what Sjur refers to.
I think below is a good idea for 3.6. Thoughts?
--->
virtio: don't crash when device is buggy
Because of a sanity check in virtio_dev_remove, a buggy device can crash
kernel. And in case of rproc it's userspace so it's not a good idea.
We are unloading a driver so how bad can it be?
Be less aggressive in handling this error: if it's a driver bug,
warning once should be enough.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
--
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index c3b3f7f..1e8659c 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -159,7 +159,7 @@ static int virtio_dev_remove(struct device *_d)
drv->remove(dev);
/* Driver should have reset device. */
- BUG_ON(dev->config->get_status(dev));
+ WARN_ON_ONCE(dev->config->get_status(dev));
/* Acknowledge the device's existence again. */
add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
--
MST
^ permalink raw reply related
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Kent Overstreet @ 2012-09-06 7:41 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization, linux-kernel, mst
In-Reply-To: <87wr0ae26e.fsf@rustcorp.com.au>
On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> Kent Overstreet <koverstreet@google.com> writes:
>
> > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > instead.
>
> This is a slight mis-understanding. It's supposed to be selected by
> the particular driver, probably virtio_pci in your case.
So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
should have that.
As is, VIRTIO_BLK just has:
depends on EXPERIMENTAL && VIRTIO
which is flat out broken.
^ permalink raw reply
* [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Paolo Bonzini @ 2012-09-06 7:46 UTC (permalink / raw)
To: rusty
Cc: fes, aarcange, riel, kvm, mst, linux-kernel, mikew, yinghan,
virtualization
VIRTIO_BALLOON_F_MUST_TELL_HOST cannot be used properly because it is a
"negative" feature: it tells you that silent defalte is not supported.
Right now, QEMU refuses migration if the target does not support all the
features that were negotiated. But then:
- a migration from non-MUST_TELL_HOST to MUST_TELL_HOST will succeed,
which is wrong;
- a migration from MUST_TELL_HOST to non-MUST_TELL_HOST will fail, which
is useless.
Add instead a new feature VIRTIO_BALLOON_F_SILENT_DEFLATE, and deprecate
VIRTIO_BALLOON_F_MUST_TELL_HOST since it is never actually used.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
virtio-spec.lyx | 36 +++++++++++++++++++++++++++++++++---
1 file modificato, 33 inserzioni(+), 3 rimozioni(-)
diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 7a073f4..1a25a18 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -6238,6 +6238,8 @@ bits
\begin_deeper
\begin_layout Description
+
+\change_deleted 1531152142 1346917221
VIRTIO_BALLOON_F_MUST_TELL_HOST
\begin_inset space ~
\end_inset
@@ -6251,6 +6253,20 @@ VIRTIO_BALLOON_F_STATS_VQ
\end_inset
(1) A virtqueue for reporting guest memory statistics is present.
+\change_inserted 1531152142 1346917193
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 1531152142 1346917219
+VIRTIO_BALLOON_F_SILENT_DEFLATE
+\begin_inset space ~
+\end_inset
+
+(2) Host does not need to be told before pages from the balloon are used.
+\change_unchanged
+
\end_layout
\end_deeper
@@ -6401,9 +6417,23 @@ The driver constructs an array of addresses of memory pages it has previously
\end_layout
\begin_layout Enumerate
-If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set, the guest may not
- use these requested pages until that descriptor in the deflateq has been
- used by the device.
+If the VIRTIO_BALLOON_F_
+\change_deleted 1531152142 1346917234
+MUST_TELL_HOST
+\change_inserted 1531152142 1346917237
+SILENT_DEFLATE
+\change_unchanged
+ feature is
+\change_inserted 1531152142 1346917241
+not
+\change_unchanged
+set, the guest may not use these requested pages until that descriptor in
+ the deflateq has been used by the device.
+
+\change_inserted 1531152142 1346917253
+ If it is set, the guest may choose to not use the deflateq at all.
+\change_unchanged
+
\end_layout
\begin_layout Enumerate
--
1.7.11.2
^ permalink raw reply related
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Rusty Russell @ 2012-09-06 7:57 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, avi, Sasha Levin, kvm, virtualization
In-Reply-To: <20120906050257.GA17656@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> Yes without checksum net core always linearizes packets, so yes it is
> screwed.
> For -net, skb always allocates space for 17 frags + linear part so
> it seems sane to do same in virtio core, and allocate, for -net,
> up to max_frags + 1 from cache.
> We can adjust it: no _SG -> 2 otherwise 18.
But I thought it used individual buffers these days?
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Michael S. Tsirkin @ 2012-09-06 8:44 UTC (permalink / raw)
To: Kent Overstreet; +Cc: linux-kernel, virtualization
In-Reply-To: <20120906074113.GA5902@koverstreet-glaptop>
On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > Kent Overstreet <koverstreet@google.com> writes:
> >
> > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > instead.
> >
> > This is a slight mis-understanding. It's supposed to be selected by
> > the particular driver, probably virtio_pci in your case.
>
> So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> should have that.
>
> As is, VIRTIO_BLK just has:
> depends on EXPERIMENTAL && VIRTIO
>
> which is flat out broken.
I don't think anything is broken.
Can you show an example of a broken configuration?
--
MST
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Michael S. Tsirkin @ 2012-09-06 8:45 UTC (permalink / raw)
To: Rusty Russell; +Cc: linux-kernel, avi, Sasha Levin, kvm, virtualization
In-Reply-To: <877gs7inx8.fsf@rustcorp.com.au>
On Thu, Sep 06, 2012 at 05:27:23PM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > Yes without checksum net core always linearizes packets, so yes it is
> > screwed.
> > For -net, skb always allocates space for 17 frags + linear part so
> > it seems sane to do same in virtio core, and allocate, for -net,
> > up to max_frags + 1 from cache.
> > We can adjust it: no _SG -> 2 otherwise 18.
>
> But I thought it used individual buffers these days?
Yes for receive, no for transmit. That's probably why
we should have the threshold per vq, not per device, BTW.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Michael S. Tsirkin @ 2012-09-06 8:47 UTC (permalink / raw)
To: Paolo Bonzini
Cc: fes, aarcange, riel, kvm, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <1346917610-14568-1-git-send-email-pbonzini@redhat.com>
On Thu, Sep 06, 2012 at 09:46:50AM +0200, Paolo Bonzini wrote:
> VIRTIO_BALLOON_F_MUST_TELL_HOST cannot be used properly because it is a
> "negative" feature: it tells you that silent defalte is not supported.
> Right now, QEMU refuses migration if the target does not support all the
> features that were negotiated. But then:
>
> - a migration from non-MUST_TELL_HOST to MUST_TELL_HOST will succeed,
> which is wrong;
>
> - a migration from MUST_TELL_HOST to non-MUST_TELL_HOST will fail, which
> is useless.
>
> Add instead a new feature VIRTIO_BALLOON_F_SILENT_DEFLATE, and deprecate
> VIRTIO_BALLOON_F_MUST_TELL_HOST since it is never actually used.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Frankly I think it's a qemu migration bug. I don't see why
we need to tweak spec: just teach qemu to be smarter
during migration.
Can you show a scenario with old driver/new hypervisor or
new driver/old hypervisor that fails?
> ---
> virtio-spec.lyx | 36 +++++++++++++++++++++++++++++++++---
> 1 file modificato, 33 inserzioni(+), 3 rimozioni(-)
>
> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> index 7a073f4..1a25a18 100644
> --- a/virtio-spec.lyx
> +++ b/virtio-spec.lyx
> @@ -6238,6 +6238,8 @@ bits
>
> \begin_deeper
> \begin_layout Description
> +
> +\change_deleted 1531152142 1346917221
> VIRTIO_BALLOON_F_MUST_TELL_HOST
> \begin_inset space ~
> \end_inset
> @@ -6251,6 +6253,20 @@ VIRTIO_BALLOON_F_STATS_VQ
> \end_inset
>
> (1) A virtqueue for reporting guest memory statistics is present.
> +\change_inserted 1531152142 1346917193
> +
> +\end_layout
> +
> +\begin_layout Description
> +
> +\change_inserted 1531152142 1346917219
> +VIRTIO_BALLOON_F_SILENT_DEFLATE
> +\begin_inset space ~
> +\end_inset
> +
> +(2) Host does not need to be told before pages from the balloon are used.
> +\change_unchanged
> +
> \end_layout
>
> \end_deeper
> @@ -6401,9 +6417,23 @@ The driver constructs an array of addresses of memory pages it has previously
> \end_layout
>
> \begin_layout Enumerate
> -If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set, the guest may not
> - use these requested pages until that descriptor in the deflateq has been
> - used by the device.
> +If the VIRTIO_BALLOON_F_
> +\change_deleted 1531152142 1346917234
> +MUST_TELL_HOST
> +\change_inserted 1531152142 1346917237
> +SILENT_DEFLATE
> +\change_unchanged
> + feature is
> +\change_inserted 1531152142 1346917241
> +not
> +\change_unchanged
> +set, the guest may not use these requested pages until that descriptor in
> + the deflateq has been used by the device.
> +
> +\change_inserted 1531152142 1346917253
> + If it is set, the guest may choose to not use the deflateq at all.
> +\change_unchanged
> +
> \end_layout
>
> \begin_layout Enumerate
> --
> 1.7.11.2
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Paolo Bonzini @ 2012-09-06 9:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: fes, aarcange, riel, kvm, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <20120906084736.GF17656@redhat.com>
Il 06/09/2012 10:47, Michael S. Tsirkin ha scritto:
>> > - a migration from non-MUST_TELL_HOST to MUST_TELL_HOST will succeed,
>> > which is wrong;
>> >
>> > - a migration from MUST_TELL_HOST to non-MUST_TELL_HOST will fail, which
>> > is useless.
>> >
>> > Add instead a new feature VIRTIO_BALLOON_F_SILENT_DEFLATE, and deprecate
>> > VIRTIO_BALLOON_F_MUST_TELL_HOST since it is never actually used.
>> >
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Frankly I think it's a qemu migration bug. I don't see why
> we need to tweak spec: just teach qemu to be smarter
> during migration.
Of course you can just teach QEMU to be smarter, but that would be a
one-off hack for the only ill-defined feature that says something is
_not_ supported. Since in practice all virtio_balloon-enbled
hypervisors supported silent deflate (so the bit was always zero), and
no client used it (so it was never checked), it's easier to just reverse
the direction.
In fact, it's not clear how the driver should use the feature. My guess
is that, if it wants to use silent deflate, it tries to negotiate
VIRTIO_BALLOON_F_MUST_TELL_HOST, and can use silent deflate if
negotiation fails. This is against the logic of all other features.
> Can you show a scenario with old driver/new hypervisor or
> new driver/old hypervisor that fails?
Suppose the driver tried to negotiate the feature as above. We then
have the two scenarios above.
In the harmless but annoying scenario, the source hypervisor doesn't
support silent deflate, so VIRTIO_BALLOON_F_MUST_TELL_HOST has been
negotiated successfully. The destination hypervisor supports silent
deflate, so it does _not_ include the feature. It sees that the guest
requests VIRTIO_BALLOON_F_MUST_TELL_HOST, and fails migration.
In the incorrect scenario, you are migrating to an older hypervisor.
The source hypervisor is newer and supports silent deflate, so
VIRTIO_BALLOON_F_MUST_TELL_HOST was _not_ negotiated. The destination
hypervisor does not supports silent deflate. However, the guest is not
requesting VIRTIO_BALLOON_F_MUST_TELL_HOST, and migration succeeds.
Next time the guest tries to do use a page from the balloon, badness
happens, because the hypervisor does not expect it.
Paolo
^ permalink raw reply
* Re: [PATCH] virtio-blk: Fix kconfig option
From: Kent Overstreet @ 2012-09-06 9:25 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, virtualization
In-Reply-To: <20120906084403.GD17656@redhat.com>
On Thu, Sep 06, 2012 at 11:44:03AM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> > On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > > Kent Overstreet <koverstreet@google.com> writes:
> > >
> > > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > > instead.
> > >
> > > This is a slight mis-understanding. It's supposed to be selected by
> > > the particular driver, probably virtio_pci in your case.
> >
> > So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> > should have that.
> >
> > As is, VIRTIO_BLK just has:
> > depends on EXPERIMENTAL && VIRTIO
> >
> > which is flat out broken.
>
> I don't think anything is broken.
> Can you show an example of a broken configuration?
Do you not understand the difference between depends an selects? Or did
you not read my original mail?
Flip off everything in drivers -> virtio
Now go to drivers -> block and try to turn on virtio-blk.
It's not listed!
Now go back to drivers -> virtio and turn on (randomly) balloon.
Go back to drivers -> block, and now you can turn on virtio-blk!
Do you see what's wrong with this picture?
^ permalink raw reply
* Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works
From: Michael S. Tsirkin @ 2012-09-06 9:44 UTC (permalink / raw)
To: Paolo Bonzini
Cc: fes, aarcange, riel, kvm, linux-kernel, mikew, yinghan,
virtualization
In-Reply-To: <50486BB2.7070108@redhat.com>
On Thu, Sep 06, 2012 at 11:24:02AM +0200, Paolo Bonzini wrote:
> Il 06/09/2012 10:47, Michael S. Tsirkin ha scritto:
> >> > - a migration from non-MUST_TELL_HOST to MUST_TELL_HOST will succeed,
> >> > which is wrong;
> >> >
> >> > - a migration from MUST_TELL_HOST to non-MUST_TELL_HOST will fail, which
> >> > is useless.
> >> >
> >> > Add instead a new feature VIRTIO_BALLOON_F_SILENT_DEFLATE, and deprecate
> >> > VIRTIO_BALLOON_F_MUST_TELL_HOST since it is never actually used.
> >> >
> >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > Frankly I think it's a qemu migration bug. I don't see why
> > we need to tweak spec: just teach qemu to be smarter
> > during migration.
>
> Of course you can just teach QEMU to be smarter, but that would be a
> one-off hack for the only ill-defined feature that says something is
> _not_ supported. Since in practice all virtio_balloon-enbled
> hypervisors supported silent deflate (so the bit was always zero), and
> no client used it (so it was never checked), it's easier to just reverse
> the direction.
>
> In fact, it's not clear how the driver should use the feature. My guess
> is that, if it wants to use silent deflate, it tries to negotiate
> VIRTIO_BALLOON_F_MUST_TELL_HOST, and can use silent deflate if
> negotiation fails. This is against the logic of all other features.
Let's take a step back from the implementation details.
You are trying to add a new feature bit, after all.
Why? Why is silent deflate useful? This is what is
missing in all this discussion. If it is not useful
we do not need a bit for it.
> > Can you show a scenario with old driver/new hypervisor or
> > new driver/old hypervisor that fails?
>
> Suppose the driver tried to negotiate the feature as above. We then
> have the two scenarios above.
>
> In the harmless but annoying scenario, the source hypervisor doesn't
> support silent deflate, so VIRTIO_BALLOON_F_MUST_TELL_HOST has been
> negotiated successfully. The destination hypervisor supports silent
> deflate, so it does _not_ include the feature. It sees that the guest
> requests VIRTIO_BALLOON_F_MUST_TELL_HOST, and fails migration.
>
> In the incorrect scenario, you are migrating to an older hypervisor.
> The source hypervisor is newer and supports silent deflate, so
> VIRTIO_BALLOON_F_MUST_TELL_HOST was _not_ negotiated. The destination
> hypervisor does not supports silent deflate. However, the guest is not
> requesting VIRTIO_BALLOON_F_MUST_TELL_HOST, and migration succeeds.
> Next time the guest tries to do use a page from the balloon, badness
> happens, because the hypervisor does not expect it.
>
> Paolo
Sorry this is not the example I asked for. Please give and example
without migration.
Migration is qemu's problem: it is hypervisor's job to
make sure guest sees no change during migration.
It should be able to do this with any hardware it emulates,
there should be no need to change hardware to make it
"migrateable" somehow.
--
MST
^ 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