* [PATCH 0/5] Multiqueue virtio-scsi
From: Paolo Bonzini @ 2012-08-28 11:54 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-scsi, kvm, mst, virtualization
Hi all,
this series adds multiqueue support to the virtio-scsi driver, based
on Jason Wang's work on virtio-net. It uses a simple queue steering
algorithm that expects one queue per CPU. LUNs in the same target always
use the same queue (so that commands are not reordered); queue switching
occurs when the request being queued is the only one for the target.
Also based on Jason's patches, the virtqueue affinity is set so that
each CPU is associated to one virtqueue.
I tested the patches with fio, using up to 32 virtio-scsi disks backed
by tmpfs on the host, and 1 LUN per target.
FIO configuration
-----------------
[global]
rw=read
bsrange=4k-64k
ioengine=libaio
direct=1
iodepth=4
loops=20
overall bandwidth (MB/s)
-----------------
# of targets single-queue multi-queue, 4 VCPUs multi-queue, 8 VCPUs
1 540 626 599
2 795 965 925
4 997 1376 1500
8 1136 2130 2060
16 1440 2269 2474
24 1408 2179 2436
32 1515 1978 2319
(These numbers for single-queue are with 4 VCPUs, but the impact of adding
more VCPUs is very limited).
avg bandwidth per LUN (MB/s)
---------------------
# of targets single-queue multi-queue, 4 VCPUs multi-queue, 8 VCPUs
1 540 626 599
2 397 482 462
4 249 344 375
8 142 266 257
16 90 141 154
24 58 90 101
32 47 61 72
Testing this may require an irqbalance daemon that is built from git,
due to http://code.google.com/p/irqbalance/issues/detail?id=37.
Alternatively you can just set the affinity manually in /proc.
Rusty, can you please give your Acked-by to the first two patches?
Jason Wang (2):
virtio-ring: move queue_index to vring_virtqueue
virtio: introduce an API to set affinity for a virtqueue
Paolo Bonzini (3):
virtio-scsi: allocate target pointers in a separate memory block
virtio-scsi: pass struct virtio_scsi to virtqueue completion function
virtio-scsi: introduce multiqueue support
drivers/lguest/lguest_device.c | 1 +
drivers/remoteproc/remoteproc_virtio.c | 1 +
drivers/s390/kvm/kvm_virtio.c | 1 +
drivers/scsi/virtio_scsi.c | 200 ++++++++++++++++++++++++--------
drivers/virtio/virtio_mmio.c | 11 +-
drivers/virtio/virtio_pci.c | 58 ++++++++-
drivers/virtio/virtio_ring.c | 17 +++
include/linux/virtio.h | 4 +
include/linux/virtio_config.h | 21 ++++
9 files changed, 253 insertions(+), 61 deletions(-)
^ permalink raw reply
* [PATCH 1/5] virtio-ring: move queue_index to vring_virtqueue
From: Paolo Bonzini @ 2012-08-28 11:54 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-scsi, kvm, mst, virtualization
In-Reply-To: <1346154857-12487-1-git-send-email-pbonzini@redhat.com>
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>
---
I fixed the problems in Jason's v5 (posted at
http://permalink.gmane.org/gmane.linux.kernel.virtualization/15910)
and switched from virtio_set_queue_index to a new argument of
vring_new_virtqueue. This breaks at compile-time any virtio
transport that is not updated.
drivers/lguest/lguest_device.c | 2 +-
drivers/remoteproc/remoteproc_virtio.c | 2 +-
drivers/s390/kvm/kvm_virtio.c | 2 +-
drivers/virtio/virtio_mmio.c | 12 ++++--------
drivers/virtio/virtio_pci.c | 13 +++++--------
drivers/virtio/virtio_ring.c | 14 +++++++++++++-
include/linux/virtio.h | 2 ++
include/linux/virtio_ring.h | 3 ++-
8 files changed, 29 insertions(+), 21 deletions(-)
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index 9e8388e..ccb7dfb 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -296,7 +296,7 @@ static struct virtqueue *lg_find_vq(struct virtio_device *vdev,
* to 'true': the host just a(nother) SMP CPU, so we only need inter-cpu
* barriers.
*/
- vq = vring_new_virtqueue(lvq->config.num, LGUEST_VRING_ALIGN, vdev,
+ vq = vring_new_virtqueue(index, lvq->config.num, LGUEST_VRING_ALIGN, vdev,
true, lvq->pages, lg_notify, callback, name);
if (!vq) {
err = -ENOMEM;
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 3541b44..343c194 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -103,7 +103,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
* Create the new vq, and tell virtio we're not interested in
* the 'weak' smp barriers, since we're talking with a real device.
*/
- vq = vring_new_virtqueue(len, rvring->align, vdev, false, addr,
+ vq = vring_new_virtqueue(id, len, rvring->align, vdev, false, addr,
rproc_virtio_notify, callback, name);
if (!vq) {
dev_err(dev, "vring_new_virtqueue %s failed\n", name);
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index 47cccd5..5565af2 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -198,7 +198,7 @@ static struct virtqueue *kvm_find_vq(struct virtio_device *vdev,
if (err)
goto out;
- vq = vring_new_virtqueue(config->num, KVM_S390_VIRTIO_RING_ALIGN,
+ vq = vring_new_virtqueue(index, config->num, KVM_S390_VIRTIO_RING_ALIGN,
vdev, true, (void *) config->address,
kvm_notify, callback, name);
if (!vq) {
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 453db0c..008bf58 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -131,9 +131,6 @@ struct virtio_mmio_vq_info {
/* the number of entries in the queue */
unsigned int num;
- /* the index of the queue */
- int queue_index;
-
/* the virtual address of the ring queue */
void *queue;
@@ -225,11 +222,10 @@ static void vm_reset(struct virtio_device *vdev)
static void vm_notify(struct virtqueue *vq)
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
- struct virtio_mmio_vq_info *info = vq->priv;
/* We write the queue's selector into the notification register to
* signal the other end */
- writel(info->queue_index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
+ writel(virtqueue_get_queue_index(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
}
/* Notify all virtqueues on an interrupt. */
@@ -270,6 +266,7 @@ static void vm_del_vq(struct virtqueue *vq)
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
struct virtio_mmio_vq_info *info = vq->priv;
unsigned long flags, size;
+ unsigned int index = virtqueue_get_queue_index(vq);
spin_lock_irqsave(&vm_dev->lock, flags);
list_del(&info->node);
@@ -278,7 +275,7 @@ static void vm_del_vq(struct virtqueue *vq)
vring_del_virtqueue(vq);
/* Select and deactivate the queue */
- writel(info->queue_index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
+ writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
size = PAGE_ALIGN(vring_size(info->num, VIRTIO_MMIO_VRING_ALIGN));
@@ -324,7 +321,6 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
err = -ENOMEM;
goto error_kmalloc;
}
- info->queue_index = index;
/* Allocate pages for the queue - start with a queue as big as
* possible (limited by maximum size allowed by device), drop down
@@ -356,7 +352,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
/* Create the vring */
- vq = vring_new_virtqueue(info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
+ vq = vring_new_virtqueue(index, info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
true, info->queue, vm_notify, callback, name);
if (!vq) {
err = -ENOMEM;
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 2e03d41..d902464 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -79,9 +79,6 @@ struct virtio_pci_vq_info
/* the number of entries in the queue */
int num;
- /* the index of the queue */
- int queue_index;
-
/* the virtual address of the ring queue */
void *queue;
@@ -202,11 +199,11 @@ static void vp_reset(struct virtio_device *vdev)
static void vp_notify(struct virtqueue *vq)
{
struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
- struct virtio_pci_vq_info *info = vq->priv;
/* we write the queue's selector into the notification register to
* signal the other end */
- iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
+ iowrite16(virtqueue_get_queue_index(vq),
+ vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
}
/* Handle a configuration change: Tell driver if it wants to know. */
@@ -402,7 +399,6 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
if (!info)
return ERR_PTR(-ENOMEM);
- info->queue_index = index;
info->num = num;
info->msix_vector = msix_vec;
@@ -418,7 +414,7 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
/* create the vring */
- vq = vring_new_virtqueue(info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
+ vq = vring_new_virtqueue(index, info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
true, info->queue, vp_notify, callback, name);
if (!vq) {
err = -ENOMEM;
@@ -467,7 +463,8 @@ static void vp_del_vq(struct virtqueue *vq)
list_del(&info->node);
spin_unlock_irqrestore(&vp_dev->lock, flags);
- iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
+ iowrite16(virtqueue_get_queue_index(vq),
+ vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
if (vp_dev->msix_enabled) {
iowrite16(VIRTIO_MSI_NO_VECTOR,
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5aa43c3..e639584 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -106,6 +106,9 @@ struct vring_virtqueue
/* How to notify other side. FIXME: commonalize hcalls! */
void (*notify)(struct virtqueue *vq);
+ /* Index of the queue */
+ int queue_index;
+
#ifdef DEBUG
/* They're supposed to lock for us. */
unsigned int in_use;
@@ -171,6 +174,13 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
return head;
}
+int virtqueue_get_queue_index(struct virtqueue *_vq)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ return vq->queue_index;
+}
+EXPORT_SYMBOL_GPL(virtqueue_get_queue_index);
+
/**
* virtqueue_add_buf - expose buffer to other end
* @vq: the struct virtqueue we're talking about.
@@ -616,7 +626,8 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
}
EXPORT_SYMBOL_GPL(vring_interrupt);
-struct virtqueue *vring_new_virtqueue(unsigned int num,
+struct virtqueue *vring_new_virtqueue(unsigned int index,
+ unsigned int num,
unsigned int vring_align,
struct virtio_device *vdev,
bool weak_barriers,
@@ -647,6 +658,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
vq->broken = false;
vq->last_used_idx = 0;
vq->num_added = 0;
+ vq->queue_index = index;
list_add_tail(&vq->vq.list, &vdev->vqs);
#ifdef DEBUG
vq->in_use = false;
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index a1ba8bb..533b115 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -50,6 +50,8 @@ void *virtqueue_detach_unused_buf(struct virtqueue *vq);
unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
+int virtqueue_get_queue_index(struct virtqueue *vq);
+
/**
* virtio_device - representation of a device using virtio
* @index: unique position on the virtio bus
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index e338730..c2d793a 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -165,7 +165,8 @@ static inline int vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old)
struct virtio_device;
struct virtqueue;
-struct virtqueue *vring_new_virtqueue(unsigned int num,
+struct virtqueue *vring_new_virtqueue(unsigned int index,
+ unsigned int num,
unsigned int vring_align,
struct virtio_device *vdev,
bool weak_barriers,
--
1.7.1
^ permalink raw reply related
* [PATCH 2/5] virtio: introduce an API to set affinity for a virtqueue
From: Paolo Bonzini @ 2012-08-28 11:54 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-scsi, kvm, mst, virtualization
In-Reply-To: <1346154857-12487-1-git-send-email-pbonzini@redhat.com>
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>
---
drivers/virtio/virtio_pci.c | 46 +++++++++++++++++++++++++++++++++++++++++
include/linux/virtio_config.h | 21 ++++++++++++++++++
2 files changed, 67 insertions(+), 0 deletions(-)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index adb24f2..2ff0451 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -48,6 +48,7 @@ struct virtio_pci_device
int msix_enabled;
int intx_enabled;
struct msix_entry *msix_entries;
+ cpumask_var_t *msix_affinity_masks;
/* Name strings for interrupts. This size should be enough,
* and I'm too lazy to allocate each name separately. */
char (*msix_names)[256];
@@ -276,6 +277,10 @@ static void vp_free_vectors(struct virtio_device *vdev)
for (i = 0; i < vp_dev->msix_used_vectors; ++i)
free_irq(vp_dev->msix_entries[i].vector, vp_dev);
+ for (i = 0; i < vp_dev->msix_vectors; i++)
+ if (vp_dev->msix_affinity_masks[i])
+ free_cpumask_var(vp_dev->msix_affinity_masks[i]);
+
if (vp_dev->msix_enabled) {
/* Disable the vector used for configuration */
iowrite16(VIRTIO_MSI_NO_VECTOR,
@@ -293,6 +298,8 @@ static void vp_free_vectors(struct virtio_device *vdev)
vp_dev->msix_names = NULL;
kfree(vp_dev->msix_entries);
vp_dev->msix_entries = NULL;
+ kfree(vp_dev->msix_affinity_masks);
+ vp_dev->msix_affinity_masks = NULL;
}
static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
@@ -311,6 +318,15 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
GFP_KERNEL);
if (!vp_dev->msix_names)
goto error;
+ vp_dev->msix_affinity_masks
+ = kzalloc(nvectors * sizeof *vp_dev->msix_affinity_masks,
+ GFP_KERNEL);
+ if (!vp_dev->msix_affinity_masks)
+ goto error;
+ for (i = 0; i < nvectors; ++i)
+ if (!alloc_cpumask_var(&vp_dev->msix_affinity_masks[i],
+ GFP_KERNEL))
+ goto error;
for (i = 0; i < nvectors; ++i)
vp_dev->msix_entries[i].entry = i;
@@ -607,6 +623,35 @@ static const char *vp_bus_name(struct virtio_device *vdev)
return pci_name(vp_dev->pci_dev);
}
+/* Setup the affinity for a virtqueue:
+ * - force the affinity for per vq vector
+ * - OR over all affinities for shared MSI
+ * - ignore the affinity request if we're using INTX
+ */
+static int vp_set_vq_affinity(struct virtqueue *vq, int cpu)
+{
+ struct virtio_device *vdev = vq->vdev;
+ struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+ struct virtio_pci_vq_info *info = vq->priv;
+ struct cpumask *mask;
+ unsigned int irq;
+
+ if (!vq->callback)
+ return -EINVAL;
+
+ if (vp_dev->msix_enabled) {
+ mask = vp_dev->msix_affinity_masks[info->msix_vector];
+ irq = vp_dev->msix_entries[info->msix_vector].vector;
+ if (cpu == -1)
+ irq_set_affinity_hint(irq, NULL);
+ else {
+ cpumask_set_cpu(cpu, mask);
+ irq_set_affinity_hint(irq, mask);
+ }
+ }
+ return 0;
+}
+
static struct virtio_config_ops virtio_pci_config_ops = {
.get = vp_get,
.set = vp_set,
@@ -618,6 +663,7 @@ static struct virtio_config_ops virtio_pci_config_ops = {
.get_features = vp_get_features,
.finalize_features = vp_finalize_features,
.bus_name = vp_bus_name,
+ .set_vq_affinity = vp_set_vq_affinity,
};
static void virtio_pci_release_dev(struct device *_d)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index fc457f4..2c4a989 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -98,6 +98,7 @@
* vdev: the virtio_device
* This returns a pointer to the bus name a la pci_name from which
* the caller can then copy.
+ * @set_vq_affinity: set the affinity for a virtqueue.
*/
typedef void vq_callback_t(struct virtqueue *);
struct virtio_config_ops {
@@ -116,6 +117,7 @@ struct virtio_config_ops {
u32 (*get_features)(struct virtio_device *vdev);
void (*finalize_features)(struct virtio_device *vdev);
const char *(*bus_name)(struct virtio_device *vdev);
+ int (*set_vq_affinity)(struct virtqueue *vq, int cpu);
};
/* If driver didn't advertise the feature, it will never appear. */
@@ -190,5 +192,24 @@ const char *virtio_bus_name(struct virtio_device *vdev)
return vdev->config->bus_name(vdev);
}
+/**
+ * virtqueue_set_affinity - setting affinity for a virtqueue
+ * @vq: the virtqueue
+ * @cpu: the cpu no.
+ *
+ * Pay attention the function are best-effort: the affinity hint may not be set
+ * due to config support, irq type and sharing.
+ *
+ */
+static inline
+int virtqueue_set_affinity(struct virtqueue *vq, int cpu)
+{
+ struct virtio_device *vdev = vq->vdev;
+ if (vdev->config->set_vq_affinity)
+ return vdev->config->set_vq_affinity(vq, cpu);
+ return 0;
+}
+
+
#endif /* __KERNEL__ */
#endif /* _LINUX_VIRTIO_CONFIG_H */
--
1.7.1
^ permalink raw reply related
* [PATCH 3/5] virtio-scsi: allocate target pointers in a separate memory block
From: Paolo Bonzini @ 2012-08-28 11:54 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-scsi, kvm, mst, virtualization
In-Reply-To: <1346154857-12487-1-git-send-email-pbonzini@redhat.com>
We will place the request virtqueues in the flexible array member.
Refining the virtqueue API would let us drop the sglist copy, at
which point the pointer-to-array-of-pointers can become a simple
pointer-to-array. It would both simplify the allocation and remove a
dereference in several hot paths.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
drivers/scsi/virtio_scsi.c | 23 +++++++++++++++--------
1 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 595af1a..62fec04 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -77,7 +77,7 @@ struct virtio_scsi {
/* Get some buffers ready for event vq */
struct virtio_scsi_event_node event_list[VIRTIO_SCSI_EVENT_LEN];
- struct virtio_scsi_target_state *tgt[];
+ struct virtio_scsi_target_state **tgt;
};
static struct kmem_cache *virtscsi_cmd_cache;
@@ -615,10 +615,13 @@ static void virtscsi_remove_vqs(struct virtio_device *vdev)
/* Stop all the virtqueues. */
vdev->config->reset(vdev);
- num_targets = sh->max_id;
- for (i = 0; i < num_targets; i++) {
- kfree(vscsi->tgt[i]);
- vscsi->tgt[i] = NULL;
+ if (vscsi->tgt) {
+ num_targets = sh->max_id;
+ for (i = 0; i < num_targets; i++) {
+ kfree(vscsi->tgt[i]);
+ vscsi->tgt[i] = NULL;
+ }
+ kfree(vscsi->tgt);
}
vdev->config->del_vqs(vdev);
@@ -660,6 +663,12 @@ static int virtscsi_init(struct virtio_device *vdev,
/* We need to know how many segments before we allocate. */
sg_elems = virtscsi_config_get(vdev, seg_max) ?: 1;
+ vscsi->tgt = kmalloc(num_targets *
+ sizeof(struct virtio_scsi_target_state *), GFP_KERNEL);
+ if (!vscsi->tgt) {
+ err = -ENOMEM;
+ goto out;
+ }
for (i = 0; i < num_targets; i++) {
vscsi->tgt[i] = virtscsi_alloc_tgt(vdev, sg_elems);
if (!vscsi->tgt[i]) {
@@ -685,9 +694,7 @@ static int __devinit virtscsi_probe(struct virtio_device *vdev)
/* Allocate memory and link the structs together. */
num_targets = virtscsi_config_get(vdev, max_target) + 1;
- shost = scsi_host_alloc(&virtscsi_host_template,
- sizeof(*vscsi)
- + num_targets * sizeof(struct virtio_scsi_target_state));
+ shost = scsi_host_alloc(&virtscsi_host_template, sizeof(*vscsi));
if (!shost)
return -ENOMEM;
--
1.7.1
^ permalink raw reply related
* [PATCH 4/5] virtio-scsi: pass struct virtio_scsi to virtqueue completion function
From: Paolo Bonzini @ 2012-08-28 11:54 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-scsi, kvm, mst, virtualization
In-Reply-To: <1346154857-12487-1-git-send-email-pbonzini@redhat.com>
This will be needed soon in order to retrieve the per-target
struct.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
drivers/scsi/virtio_scsi.c | 17 +++++++++--------
1 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 62fec04..6414ea0 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -107,7 +107,7 @@ static void virtscsi_compute_resid(struct scsi_cmnd *sc, u32 resid)
*
* Called with vq_lock held.
*/
-static void virtscsi_complete_cmd(void *buf)
+static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
{
struct virtio_scsi_cmd *cmd = buf;
struct scsi_cmnd *sc = cmd->sc;
@@ -168,7 +168,8 @@ static void virtscsi_complete_cmd(void *buf)
sc->scsi_done(sc);
}
-static void virtscsi_vq_done(struct virtqueue *vq, void (*fn)(void *buf))
+static void virtscsi_vq_done(struct virtio_scsi *vscsi, struct virtqueue *vq,
+ void (*fn)(struct virtio_scsi *vscsi, void *buf))
{
void *buf;
unsigned int len;
@@ -176,7 +177,7 @@ static void virtscsi_vq_done(struct virtqueue *vq, void (*fn)(void *buf))
do {
virtqueue_disable_cb(vq);
while ((buf = virtqueue_get_buf(vq, &len)) != NULL)
- fn(buf);
+ fn(vscsi, buf);
} while (!virtqueue_enable_cb(vq));
}
@@ -187,11 +188,11 @@ static void virtscsi_req_done(struct virtqueue *vq)
unsigned long flags;
spin_lock_irqsave(&vscsi->req_vq.vq_lock, flags);
- virtscsi_vq_done(vq, virtscsi_complete_cmd);
+ virtscsi_vq_done(vscsi, vq, virtscsi_complete_cmd);
spin_unlock_irqrestore(&vscsi->req_vq.vq_lock, flags);
};
-static void virtscsi_complete_free(void *buf)
+static void virtscsi_complete_free(struct virtio_scsi *vscsi, void *buf)
{
struct virtio_scsi_cmd *cmd = buf;
@@ -208,7 +209,7 @@ static void virtscsi_ctrl_done(struct virtqueue *vq)
unsigned long flags;
spin_lock_irqsave(&vscsi->ctrl_vq.vq_lock, flags);
- virtscsi_vq_done(vq, virtscsi_complete_free);
+ virtscsi_vq_done(vscsi, vq, virtscsi_complete_free);
spin_unlock_irqrestore(&vscsi->ctrl_vq.vq_lock, flags);
};
@@ -331,7 +332,7 @@ static void virtscsi_handle_event(struct work_struct *work)
virtscsi_kick_event(vscsi, event_node);
}
-static void virtscsi_complete_event(void *buf)
+static void virtscsi_complete_event(struct virtio_scsi *vscsi, void *buf)
{
struct virtio_scsi_event_node *event_node = buf;
@@ -346,7 +347,7 @@ static void virtscsi_event_done(struct virtqueue *vq)
unsigned long flags;
spin_lock_irqsave(&vscsi->event_vq.vq_lock, flags);
- virtscsi_vq_done(vq, virtscsi_complete_event);
+ virtscsi_vq_done(vscsi, vq, virtscsi_complete_event);
spin_unlock_irqrestore(&vscsi->event_vq.vq_lock, flags);
};
--
1.7.1
^ permalink raw reply related
* [PATCH 5/5] virtio-scsi: introduce multiqueue support
From: Paolo Bonzini @ 2012-08-28 11:54 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-scsi, kvm, mst, virtualization
In-Reply-To: <1346154857-12487-1-git-send-email-pbonzini@redhat.com>
This patch adds queue steering to virtio-scsi. When a target is sent
multiple requests, we always drive them to the same queue so that FIFO
processing order is kept. However, if a target was idle, we can choose
a queue arbitrarily. In this case the queue is chosen according to the
current VCPU, so the driver expects the number of request queues to be
equal to the number of VCPUs. This makes it easy and fast to select
the queue, and also lets the driver optimize the IRQ affinity for the
virtqueues (each virtqueue's affinity is set to the CPU that "owns"
the queue).
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
drivers/scsi/virtio_scsi.c | 162 +++++++++++++++++++++++++++++++++++---------
1 files changed, 130 insertions(+), 32 deletions(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 6414ea0..0c4b096 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -26,6 +26,7 @@
#define VIRTIO_SCSI_MEMPOOL_SZ 64
#define VIRTIO_SCSI_EVENT_LEN 8
+#define VIRTIO_SCSI_VQ_BASE 2
/* Command queue element */
struct virtio_scsi_cmd {
@@ -59,9 +60,13 @@ struct virtio_scsi_vq {
/* Per-target queue state */
struct virtio_scsi_target_state {
- /* Protects sg. Lock hierarchy is tgt_lock -> vq_lock. */
+ /* Protects sg, req_vq. Lock hierarchy is tgt_lock -> vq_lock. */
spinlock_t tgt_lock;
+ struct virtio_scsi_vq *req_vq;
+
+ atomic_t reqs;
+
/* For sglist construction when adding commands to the virtqueue. */
struct scatterlist sg[];
};
@@ -70,14 +75,15 @@ struct virtio_scsi_target_state {
struct virtio_scsi {
struct virtio_device *vdev;
- struct virtio_scsi_vq ctrl_vq;
- struct virtio_scsi_vq event_vq;
- struct virtio_scsi_vq req_vq;
-
/* Get some buffers ready for event vq */
struct virtio_scsi_event_node event_list[VIRTIO_SCSI_EVENT_LEN];
+ u32 num_queues;
struct virtio_scsi_target_state **tgt;
+
+ struct virtio_scsi_vq ctrl_vq;
+ struct virtio_scsi_vq event_vq;
+ struct virtio_scsi_vq req_vqs[];
};
static struct kmem_cache *virtscsi_cmd_cache;
@@ -112,6 +118,9 @@ static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
struct virtio_scsi_cmd *cmd = buf;
struct scsi_cmnd *sc = cmd->sc;
struct virtio_scsi_cmd_resp *resp = &cmd->resp.cmd;
+ struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
+
+ atomic_dec(&tgt->reqs);
dev_dbg(&sc->device->sdev_gendev,
"cmd %p response %u status %#02x sense_len %u\n",
@@ -185,11 +194,13 @@ static void virtscsi_req_done(struct virtqueue *vq)
{
struct Scsi_Host *sh = virtio_scsi_host(vq->vdev);
struct virtio_scsi *vscsi = shost_priv(sh);
+ int index = virtqueue_get_queue_index(vq) - VIRTIO_SCSI_VQ_BASE;
+ struct virtio_scsi_vq *req_vq = &vscsi->req_vqs[index];
unsigned long flags;
- spin_lock_irqsave(&vscsi->req_vq.vq_lock, flags);
+ spin_lock_irqsave(&req_vq->vq_lock, flags);
virtscsi_vq_done(vscsi, vq, virtscsi_complete_cmd);
- spin_unlock_irqrestore(&vscsi->req_vq.vq_lock, flags);
+ spin_unlock_irqrestore(&req_vq->vq_lock, flags);
};
static void virtscsi_complete_free(struct virtio_scsi *vscsi, void *buf)
@@ -429,10 +440,10 @@ static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
return ret;
}
-static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
+static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
+ struct virtio_scsi_target_state *tgt,
+ struct scsi_cmnd *sc)
{
- struct virtio_scsi *vscsi = shost_priv(sh);
- struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
struct virtio_scsi_cmd *cmd;
int ret;
@@ -466,7 +477,7 @@ static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
- if (virtscsi_kick_cmd(tgt, &vscsi->req_vq, cmd,
+ if (virtscsi_kick_cmd(tgt, tgt->req_vq, cmd,
sizeof cmd->req.cmd, sizeof cmd->resp.cmd,
GFP_ATOMIC) >= 0)
ret = 0;
@@ -475,6 +486,38 @@ out:
return ret;
}
+static int virtscsi_queuecommand_single(struct Scsi_Host *sh,
+ struct scsi_cmnd *sc)
+{
+ struct virtio_scsi *vscsi = shost_priv(sh);
+ struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
+
+ atomic_inc(&tgt->reqs);
+ return virtscsi_queuecommand(vscsi, tgt, sc);
+}
+
+static int virtscsi_queuecommand_multi(struct Scsi_Host *sh,
+ struct scsi_cmnd *sc)
+{
+ struct virtio_scsi *vscsi = shost_priv(sh);
+ struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
+ unsigned long flags;
+ u32 queue_num;
+
+ /* Using an atomic_t for tgt->reqs lets the virtqueue handler
+ * decrement it without taking the spinlock.
+ */
+ spin_lock_irqsave(&tgt->tgt_lock, flags);
+ if (atomic_inc_return(&tgt->reqs) == 1) {
+ queue_num = smp_processor_id();
+ while (unlikely(queue_num >= vscsi->num_queues))
+ queue_num -= vscsi->num_queues;
+ tgt->req_vq = &vscsi->req_vqs[queue_num];
+ }
+ spin_unlock_irqrestore(&tgt->tgt_lock, flags);
+ return virtscsi_queuecommand(vscsi, tgt, sc);
+}
+
static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
{
DECLARE_COMPLETION_ONSTACK(comp);
@@ -544,12 +585,26 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
return virtscsi_tmf(vscsi, cmd);
}
-static struct scsi_host_template virtscsi_host_template = {
+static struct scsi_host_template virtscsi_host_template_single = {
.module = THIS_MODULE,
.name = "Virtio SCSI HBA",
.proc_name = "virtio_scsi",
- .queuecommand = virtscsi_queuecommand,
.this_id = -1,
+ .queuecommand = virtscsi_queuecommand_single,
+ .eh_abort_handler = virtscsi_abort,
+ .eh_device_reset_handler = virtscsi_device_reset,
+
+ .can_queue = 1024,
+ .dma_boundary = UINT_MAX,
+ .use_clustering = ENABLE_CLUSTERING,
+};
+
+static struct scsi_host_template virtscsi_host_template_multi = {
+ .module = THIS_MODULE,
+ .name = "Virtio SCSI HBA",
+ .proc_name = "virtio_scsi",
+ .this_id = -1,
+ .queuecommand = virtscsi_queuecommand_multi,
.eh_abort_handler = virtscsi_abort,
.eh_device_reset_handler = virtscsi_device_reset,
@@ -575,15 +630,19 @@ static struct scsi_host_template virtscsi_host_template = {
&__val, sizeof(__val)); \
})
+
static void virtscsi_init_vq(struct virtio_scsi_vq *virtscsi_vq,
- struct virtqueue *vq)
+ struct virtqueue *vq, bool affinity)
{
spin_lock_init(&virtscsi_vq->vq_lock);
virtscsi_vq->vq = vq;
+ if (affinity)
+ virtqueue_set_affinity(vq, virtqueue_get_queue_index(vq) -
+ VIRTIO_SCSI_VQ_BASE);
}
static struct virtio_scsi_target_state *virtscsi_alloc_tgt(
- struct virtio_device *vdev, int sg_elems)
+ struct virtio_scsi *vscsi, u32 sg_elems)
{
struct virtio_scsi_target_state *tgt;
gfp_t gfp_mask = GFP_KERNEL;
@@ -597,6 +656,13 @@ static struct virtio_scsi_target_state *virtscsi_alloc_tgt(
spin_lock_init(&tgt->tgt_lock);
sg_init_table(tgt->sg, sg_elems + 2);
+ atomic_set(&tgt->reqs, 0);
+
+ /*
+ * The default is unused for multiqueue, but with a single queue
+ * or target we use it in virtscsi_queuecommand.
+ */
+ tgt->req_vq = &vscsi->req_vqs[0];
return tgt;
}
@@ -632,28 +698,41 @@ static int virtscsi_init(struct virtio_device *vdev,
struct virtio_scsi *vscsi, int num_targets)
{
int err;
- struct virtqueue *vqs[3];
u32 i, sg_elems;
+ u32 num_vqs;
+ vq_callback_t **callbacks;
+ const char **names;
+ struct virtqueue **vqs;
- vq_callback_t *callbacks[] = {
- virtscsi_ctrl_done,
- virtscsi_event_done,
- virtscsi_req_done
- };
- const char *names[] = {
- "control",
- "event",
- "request"
- };
+ num_vqs = vscsi->num_queues + VIRTIO_SCSI_VQ_BASE;
+ vqs = kmalloc(num_vqs * sizeof(struct virtqueue *), GFP_KERNEL);
+ callbacks = kmalloc(num_vqs * sizeof(vq_callback_t *), GFP_KERNEL);
+ names = kmalloc(num_vqs * sizeof(char *), GFP_KERNEL);
+
+ if (!callbacks || !vqs || !names) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ callbacks[0] = virtscsi_ctrl_done;
+ callbacks[1] = virtscsi_event_done;
+ names[0] = "control";
+ names[1] = "event";
+ for (i = VIRTIO_SCSI_VQ_BASE; i < num_vqs; i++) {
+ callbacks[i] = virtscsi_req_done;
+ names[i] = "request";
+ }
/* Discover virtqueues and write information to configuration. */
- err = vdev->config->find_vqs(vdev, 3, vqs, callbacks, names);
+ err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
if (err)
return err;
- virtscsi_init_vq(&vscsi->ctrl_vq, vqs[0]);
- virtscsi_init_vq(&vscsi->event_vq, vqs[1]);
- virtscsi_init_vq(&vscsi->req_vq, vqs[2]);
+ virtscsi_init_vq(&vscsi->ctrl_vq, vqs[0], false);
+ virtscsi_init_vq(&vscsi->event_vq, vqs[1], false);
+ for (i = VIRTIO_SCSI_VQ_BASE; i < num_vqs; i++)
+ virtscsi_init_vq(&vscsi->req_vqs[i - VIRTIO_SCSI_VQ_BASE],
+ vqs[i], vscsi->num_queues > 1);
virtscsi_config_set(vdev, cdb_size, VIRTIO_SCSI_CDB_SIZE);
virtscsi_config_set(vdev, sense_size, VIRTIO_SCSI_SENSE_SIZE);
@@ -671,7 +750,7 @@ static int virtscsi_init(struct virtio_device *vdev,
goto out;
}
for (i = 0; i < num_targets; i++) {
- vscsi->tgt[i] = virtscsi_alloc_tgt(vdev, sg_elems);
+ vscsi->tgt[i] = virtscsi_alloc_tgt(vscsi, sg_elems);
if (!vscsi->tgt[i]) {
err = -ENOMEM;
goto out;
@@ -680,6 +759,9 @@ static int virtscsi_init(struct virtio_device *vdev,
err = 0;
out:
+ kfree(names);
+ kfree(callbacks);
+ kfree(vqs);
if (err)
virtscsi_remove_vqs(vdev);
return err;
@@ -692,11 +774,26 @@ static int __devinit virtscsi_probe(struct virtio_device *vdev)
int err;
u32 sg_elems, num_targets;
u32 cmd_per_lun;
+ u32 num_queues;
+ struct scsi_host_template *hostt;
+
+ /* We need to know how many queues before we allocate. */
+ num_queues = virtscsi_config_get(vdev, num_queues) ?: 1;
/* Allocate memory and link the structs together. */
num_targets = virtscsi_config_get(vdev, max_target) + 1;
- shost = scsi_host_alloc(&virtscsi_host_template, sizeof(*vscsi));
+ /* Multiqueue is not beneficial with a single target. */
+ if (num_targets == 1)
+ num_queues = 1;
+
+ if (num_queues == 1)
+ hostt = &virtscsi_host_template_single;
+ else
+ hostt = &virtscsi_host_template_multi;
+
+ shost = scsi_host_alloc(hostt,
+ sizeof(*vscsi) + sizeof(vscsi->req_vqs[0]) * num_queues);
if (!shost)
return -ENOMEM;
@@ -704,6 +801,7 @@ static int __devinit virtscsi_probe(struct virtio_device *vdev)
shost->sg_tablesize = sg_elems;
vscsi = shost_priv(shost);
vscsi->vdev = vdev;
+ vscsi->num_queues = num_queues;
vdev->priv = shost;
err = virtscsi_init(vdev, vscsi, num_targets);
--
1.7.1
^ permalink raw reply related
* Re: [RFC 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Sasha Levin @ 2012-08-28 12:54 UTC (permalink / raw)
To: Jean-Mickael Guerin; +Cc: virtualization, kvm, mst
In-Reply-To: <4FE01355.6070909@6wind.com>
On 06/19/2012 07:51 AM, Jean-Mickael Guerin wrote:
>> + if (vq->vring.desc[i].flags& VRING_DESC_F_INDIRECT) {
>> + if (vq->vring.desc[i].len< vq->indirect_alloc_thresh)
> This should be > instead of <, no?
It should, yeah.
Too bad slub doesn't yell at you for doing that.
^ permalink raw reply
* [PATCH v2 1/2] virtio-ring: Use threshold for switching to indirect descriptors
From: Sasha Levin @ 2012-08-28 13:04 UTC (permalink / raw)
To: rusty, mst; +Cc: avi, Sasha Levin, linux-kernel, kvm, virtualization
Currently if VIRTIO_RING_F_INDIRECT_DESC is enabled we will use indirect
descriptors even if we have plenty of space in the ring. This means that
we take a performance hit at all times due to the overhead of creating
indirect descriptors.
Instead, use it only after we're below a configurable offset.
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
drivers/block/virtio_blk.c | 4 ++++
drivers/char/hw_random/virtio-rng.c | 4 ++++
drivers/char/virtio_console.c | 4 ++++
drivers/net/virtio_net.c | 4 ++++
drivers/virtio/virtio_balloon.c | 4 ++++
drivers/virtio/virtio_ring.c | 21 +++++++++++++++------
include/linux/virtio.h | 1 +
net/9p/trans_virtio.c | 4 ++++
8 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 2edfb5c..13b8ae9 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -22,6 +22,9 @@ static DEFINE_IDA(vd_index_ida);
struct workqueue_struct *virtblk_wq;
+static unsigned int indirect_thresh;
+module_param(indirect_thresh, uint, S_IRUGO);
+
struct virtio_blk
{
struct virtio_device *vdev;
@@ -735,6 +738,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
vblk->config_enable = true;
+ vdev->indirect_thresh = indirect_thresh;
err = init_vq(vblk);
if (err)
diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 5708299..02d8421 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -25,6 +25,9 @@
#include <linux/virtio_rng.h>
#include <linux/module.h>
+static unsigned int indirect_thresh;
+module_param(indirect_thresh, uint, S_IRUGO);
+
static struct virtqueue *vq;
static unsigned int data_avail;
static DECLARE_COMPLETION(have_data);
@@ -93,6 +96,7 @@ static int probe_common(struct virtio_device *vdev)
int err;
/* We expect a single virtqueue. */
+ vdev->indirect_thresh = indirect_thresh;
vq = virtio_find_single_vq(vdev, random_recv_done, "input");
if (IS_ERR(vq))
return PTR_ERR(vq);
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e88f843..fc14e7f 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -39,6 +39,9 @@
#include <linux/module.h>
#include "../tty/hvc/hvc_console.h"
+static unsigned int indirect_thresh;
+module_param(indirect_thresh, uint, S_IRUGO);
+
/*
* This is a global struct for storing common data for all the devices
* this driver handles.
@@ -1887,6 +1890,7 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
max_nr_ports),
&portdev->config.max_nr_ports) == 0)
multiport = true;
+ vdev->indirect_thresh = indirect_thresh;
err = init_vqs(portdev);
if (err < 0) {
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index cbf8b06..64a8321 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -34,6 +34,9 @@ static bool csum = true, gso = true;
module_param(csum, bool, 0444);
module_param(gso, bool, 0444);
+static unsigned int indirect_thresh;
+module_param(indirect_thresh, uint, S_IRUGO);
+
/* FIXME: MTU in config. */
#define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
#define GOOD_COPY_LEN 128
@@ -1128,6 +1131,7 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
vi->mergeable_rx_bufs = true;
+ vdev->indirect_thresh = indirect_thresh;
err = init_vqs(vi);
if (err)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0908e60..fce7347 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -35,6 +35,9 @@
*/
#define VIRTIO_BALLOON_PAGES_PER_PAGE (PAGE_SIZE >> VIRTIO_BALLOON_PFN_SHIFT)
+static unsigned int indirect_thresh;
+module_param(indirect_thresh, uint, S_IRUGO);
+
struct virtio_balloon
{
struct virtio_device *vdev;
@@ -356,6 +359,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
init_waitqueue_head(&vb->acked);
vb->vdev = vdev;
vb->need_stats_update = 0;
+ vdev->indirect_thresh = indirect_thresh;
err = init_vqs(vb);
if (err)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5aa43c3..99a64a7 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -87,8 +87,11 @@ struct vring_virtqueue
/* Other side has made a mess, don't try any more. */
bool broken;
- /* Host supports indirect buffers */
- bool indirect;
+ /*
+ * Min. number of free space in the ring to trigger direct
+ * descriptor use
+ */
+ unsigned int indirect_thresh;
/* Host publishes avail event idx */
bool event;
@@ -216,9 +219,12 @@ int virtqueue_add_buf(struct virtqueue *_vq,
}
#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) {
+ /*
+ * If the host supports indirect descriptor tables, and we have multiple
+ * buffers, then go indirect.
+ */
+ if ((out + in) > 1 && vq->num_free &&
+ (vq->num_free < vq->indirect_thresh)) {
head = vring_add_indirect(vq, sg, out, in, gfp);
if (likely(head >= 0))
goto add_head;
@@ -647,13 +653,16 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
vq->broken = false;
vq->last_used_idx = 0;
vq->num_added = 0;
+ vq->indirect_thresh = 0;
list_add_tail(&vq->vq.list, &vdev->vqs);
#ifdef DEBUG
vq->in_use = false;
vq->last_add_time_valid = false;
#endif
- vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
+ if (virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC))
+ vq->indirect_thresh = vdev->indirect_thresh;
+
vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
/* No callback? Tell other side not to bother us. */
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index a1ba8bb..48bc457 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -69,6 +69,7 @@ struct virtio_device {
/* Note that this is a Linux set_bit-style bitmap. */
unsigned long features[1];
void *priv;
+ unsigned int indirect_thresh;
};
#define dev_to_virtio(dev) container_of(dev, struct virtio_device, dev)
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 35b8911..fc93962 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -52,6 +52,9 @@
#define VIRTQUEUE_NUM 128
+static unsigned int indirect_thresh;
+module_param(indirect_thresh, uint, S_IRUGO);
+
/* a single mutex to manage channel initialization and attachment */
static DEFINE_MUTEX(virtio_9p_lock);
static DECLARE_WAIT_QUEUE_HEAD(vp_wq);
@@ -501,6 +504,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
chan->vdev = vdev;
/* We expect one virtqueue, for requests. */
+ vdev->indirect_thresh = indirect_thresh;
chan->vq = virtio_find_single_vq(vdev, req_done, "requests");
if (IS_ERR(chan->vq)) {
err = PTR_ERR(chan->vq);
--
1.7.12
^ permalink raw reply related
* [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Sasha Levin @ 2012-08-28 13:04 UTC (permalink / raw)
To: rusty, mst; +Cc: avi, Sasha Levin, linux-kernel, kvm, virtualization
In-Reply-To: <1346159043-16446-1-git-send-email-levinsasha928@gmail.com>
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>
---
Changes in v2:
- Free correctly indirect buffers.
drivers/block/virtio_blk.c | 4 ++++
drivers/char/hw_random/virtio-rng.c | 4 ++++
drivers/char/virtio_console.c | 4 ++++
drivers/net/virtio_net.c | 4 ++++
drivers/virtio/virtio_balloon.c | 4 ++++
drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++++----
include/linux/virtio.h | 1 +
net/9p/trans_virtio.c | 5 +++++
8 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 13b8ae9..7f670af 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -25,6 +25,9 @@ struct workqueue_struct *virtblk_wq;
static unsigned int indirect_thresh;
module_param(indirect_thresh, uint, S_IRUGO);
+static unsigned int indirect_alloc_thresh;
+module_param(indirect_alloc_thresh, uint, S_IRUGO);
+
struct virtio_blk
{
struct virtio_device *vdev;
@@ -739,6 +742,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
vblk->config_enable = true;
vdev->indirect_thresh = indirect_thresh;
+ vdev->indirect_alloc_thresh = indirect_alloc_thresh;
err = init_vq(vblk);
if (err)
diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 02d8421..8475ece 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -28,6 +28,9 @@
static unsigned int indirect_thresh;
module_param(indirect_thresh, uint, S_IRUGO);
+static unsigned int indirect_alloc_thresh;
+module_param(indirect_alloc_thresh, uint, S_IRUGO);
+
static struct virtqueue *vq;
static unsigned int data_avail;
static DECLARE_COMPLETION(have_data);
@@ -97,6 +100,7 @@ static int probe_common(struct virtio_device *vdev)
/* We expect a single virtqueue. */
vdev->indirect_thresh = indirect_thresh;
+ vdev->indirect_alloc_thresh = indirect_alloc_thresh;
vq = virtio_find_single_vq(vdev, random_recv_done, "input");
if (IS_ERR(vq))
return PTR_ERR(vq);
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fc14e7f..c6f24a7 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -42,6 +42,9 @@
static unsigned int indirect_thresh;
module_param(indirect_thresh, uint, S_IRUGO);
+static unsigned int indirect_alloc_thresh;
+module_param(indirect_alloc_thresh, uint, S_IRUGO);
+
/*
* This is a global struct for storing common data for all the devices
* this driver handles.
@@ -1891,6 +1894,7 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
&portdev->config.max_nr_ports) == 0)
multiport = true;
vdev->indirect_thresh = indirect_thresh;
+ vdev->indirect_alloc_thresh = indirect_alloc_thresh;
err = init_vqs(portdev);
if (err < 0) {
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 64a8321..c091efd 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -37,6 +37,9 @@ module_param(gso, bool, 0444);
static unsigned int indirect_thresh;
module_param(indirect_thresh, uint, S_IRUGO);
+static unsigned int indirect_alloc_thresh;
+module_param(indirect_alloc_thresh, uint, S_IRUGO);
+
/* FIXME: MTU in config. */
#define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
#define GOOD_COPY_LEN 128
@@ -1132,6 +1135,7 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
vi->mergeable_rx_bufs = true;
vdev->indirect_thresh = indirect_thresh;
+ vdev->indirect_alloc_thresh = indirect_alloc_thresh;
err = init_vqs(vi);
if (err)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index fce7347..ccf87db 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -38,6 +38,9 @@
static unsigned int indirect_thresh;
module_param(indirect_thresh, uint, S_IRUGO);
+static unsigned int indirect_alloc_thresh;
+module_param(indirect_alloc_thresh, uint, S_IRUGO);
+
struct virtio_balloon
{
struct virtio_device *vdev;
@@ -360,6 +363,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
vb->vdev = vdev;
vb->need_stats_update = 0;
vdev->indirect_thresh = indirect_thresh;
+ vdev->indirect_alloc_thresh = indirect_alloc_thresh;
err = init_vqs(vb);
if (err)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 99a64a7..e8b9c54 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -93,6 +93,10 @@ struct vring_virtqueue
*/
unsigned int indirect_thresh;
+ /* Buffers below this size will be allocated from cache */
+ unsigned int indirect_alloc_thresh;
+ struct kmem_cache *indirect_cache;
+
/* Host publishes avail event idx */
bool event;
@@ -135,7 +139,10 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
unsigned head;
int i;
- desc = kmalloc((out + in) * sizeof(struct vring_desc), gfp);
+ if ((out + in) <= vq->indirect_alloc_thresh)
+ desc = kmem_cache_alloc(vq->indirect_cache, gfp);
+ else
+ desc = kmalloc((out + in) * sizeof(struct vring_desc), gfp);
if (!desc)
return -ENOMEM;
@@ -384,8 +391,13 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
i = head;
/* Free the indirect table */
- if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT)
- kfree(phys_to_virt(vq->vring.desc[i].addr));
+ if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT) {
+ if (vq->vring.desc[i].len > vq->indirect_alloc_thresh)
+ kfree(phys_to_virt(vq->vring.desc[i].addr));
+ else
+ kmem_cache_free(vq->indirect_cache,
+ phys_to_virt(vq->vring.desc[i].addr));
+ }
while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) {
i = vq->vring.desc[i].next;
@@ -654,14 +666,20 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
vq->last_used_idx = 0;
vq->num_added = 0;
vq->indirect_thresh = 0;
+ vq->indirect_alloc_thresh = 0;
+ vq->indirect_cache = NULL;
list_add_tail(&vq->vq.list, &vdev->vqs);
#ifdef DEBUG
vq->in_use = false;
vq->last_add_time_valid = false;
#endif
- if (virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC))
+ if (virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
vq->indirect_thresh = vdev->indirect_thresh;
+ vq->indirect_alloc_thresh = vdev->indirect_alloc_thresh;
+ if (vq->indirect_alloc_thresh)
+ vq->indirect_cache = KMEM_CACHE(vring_desc[vq->indirect_alloc_thresh], 0);
+ }
vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
@@ -685,6 +703,8 @@ EXPORT_SYMBOL_GPL(vring_new_virtqueue);
void vring_del_virtqueue(struct virtqueue *vq)
{
list_del(&vq->list);
+ if (to_vvq(vq)->indirect_cache)
+ kmem_cache_destroy(to_vvq(vq)->indirect_cache);
kfree(to_vvq(vq));
}
EXPORT_SYMBOL_GPL(vring_del_virtqueue);
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 48bc457..3261c02 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -70,6 +70,7 @@ struct virtio_device {
unsigned long features[1];
void *priv;
unsigned int indirect_thresh;
+ unsigned int indirect_alloc_thresh;
};
#define dev_to_virtio(dev) container_of(dev, struct virtio_device, dev)
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index fc93962..3044c86 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -55,6 +55,9 @@
static unsigned int indirect_thresh;
module_param(indirect_thresh, uint, S_IRUGO);
+static unsigned int indirect_alloc_thresh;
+module_param(indirect_alloc_thresh, uint, S_IRUGO);
+
/* a single mutex to manage channel initialization and attachment */
static DEFINE_MUTEX(virtio_9p_lock);
static DECLARE_WAIT_QUEUE_HEAD(vp_wq);
@@ -505,6 +508,8 @@ static int p9_virtio_probe(struct virtio_device *vdev)
/* We expect one virtqueue, for requests. */
vdev->indirect_thresh = indirect_thresh;
+ vdev->indirect_alloc_thresh = indirect_alloc_thresh;
+
chan->vq = virtio_find_single_vq(vdev, req_done, "requests");
if (IS_ERR(chan->vq)) {
err = PTR_ERR(chan->vq);
--
1.7.12
^ permalink raw reply related
* Re: [PATCH v2 1/2] virtio-ring: Use threshold for switching to indirect descriptors
From: Michael S. Tsirkin @ 2012-08-28 13:20 UTC (permalink / raw)
To: Sasha Levin; +Cc: avi, linux-kernel, kvm, virtualization
In-Reply-To: <1346159043-16446-1-git-send-email-levinsasha928@gmail.com>
On Tue, Aug 28, 2012 at 03:04:02PM +0200, Sasha Levin wrote:
> Currently if VIRTIO_RING_F_INDIRECT_DESC is enabled we will use indirect
> descriptors even if we have plenty of space in the ring. This means that
> we take a performance hit at all times due to the overhead of creating
> indirect descriptors.
>
> Instead, use it only after we're below a configurable offset.
>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
I imagine this helps performance? Any numbers?
> ---
> drivers/block/virtio_blk.c | 4 ++++
> drivers/char/hw_random/virtio-rng.c | 4 ++++
> drivers/char/virtio_console.c | 4 ++++
> drivers/net/virtio_net.c | 4 ++++
> drivers/virtio/virtio_balloon.c | 4 ++++
> drivers/virtio/virtio_ring.c | 21 +++++++++++++++------
> include/linux/virtio.h | 1 +
> net/9p/trans_virtio.c | 4 ++++
> 8 files changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 2edfb5c..13b8ae9 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -22,6 +22,9 @@ static DEFINE_IDA(vd_index_ida);
>
> struct workqueue_struct *virtblk_wq;
>
> +static unsigned int indirect_thresh;
> +module_param(indirect_thresh, uint, S_IRUGO);
> +
> struct virtio_blk
> {
> struct virtio_device *vdev;
> @@ -735,6 +738,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>
> INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
> vblk->config_enable = true;
> + vdev->indirect_thresh = indirect_thresh;
>
> err = init_vq(vblk);
> if (err)
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index 5708299..02d8421 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -25,6 +25,9 @@
> #include <linux/virtio_rng.h>
> #include <linux/module.h>
>
> +static unsigned int indirect_thresh;
> +module_param(indirect_thresh, uint, S_IRUGO);
> +
> static struct virtqueue *vq;
> static unsigned int data_avail;
> static DECLARE_COMPLETION(have_data);
> @@ -93,6 +96,7 @@ static int probe_common(struct virtio_device *vdev)
> int err;
>
> /* We expect a single virtqueue. */
> + vdev->indirect_thresh = indirect_thresh;
> vq = virtio_find_single_vq(vdev, random_recv_done, "input");
> if (IS_ERR(vq))
> return PTR_ERR(vq);
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index e88f843..fc14e7f 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -39,6 +39,9 @@
> #include <linux/module.h>
> #include "../tty/hvc/hvc_console.h"
>
> +static unsigned int indirect_thresh;
> +module_param(indirect_thresh, uint, S_IRUGO);
> +
> /*
> * This is a global struct for storing common data for all the devices
> * this driver handles.
> @@ -1887,6 +1890,7 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
> max_nr_ports),
> &portdev->config.max_nr_ports) == 0)
> multiport = true;
> + vdev->indirect_thresh = indirect_thresh;
>
> err = init_vqs(portdev);
> if (err < 0) {
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index cbf8b06..64a8321 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -34,6 +34,9 @@ static bool csum = true, gso = true;
> module_param(csum, bool, 0444);
> module_param(gso, bool, 0444);
>
> +static unsigned int indirect_thresh;
> +module_param(indirect_thresh, uint, S_IRUGO);
> +
> /* FIXME: MTU in config. */
> #define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> #define GOOD_COPY_LEN 128
> @@ -1128,6 +1131,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
> vi->mergeable_rx_bufs = true;
> + vdev->indirect_thresh = indirect_thresh;
>
> err = init_vqs(vi);
> if (err)
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 0908e60..fce7347 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -35,6 +35,9 @@
> */
> #define VIRTIO_BALLOON_PAGES_PER_PAGE (PAGE_SIZE >> VIRTIO_BALLOON_PFN_SHIFT)
>
> +static unsigned int indirect_thresh;
> +module_param(indirect_thresh, uint, S_IRUGO);
> +
> struct virtio_balloon
> {
> struct virtio_device *vdev;
> @@ -356,6 +359,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> init_waitqueue_head(&vb->acked);
> vb->vdev = vdev;
> vb->need_stats_update = 0;
> + vdev->indirect_thresh = indirect_thresh;
>
> err = init_vqs(vb);
> if (err)
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 5aa43c3..99a64a7 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -87,8 +87,11 @@ struct vring_virtqueue
> /* Other side has made a mess, don't try any more. */
> bool broken;
>
> - /* Host supports indirect buffers */
> - bool indirect;
> + /*
> + * Min. number of free space in the ring to trigger direct
> + * descriptor use
> + */
> + unsigned int indirect_thresh;
>
> /* Host publishes avail event idx */
> bool event;
> @@ -216,9 +219,12 @@ int virtqueue_add_buf(struct virtqueue *_vq,
> }
> #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) {
> + /*
> + * If the host supports indirect descriptor tables, and we have multiple
> + * buffers, then go indirect.
> + */
> + if ((out + in) > 1 && vq->num_free &&
> + (vq->num_free < vq->indirect_thresh)) {
> head = vring_add_indirect(vq, sg, out, in, gfp);
> if (likely(head >= 0))
> goto add_head;
> @@ -647,13 +653,16 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
> vq->broken = false;
> vq->last_used_idx = 0;
> vq->num_added = 0;
> + vq->indirect_thresh = 0;
> list_add_tail(&vq->vq.list, &vdev->vqs);
> #ifdef DEBUG
> vq->in_use = false;
> vq->last_add_time_valid = false;
> #endif
>
> - vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
> + if (virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC))
> + vq->indirect_thresh = vdev->indirect_thresh;
> +
> vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
>
> /* No callback? Tell other side not to bother us. */
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index a1ba8bb..48bc457 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -69,6 +69,7 @@ struct virtio_device {
> /* Note that this is a Linux set_bit-style bitmap. */
> unsigned long features[1];
> void *priv;
> + unsigned int indirect_thresh;
> };
>
> #define dev_to_virtio(dev) container_of(dev, struct virtio_device, dev)
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 35b8911..fc93962 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -52,6 +52,9 @@
>
> #define VIRTQUEUE_NUM 128
>
> +static unsigned int indirect_thresh;
> +module_param(indirect_thresh, uint, S_IRUGO);
> +
> /* a single mutex to manage channel initialization and attachment */
> static DEFINE_MUTEX(virtio_9p_lock);
> static DECLARE_WAIT_QUEUE_HEAD(vp_wq);
> @@ -501,6 +504,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
> chan->vdev = vdev;
>
> /* We expect one virtqueue, for requests. */
> + vdev->indirect_thresh = indirect_thresh;
> chan->vq = virtio_find_single_vq(vdev, req_done, "requests");
> if (IS_ERR(chan->vq)) {
> err = PTR_ERR(chan->vq);
> --
> 1.7.12
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Michael S. Tsirkin @ 2012-08-28 13:20 UTC (permalink / raw)
To: Sasha Levin; +Cc: avi, linux-kernel, kvm, virtualization
In-Reply-To: <1346159043-16446-2-git-send-email-levinsasha928@gmail.com>
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?
> ---
>
> Changes in v2:
>
> - Free correctly indirect buffers.
>
> drivers/block/virtio_blk.c | 4 ++++
> drivers/char/hw_random/virtio-rng.c | 4 ++++
> drivers/char/virtio_console.c | 4 ++++
> drivers/net/virtio_net.c | 4 ++++
> drivers/virtio/virtio_balloon.c | 4 ++++
> drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++++----
> include/linux/virtio.h | 1 +
> net/9p/trans_virtio.c | 5 +++++
> 8 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 13b8ae9..7f670af 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -25,6 +25,9 @@ struct workqueue_struct *virtblk_wq;
> static unsigned int indirect_thresh;
> module_param(indirect_thresh, uint, S_IRUGO);
>
> +static unsigned int indirect_alloc_thresh;
> +module_param(indirect_alloc_thresh, uint, S_IRUGO);
> +
> struct virtio_blk
> {
> struct virtio_device *vdev;
> @@ -739,6 +742,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
> INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
> vblk->config_enable = true;
> vdev->indirect_thresh = indirect_thresh;
> + vdev->indirect_alloc_thresh = indirect_alloc_thresh;
>
> err = init_vq(vblk);
> if (err)
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index 02d8421..8475ece 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -28,6 +28,9 @@
> static unsigned int indirect_thresh;
> module_param(indirect_thresh, uint, S_IRUGO);
>
> +static unsigned int indirect_alloc_thresh;
> +module_param(indirect_alloc_thresh, uint, S_IRUGO);
> +
> static struct virtqueue *vq;
> static unsigned int data_avail;
> static DECLARE_COMPLETION(have_data);
> @@ -97,6 +100,7 @@ static int probe_common(struct virtio_device *vdev)
>
> /* We expect a single virtqueue. */
> vdev->indirect_thresh = indirect_thresh;
> + vdev->indirect_alloc_thresh = indirect_alloc_thresh;
> vq = virtio_find_single_vq(vdev, random_recv_done, "input");
> if (IS_ERR(vq))
> return PTR_ERR(vq);
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fc14e7f..c6f24a7 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -42,6 +42,9 @@
> static unsigned int indirect_thresh;
> module_param(indirect_thresh, uint, S_IRUGO);
>
> +static unsigned int indirect_alloc_thresh;
> +module_param(indirect_alloc_thresh, uint, S_IRUGO);
> +
> /*
> * This is a global struct for storing common data for all the devices
> * this driver handles.
> @@ -1891,6 +1894,7 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
> &portdev->config.max_nr_ports) == 0)
> multiport = true;
> vdev->indirect_thresh = indirect_thresh;
> + vdev->indirect_alloc_thresh = indirect_alloc_thresh;
>
> err = init_vqs(portdev);
> if (err < 0) {
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 64a8321..c091efd 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -37,6 +37,9 @@ module_param(gso, bool, 0444);
> static unsigned int indirect_thresh;
> module_param(indirect_thresh, uint, S_IRUGO);
>
> +static unsigned int indirect_alloc_thresh;
> +module_param(indirect_alloc_thresh, uint, S_IRUGO);
> +
> /* FIXME: MTU in config. */
> #define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> #define GOOD_COPY_LEN 128
> @@ -1132,6 +1135,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
> vi->mergeable_rx_bufs = true;
> vdev->indirect_thresh = indirect_thresh;
> + vdev->indirect_alloc_thresh = indirect_alloc_thresh;
>
> err = init_vqs(vi);
> if (err)
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index fce7347..ccf87db 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -38,6 +38,9 @@
> static unsigned int indirect_thresh;
> module_param(indirect_thresh, uint, S_IRUGO);
>
> +static unsigned int indirect_alloc_thresh;
> +module_param(indirect_alloc_thresh, uint, S_IRUGO);
> +
> struct virtio_balloon
> {
> struct virtio_device *vdev;
> @@ -360,6 +363,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> vb->vdev = vdev;
> vb->need_stats_update = 0;
> vdev->indirect_thresh = indirect_thresh;
> + vdev->indirect_alloc_thresh = indirect_alloc_thresh;
>
> err = init_vqs(vb);
> if (err)
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 99a64a7..e8b9c54 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -93,6 +93,10 @@ struct vring_virtqueue
> */
> unsigned int indirect_thresh;
>
> + /* Buffers below this size will be allocated from cache */
> + unsigned int indirect_alloc_thresh;
> + struct kmem_cache *indirect_cache;
> +
> /* Host publishes avail event idx */
> bool event;
>
> @@ -135,7 +139,10 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
> unsigned head;
> int i;
>
> - desc = kmalloc((out + in) * sizeof(struct vring_desc), gfp);
> + if ((out + in) <= vq->indirect_alloc_thresh)
> + desc = kmem_cache_alloc(vq->indirect_cache, gfp);
> + else
> + desc = kmalloc((out + in) * sizeof(struct vring_desc), gfp);
> if (!desc)
> return -ENOMEM;
>
> @@ -384,8 +391,13 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
> i = head;
>
> /* Free the indirect table */
> - if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT)
> - kfree(phys_to_virt(vq->vring.desc[i].addr));
> + if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT) {
> + if (vq->vring.desc[i].len > vq->indirect_alloc_thresh)
> + kfree(phys_to_virt(vq->vring.desc[i].addr));
> + else
> + kmem_cache_free(vq->indirect_cache,
> + phys_to_virt(vq->vring.desc[i].addr));
> + }
>
> while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) {
> i = vq->vring.desc[i].next;
> @@ -654,14 +666,20 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
> vq->last_used_idx = 0;
> vq->num_added = 0;
> vq->indirect_thresh = 0;
> + vq->indirect_alloc_thresh = 0;
> + vq->indirect_cache = NULL;
> list_add_tail(&vq->vq.list, &vdev->vqs);
> #ifdef DEBUG
> vq->in_use = false;
> vq->last_add_time_valid = false;
> #endif
>
> - if (virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC))
> + if (virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
> vq->indirect_thresh = vdev->indirect_thresh;
> + vq->indirect_alloc_thresh = vdev->indirect_alloc_thresh;
> + if (vq->indirect_alloc_thresh)
> + vq->indirect_cache = KMEM_CACHE(vring_desc[vq->indirect_alloc_thresh], 0);
> + }
>
> vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
>
> @@ -685,6 +703,8 @@ EXPORT_SYMBOL_GPL(vring_new_virtqueue);
> void vring_del_virtqueue(struct virtqueue *vq)
> {
> list_del(&vq->list);
> + if (to_vvq(vq)->indirect_cache)
> + kmem_cache_destroy(to_vvq(vq)->indirect_cache);
> kfree(to_vvq(vq));
> }
> EXPORT_SYMBOL_GPL(vring_del_virtqueue);
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 48bc457..3261c02 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -70,6 +70,7 @@ struct virtio_device {
> unsigned long features[1];
> void *priv;
> unsigned int indirect_thresh;
> + unsigned int indirect_alloc_thresh;
> };
>
> #define dev_to_virtio(dev) container_of(dev, struct virtio_device, dev)
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index fc93962..3044c86 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -55,6 +55,9 @@
> static unsigned int indirect_thresh;
> module_param(indirect_thresh, uint, S_IRUGO);
>
> +static unsigned int indirect_alloc_thresh;
> +module_param(indirect_alloc_thresh, uint, S_IRUGO);
> +
> /* a single mutex to manage channel initialization and attachment */
> static DEFINE_MUTEX(virtio_9p_lock);
> static DECLARE_WAIT_QUEUE_HEAD(vp_wq);
> @@ -505,6 +508,8 @@ static int p9_virtio_probe(struct virtio_device *vdev)
>
> /* We expect one virtqueue, for requests. */
> vdev->indirect_thresh = indirect_thresh;
> + vdev->indirect_alloc_thresh = indirect_alloc_thresh;
> +
> chan->vq = virtio_find_single_vq(vdev, req_done, "requests");
> if (IS_ERR(chan->vq)) {
> err = PTR_ERR(chan->vq);
> --
> 1.7.12
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Sasha Levin @ 2012-08-28 13:35 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: avi, linux-kernel, kvm, virtualization
In-Reply-To: <20120828132032.GB2039@redhat.com>
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
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH 3/5] virtio-scsi: allocate target pointers in a separate memory block
From: Sasha Levin @ 2012-08-28 14:07 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, linux-scsi, mst, linux-kernel, virtualization
In-Reply-To: <1346154857-12487-4-git-send-email-pbonzini@redhat.com>
On 08/28/2012 01:54 PM, Paolo Bonzini wrote:
> We will place the request virtqueues in the flexible array member.
>
> Refining the virtqueue API would let us drop the sglist copy, at
> which point the pointer-to-array-of-pointers can become a simple
> pointer-to-array. It would both simplify the allocation and remove a
> dereference in several hot paths.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> drivers/scsi/virtio_scsi.c | 23 +++++++++++++++--------
> 1 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 595af1a..62fec04 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -77,7 +77,7 @@ struct virtio_scsi {
> /* Get some buffers ready for event vq */
> struct virtio_scsi_event_node event_list[VIRTIO_SCSI_EVENT_LEN];
>
> - struct virtio_scsi_target_state *tgt[];
> + struct virtio_scsi_target_state **tgt;
> };
>
> static struct kmem_cache *virtscsi_cmd_cache;
> @@ -615,10 +615,13 @@ static void virtscsi_remove_vqs(struct virtio_device *vdev)
> /* Stop all the virtqueues. */
> vdev->config->reset(vdev);
>
> - num_targets = sh->max_id;
> - for (i = 0; i < num_targets; i++) {
> - kfree(vscsi->tgt[i]);
> - vscsi->tgt[i] = NULL;
> + if (vscsi->tgt) {
> + num_targets = sh->max_id;
> + for (i = 0; i < num_targets; i++) {
> + kfree(vscsi->tgt[i]);
Since we now kmalloc() the vscsi->tgt array, it doesn't get zeroed anymore.
This means that if for example, num_targets=3, and the second
virtscsi_alloc_tgt() in virtscsi_init() failed, we're going to kfree() garbage here.
Thanks,
Sasha
> + vscsi->tgt[i] = NULL;
> + }
> + kfree(vscsi->tgt);
> }
> vdev->config->del_vqs(vdev);
> @@ -660,6 +663,12 @@ static int virtscsi_init(struct virtio_device *vdev,
> /* We need to know how many segments before we allocate. */
> sg_elems = virtscsi_config_get(vdev, seg_max) ?: 1;
>
> + vscsi->tgt = kmalloc(num_targets *
> + sizeof(struct virtio_scsi_target_state *), GFP_KERNEL);
> + if (!vscsi->tgt) {
> + err = -ENOMEM;
> + goto out;
> + }
> for (i = 0; i < num_targets; i++) {
> vscsi->tgt[i] = virtscsi_alloc_tgt(vdev, sg_elems);
> if (!vscsi->tgt[i]) {
> @@ -685,9 +694,7 @@ static int __devinit virtscsi_probe(struct virtio_device *vdev)
>
> /* Allocate memory and link the structs together. */
> num_targets = virtscsi_config_get(vdev, max_target) + 1;
> - shost = scsi_host_alloc(&virtscsi_host_template,
> - sizeof(*vscsi)
> - + num_targets * sizeof(struct virtio_scsi_target_state));
> + shost = scsi_host_alloc(&virtscsi_host_template, sizeof(*vscsi));
>
> if (!shost)
> return -ENOMEM;
>
^ permalink raw reply
* Re: [PATCH 3/5] virtio-scsi: allocate target pointers in a separate memory block
From: Paolo Bonzini @ 2012-08-28 14:25 UTC (permalink / raw)
To: Sasha Levin; +Cc: kvm, linux-scsi, mst, linux-kernel, virtualization
In-Reply-To: <503CD097.2020900@gmail.com>
Il 28/08/2012 16:07, Sasha Levin ha scritto:
>> > - num_targets = sh->max_id;
>> > - for (i = 0; i < num_targets; i++) {
>> > - kfree(vscsi->tgt[i]);
>> > - vscsi->tgt[i] = NULL;
>> > + if (vscsi->tgt) {
>> > + num_targets = sh->max_id;
>> > + for (i = 0; i < num_targets; i++) {
>> > + kfree(vscsi->tgt[i]);
> Since we now kmalloc() the vscsi->tgt array, it doesn't get zeroed anymore.
>
> This means that if for example, num_targets=3, and the second
> virtscsi_alloc_tgt() in virtscsi_init() failed, we're going to kfree() garbage here.
Right, changed to kzalloc.
Paolo
^ permalink raw reply
* Re: [PATCH v9 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-28 15:23 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Peter Zijlstra, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120827202834.GC6517@t510.redhat.com>
On Mon, Aug 27, 2012 at 05:28:35PM -0300, Rafael Aquini wrote:
> On Sun, Aug 26, 2012 at 10:55:58AM +0300, Michael S. Tsirkin wrote:
> > On Sat, Aug 25, 2012 at 02:24:56AM -0300, Rafael Aquini wrote:
> > > Memory fragmentation introduced by ballooning might reduce significantly
> > > the number of 2MB contiguous memory blocks that can be used within a guest,
> > > thus imposing performance penalties associated with the reduced number of
> > > transparent huge pages that could be used by the guest workload.
> > >
> > > This patch introduces a common interface to help a balloon driver on
> > > making its page set movable to compaction, and thus allowing the system
> > > to better leverage the compation efforts on memory defragmentation.
> > >
> > > Signed-off-by: Rafael Aquini <aquini@redhat.com>
> >
> > Tons of rcu uses but not sync in sight. This looks suspicious.
>
> There's no critical section marked with rcu_read_lock/rcu_read_unlock. that's
> why there's no call for sync anywhere. As we are behaving mostly as updaters,
> the hole rcu usage is awkward and it's placed basically to enforce the proper
> order. To avoid hurting the RCU API usage with this awk approach I'll drop it
> for the next series submission (it will use barriers instead).
If everything is under page lock, barriers are likely not required.
If not, they might not be sufficient.
--
MST
^ permalink raw reply
* Re: [PATCH v9 3/5] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-28 15:54 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Peter Zijlstra, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120827194713.GA6517@t510.redhat.com>
On Mon, Aug 27, 2012 at 04:47:13PM -0300, Rafael Aquini wrote:
> On Sun, Aug 26, 2012 at 10:42:44AM +0300, Michael S. Tsirkin wrote:
> >
> > Reading two atomics and doing math? Result can even be negative.
> > I did not look at use closely but it looks suspicious.
> Doc on atomic_read says:
> "
> The read is atomic in that the return value is guaranteed to be one of the
> values initialized or modified with the interface operations if a proper
> implicit or explicit memory barrier is used after possible runtime
> initialization by any other thread and the value is modified only with the
> interface operations.
> "
>
> There's no runtime init by other thread than balloon's itself at device register,
> and the operations (inc, dec) are made by the proper interface operations
> only when protected by the spinlock pages_lock. It does not look suspicious, IMHO.
Any use of multiple atomics is suspicious.
Please just avoid it if you can. What's wrong with locking?
> I'm failing to see how it could become a negative on that case, since you cannot
> isolate more pages than what was previoulsy inflated to balloon's list.
There is no order guarantee. So in
A - B you can read B long after both A and B has been incremented.
Maybe it is safe in this case but it needs careful documentation
to explain how ordering works. Much easier to keep it all simple.
>
> > It's already the case everywhere except __wait_on_isolated_pages,
> > so just fix that, and then we can keep using int instead of atomics.
> >
> Sorry, I quite didn't get you here. fix what?
It's in the text you removed above. Access values under lock.
>
> > That's 1K on stack - and can become more if we increase
> > VIRTIO_BALLOON_ARRAY_PFNS_MAX. Probably too much - this is the reason
> > we use vb->pfns.
> >
> If we want to use vb->pfns we'll have to make leak_balloon mutual exclusive with
> page migration (as it was before), but that will inevictably bring us back to
> the discussion on breaking the loop when isolated pages make leak_balloon find
> less pages than it wants to release at each leak round.
>
I don't think this is an issue. The issue was busy waiting in that case.
--
MST
^ permalink raw reply
* Re: [PATCH v9 3/5] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-08-28 17:37 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Peter Zijlstra, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120828155410.GE2903@redhat.com>
On Tue, Aug 28, 2012 at 06:54:10PM +0300, Michael S. Tsirkin wrote:
> On Mon, Aug 27, 2012 at 04:47:13PM -0300, Rafael Aquini wrote:
> > On Sun, Aug 26, 2012 at 10:42:44AM +0300, Michael S. Tsirkin wrote:
> > >
> > > Reading two atomics and doing math? Result can even be negative.
> > > I did not look at use closely but it looks suspicious.
> > Doc on atomic_read says:
> > "
> > The read is atomic in that the return value is guaranteed to be one of the
> > values initialized or modified with the interface operations if a proper
> > implicit or explicit memory barrier is used after possible runtime
> > initialization by any other thread and the value is modified only with the
> > interface operations.
> > "
> >
> > There's no runtime init by other thread than balloon's itself at device register,
> > and the operations (inc, dec) are made by the proper interface operations
> > only when protected by the spinlock pages_lock. It does not look suspicious, IMHO.
>
> Any use of multiple atomics is suspicious.
> Please just avoid it if you can. What's wrong with locking?
>
> > I'm failing to see how it could become a negative on that case, since you cannot
> > isolate more pages than what was previoulsy inflated to balloon's list.
>
> There is no order guarantee. So in
> A - B you can read B long after both A and B has been incremented.
> Maybe it is safe in this case but it needs careful documentation
> to explain how ordering works. Much easier to keep it all simple.
>
> >
> > > It's already the case everywhere except __wait_on_isolated_pages,
> > > so just fix that, and then we can keep using int instead of atomics.
> > >
> > Sorry, I quite didn't get you here. fix what?
>
> It's in the text you removed above. Access values under lock.
>
So, you prefer this way:
/*
* __wait_on_isolated_pages - check if leak_balloon() must wait on isolated
* pages before proceeding with the page release.
* @vb : pointer to the struct virtio_balloon describing this device.
* @leak_target: how many pages we are attempting to release this round.
*/
static inline void __wait_on_isolated_pages(struct virtio_balloon *vb,
size_t leak_target)
{
unsigned int num_pages, isolated_pages;
spin_lock(&vb->pages_lock);
num_pages = vb->num_pages;
isolated_pages = vb->num_isolated_pages;
spin_unlock(&vb->pages_lock);
/*
* If isolated pages are making our leak target bigger than the
* total pages that we can release this round. Let's wait for
* migration returning enough pages back to balloon's list.
*/
wait_event(vb->config_change,
(!isolated_pages ||
leak_target <= (num_pages - isolated_pages)));
}
?
> >
> > > That's 1K on stack - and can become more if we increase
> > > VIRTIO_BALLOON_ARRAY_PFNS_MAX. Probably too much - this is the reason
> > > we use vb->pfns.
> > >
> > If we want to use vb->pfns we'll have to make leak_balloon mutual exclusive with
> > page migration (as it was before), but that will inevictably bring us back to
> > the discussion on breaking the loop when isolated pages make leak_balloon find
> > less pages than it wants to release at each leak round.
> >
>
> I don't think this is an issue. The issue was busy waiting in that case.
>
But, in fact, it is.
As we couldn't drop the mutex that prevents migration from happening, otherwise
the migration threads would screw up with our vb->pfns array, there will be no point
on keep waiting for isolated pages being reinserted on balloon's list, cause the
migration threads that will accomplish that task are also waiting on us dropping
the mutex.
You may argue that we could flag virtballoon_migratepage() to give up and return
before even trying to aquire the mutex, if a leak is ongoing -- deferring work
to virtballoon_putbackpage(). However, I'm eager to think that for this case,
the CPU time we spent isolating pages for compaction would be simply wasted and,
perhaps, no effective compaction was even reached.
And that makes me think it would have been better to stick with the old logics of
breaking the loop since leak_balloon(), originally, also remains busy waiting
while pursuing its target, anyway.
That's the trade here, IMO. If one really wants to wait on potentially isolated
pages getting back to the list before proceeding, we'll have to burn a little
more stack space with local variables, unfortunately.
^ permalink raw reply
* Re: [PATCH v9 3/5] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-28 17:57 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Peter Zijlstra, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120828173713.GA1750@t510.redhat.com>
On Tue, Aug 28, 2012 at 02:37:13PM -0300, Rafael Aquini wrote:
> On Tue, Aug 28, 2012 at 06:54:10PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Aug 27, 2012 at 04:47:13PM -0300, Rafael Aquini wrote:
> > > On Sun, Aug 26, 2012 at 10:42:44AM +0300, Michael S. Tsirkin wrote:
> > > >
> > > > Reading two atomics and doing math? Result can even be negative.
> > > > I did not look at use closely but it looks suspicious.
> > > Doc on atomic_read says:
> > > "
> > > The read is atomic in that the return value is guaranteed to be one of the
> > > values initialized or modified with the interface operations if a proper
> > > implicit or explicit memory barrier is used after possible runtime
> > > initialization by any other thread and the value is modified only with the
> > > interface operations.
> > > "
> > >
> > > There's no runtime init by other thread than balloon's itself at device register,
> > > and the operations (inc, dec) are made by the proper interface operations
> > > only when protected by the spinlock pages_lock. It does not look suspicious, IMHO.
> >
> > Any use of multiple atomics is suspicious.
> > Please just avoid it if you can. What's wrong with locking?
> >
> > > I'm failing to see how it could become a negative on that case, since you cannot
> > > isolate more pages than what was previoulsy inflated to balloon's list.
> >
> > There is no order guarantee. So in
> > A - B you can read B long after both A and B has been incremented.
> > Maybe it is safe in this case but it needs careful documentation
> > to explain how ordering works. Much easier to keep it all simple.
> >
> > >
> > > > It's already the case everywhere except __wait_on_isolated_pages,
> > > > so just fix that, and then we can keep using int instead of atomics.
> > > >
> > > Sorry, I quite didn't get you here. fix what?
> >
> > It's in the text you removed above. Access values under lock.
> >
>
> So, you prefer this way:
>
> /*
> * __wait_on_isolated_pages - check if leak_balloon() must wait on isolated
> * pages before proceeding with the page release.
> * @vb : pointer to the struct virtio_balloon describing this device.
> * @leak_target: how many pages we are attempting to release this round.
> */
> static inline void __wait_on_isolated_pages(struct virtio_balloon *vb,
> size_t leak_target)
> {
> unsigned int num_pages, isolated_pages;
> spin_lock(&vb->pages_lock);
> num_pages = vb->num_pages;
> isolated_pages = vb->num_isolated_pages;
> spin_unlock(&vb->pages_lock);
> /*
> * If isolated pages are making our leak target bigger than the
> * total pages that we can release this round. Let's wait for
> * migration returning enough pages back to balloon's list.
> */
> wait_event(vb->config_change,
> (!isolated_pages ||
> leak_target <= (num_pages - isolated_pages)));
This logic looks strange too - it does not 100% match the comment.
> }
>
> ?
Except that it does not work. You need to do the lock/unlock
dance and retest within wait_event.
> > >
> > > > That's 1K on stack - and can become more if we increase
> > > > VIRTIO_BALLOON_ARRAY_PFNS_MAX. Probably too much - this is the reason
> > > > we use vb->pfns.
> > > >
> > > If we want to use vb->pfns we'll have to make leak_balloon mutual exclusive with
> > > page migration (as it was before), but that will inevictably bring us back to
> > > the discussion on breaking the loop when isolated pages make leak_balloon find
> > > less pages than it wants to release at each leak round.
> > >
> >
> > I don't think this is an issue. The issue was busy waiting in that case.
> >
> But, in fact, it is.
> As we couldn't drop the mutex that prevents migration from happening, otherwise
> the migration threads would screw up with our vb->pfns array, there will be no point
> on keep waiting for isolated pages being reinserted on balloon's list, cause the
> migration threads that will accomplish that task are also waiting on us dropping
> the mutex.
>
> You may argue that we could flag virtballoon_migratepage() to give up and return
> before even trying to aquire the mutex, if a leak is ongoing -- deferring work
> to virtballoon_putbackpage(). However, I'm eager to think that for this case,
> the CPU time we spent isolating pages for compaction would be simply wasted and,
> perhaps, no effective compaction was even reached.
> And that makes me think it would have been better to stick with the old logics of
> breaking the loop since leak_balloon(), originally, also remains busy waiting
> while pursuing its target, anyway.
>
> That's the trade here, IMO. If one really wants to wait on potentially isolated
> pages getting back to the list before proceeding, we'll have to burn a little
> more stack space with local variables, unfortunately.
Sorry I do not understand what you are saying here. So find
a different locking strategy.
For example something like:
wait_event(vb->config_change,
({
lock
if (target <= (num_pages - isolated_pages))
leak balloon
cond = target <= (num_pages - isolated_pages));
unlock;
cond;
})
)
seems to have no issues?
--
MST
^ permalink raw reply
* Re: [PATCH v9 3/5] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-08-28 18:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Peter Zijlstra, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120828175716.GA4595@redhat.com>
On Tue, Aug 28, 2012 at 08:57:16PM +0300, Michael S. Tsirkin wrote:
> Sorry I do not understand what you are saying here. So find
> a different locking strategy.
>
> For example something like:
>
> wait_event(vb->config_change,
> ({
> lock
> if (target <= (num_pages - isolated_pages))
> leak balloon
> cond = target <= (num_pages - isolated_pages));
> unlock;
> cond;
> })
> )
>
> seems to have no issues?
Ok, I see what you mean. I'll change it.
^ permalink raw reply
* Re: [PATCH 1/5] virtio-ring: move queue_index to vring_virtqueue
From: Jason Wang @ 2012-08-29 7:54 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, linux-scsi, mst, linux-kernel, virtualization
In-Reply-To: <1346154857-12487-2-git-send-email-pbonzini@redhat.com>
On 08/28/2012 07:54 PM, Paolo Bonzini wrote:
> 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>
> ---
> I fixed the problems in Jason's v5 (posted at
> http://permalink.gmane.org/gmane.linux.kernel.virtualization/15910)
> and switched from virtio_set_queue_index to a new argument of
> vring_new_virtqueue. This breaks at compile-time any virtio
> transport that is not updated.
Thanks Paolo. I'm fine with this patch.
>
> drivers/lguest/lguest_device.c | 2 +-
> drivers/remoteproc/remoteproc_virtio.c | 2 +-
> drivers/s390/kvm/kvm_virtio.c | 2 +-
> drivers/virtio/virtio_mmio.c | 12 ++++--------
> drivers/virtio/virtio_pci.c | 13 +++++--------
> drivers/virtio/virtio_ring.c | 14 +++++++++++++-
> include/linux/virtio.h | 2 ++
> include/linux/virtio_ring.h | 3 ++-
> 8 files changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
> index 9e8388e..ccb7dfb 100644
> --- a/drivers/lguest/lguest_device.c
> +++ b/drivers/lguest/lguest_device.c
> @@ -296,7 +296,7 @@ static struct virtqueue *lg_find_vq(struct virtio_device *vdev,
> * to 'true': the host just a(nother) SMP CPU, so we only need inter-cpu
> * barriers.
> */
> - vq = vring_new_virtqueue(lvq->config.num, LGUEST_VRING_ALIGN, vdev,
> + vq = vring_new_virtqueue(index, lvq->config.num, LGUEST_VRING_ALIGN, vdev,
> true, lvq->pages, lg_notify, callback, name);
> if (!vq) {
> err = -ENOMEM;
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 3541b44..343c194 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -103,7 +103,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
> * Create the new vq, and tell virtio we're not interested in
> * the 'weak' smp barriers, since we're talking with a real device.
> */
> - vq = vring_new_virtqueue(len, rvring->align, vdev, false, addr,
> + vq = vring_new_virtqueue(id, len, rvring->align, vdev, false, addr,
> rproc_virtio_notify, callback, name);
> if (!vq) {
> dev_err(dev, "vring_new_virtqueue %s failed\n", name);
> diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
> index 47cccd5..5565af2 100644
> --- a/drivers/s390/kvm/kvm_virtio.c
> +++ b/drivers/s390/kvm/kvm_virtio.c
> @@ -198,7 +198,7 @@ static struct virtqueue *kvm_find_vq(struct virtio_device *vdev,
> if (err)
> goto out;
>
> - vq = vring_new_virtqueue(config->num, KVM_S390_VIRTIO_RING_ALIGN,
> + vq = vring_new_virtqueue(index, config->num, KVM_S390_VIRTIO_RING_ALIGN,
> vdev, true, (void *) config->address,
> kvm_notify, callback, name);
> if (!vq) {
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 453db0c..008bf58 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -131,9 +131,6 @@ struct virtio_mmio_vq_info {
> /* the number of entries in the queue */
> unsigned int num;
>
> - /* the index of the queue */
> - int queue_index;
> -
> /* the virtual address of the ring queue */
> void *queue;
>
> @@ -225,11 +222,10 @@ static void vm_reset(struct virtio_device *vdev)
> static void vm_notify(struct virtqueue *vq)
> {
> struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
> - struct virtio_mmio_vq_info *info = vq->priv;
>
> /* We write the queue's selector into the notification register to
> * signal the other end */
> - writel(info->queue_index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> + writel(virtqueue_get_queue_index(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> }
>
> /* Notify all virtqueues on an interrupt. */
> @@ -270,6 +266,7 @@ static void vm_del_vq(struct virtqueue *vq)
> struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
> struct virtio_mmio_vq_info *info = vq->priv;
> unsigned long flags, size;
> + unsigned int index = virtqueue_get_queue_index(vq);
>
> spin_lock_irqsave(&vm_dev->lock, flags);
> list_del(&info->node);
> @@ -278,7 +275,7 @@ static void vm_del_vq(struct virtqueue *vq)
> vring_del_virtqueue(vq);
>
> /* Select and deactivate the queue */
> - writel(info->queue_index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
> + writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
> writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
>
> size = PAGE_ALIGN(vring_size(info->num, VIRTIO_MMIO_VRING_ALIGN));
> @@ -324,7 +321,6 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
> err = -ENOMEM;
> goto error_kmalloc;
> }
> - info->queue_index = index;
>
> /* Allocate pages for the queue - start with a queue as big as
> * possible (limited by maximum size allowed by device), drop down
> @@ -356,7 +352,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
> vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
>
> /* Create the vring */
> - vq = vring_new_virtqueue(info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
> + vq = vring_new_virtqueue(index, info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
> true, info->queue, vm_notify, callback, name);
> if (!vq) {
> err = -ENOMEM;
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 2e03d41..d902464 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -79,9 +79,6 @@ struct virtio_pci_vq_info
> /* the number of entries in the queue */
> int num;
>
> - /* the index of the queue */
> - int queue_index;
> -
> /* the virtual address of the ring queue */
> void *queue;
>
> @@ -202,11 +199,11 @@ static void vp_reset(struct virtio_device *vdev)
> static void vp_notify(struct virtqueue *vq)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> - struct virtio_pci_vq_info *info = vq->priv;
>
> /* we write the queue's selector into the notification register to
> * signal the other end */
> - iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
> + iowrite16(virtqueue_get_queue_index(vq),
> + vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
> }
>
> /* Handle a configuration change: Tell driver if it wants to know. */
> @@ -402,7 +399,6 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
> if (!info)
> return ERR_PTR(-ENOMEM);
>
> - info->queue_index = index;
> info->num = num;
> info->msix_vector = msix_vec;
>
> @@ -418,7 +414,7 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
> vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
>
> /* create the vring */
> - vq = vring_new_virtqueue(info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
> + vq = vring_new_virtqueue(index, info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
> true, info->queue, vp_notify, callback, name);
> if (!vq) {
> err = -ENOMEM;
> @@ -467,7 +463,8 @@ static void vp_del_vq(struct virtqueue *vq)
> list_del(&info->node);
> spin_unlock_irqrestore(&vp_dev->lock, flags);
>
> - iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
> + iowrite16(virtqueue_get_queue_index(vq),
> + vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
>
> if (vp_dev->msix_enabled) {
> iowrite16(VIRTIO_MSI_NO_VECTOR,
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 5aa43c3..e639584 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -106,6 +106,9 @@ struct vring_virtqueue
> /* How to notify other side. FIXME: commonalize hcalls! */
> void (*notify)(struct virtqueue *vq);
>
> + /* Index of the queue */
> + int queue_index;
> +
> #ifdef DEBUG
> /* They're supposed to lock for us. */
> unsigned int in_use;
> @@ -171,6 +174,13 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
> return head;
> }
>
> +int virtqueue_get_queue_index(struct virtqueue *_vq)
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + return vq->queue_index;
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_get_queue_index);
> +
> /**
> * virtqueue_add_buf - expose buffer to other end
> * @vq: the struct virtqueue we're talking about.
> @@ -616,7 +626,8 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
> }
> EXPORT_SYMBOL_GPL(vring_interrupt);
>
> -struct virtqueue *vring_new_virtqueue(unsigned int num,
> +struct virtqueue *vring_new_virtqueue(unsigned int index,
> + unsigned int num,
> unsigned int vring_align,
> struct virtio_device *vdev,
> bool weak_barriers,
> @@ -647,6 +658,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
> vq->broken = false;
> vq->last_used_idx = 0;
> vq->num_added = 0;
> + vq->queue_index = index;
> list_add_tail(&vq->vq.list,&vdev->vqs);
> #ifdef DEBUG
> vq->in_use = false;
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index a1ba8bb..533b115 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -50,6 +50,8 @@ void *virtqueue_detach_unused_buf(struct virtqueue *vq);
>
> unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
>
> +int virtqueue_get_queue_index(struct virtqueue *vq);
> +
> /**
> * virtio_device - representation of a device using virtio
> * @index: unique position on the virtio bus
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> index e338730..c2d793a 100644
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -165,7 +165,8 @@ static inline int vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old)
> struct virtio_device;
> struct virtqueue;
>
> -struct virtqueue *vring_new_virtqueue(unsigned int num,
> +struct virtqueue *vring_new_virtqueue(unsigned int index,
> + unsigned int num,
> unsigned int vring_align,
> struct virtio_device *vdev,
> bool weak_barriers,
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Michael S. Tsirkin @ 2012-08-29 11:07 UTC (permalink / raw)
To: Sasha Levin; +Cc: avi, linux-kernel, kvm, virtualization
In-Reply-To: <503CC904.3050207@gmail.com>
On Tue, Aug 28, 2012 at 03:35:00PM +0200, Sasha Levin wrote:
> 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
>
>
> Thanks,
> Sasha
This is with both patches 1 + 2?
Sorry could you please also test what happens if you apply
- just patch 1
- just patch 2
Thanks!
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Sasha Levin @ 2012-08-29 15:03 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: avi, linux-kernel, kvm, virtualization
In-Reply-To: <20120829110748.GB5970@redhat.com>
On 08/29/2012 01:07 PM, Michael S. Tsirkin wrote:
> On Tue, Aug 28, 2012 at 03:35:00PM +0200, Sasha Levin wrote:
>> 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
>>
>>
>> Thanks,
>> Sasha
>
> This is with both patches 1 + 2?
> Sorry could you please also test what happens if you apply
> - just patch 1
> - just patch 2
>
> Thanks!
Sure thing!
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.
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Michael S. Tsirkin @ 2012-08-29 15:14 UTC (permalink / raw)
To: Sasha Levin; +Cc: avi, linux-kernel, kvm, virtualization
In-Reply-To: <503E2F27.5060904@gmail.com>
On Wed, Aug 29, 2012 at 05:03:03PM +0200, Sasha Levin wrote:
> On 08/29/2012 01:07 PM, Michael S. Tsirkin wrote:
> > On Tue, Aug 28, 2012 at 03:35:00PM +0200, Sasha Levin wrote:
> >> 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
> >>
> >>
> >> Thanks,
> >> Sasha
> >
> > This is with both patches 1 + 2?
> > Sorry could you please also test what happens if you apply
> > - just patch 1
> > - just patch 2
> >
> > Thanks!
>
> Sure thing!
>
> 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:
OK so let us set it to 16 for virtio-net by default then?
> 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.
>
>
> Thanks,
> Sasha
Checking that host CPU utilization did not jump would be nice.
E.g. measure BW/host CPU.
--
MST
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Michael S. Tsirkin @ 2012-08-29 15:38 UTC (permalink / raw)
To: Sasha Levin; +Cc: avi, linux-kernel, kvm, virtualization
In-Reply-To: <1346159043-16446-2-git-send-email-levinsasha928@gmail.com>
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>
The API is ugly - how does driver know what to set?
Is this a typical request size? Then let's call it that.
Also this is really per VQ, right?
What is a good default for net? I guess max sg?
> ---
>
> Changes in v2:
>
> - Free correctly indirect buffers.
>
> drivers/block/virtio_blk.c | 4 ++++
> drivers/char/hw_random/virtio-rng.c | 4 ++++
> drivers/char/virtio_console.c | 4 ++++
> drivers/net/virtio_net.c | 4 ++++
> drivers/virtio/virtio_balloon.c | 4 ++++
> drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++++----
> include/linux/virtio.h | 1 +
> net/9p/trans_virtio.c | 5 +++++
> 8 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 13b8ae9..7f670af 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -25,6 +25,9 @@ struct workqueue_struct *virtblk_wq;
> static unsigned int indirect_thresh;
> module_param(indirect_thresh, uint, S_IRUGO);
>
> +static unsigned int indirect_alloc_thresh;
> +module_param(indirect_alloc_thresh, uint, S_IRUGO);
> +
> struct virtio_blk
> {
> struct virtio_device *vdev;
> @@ -739,6 +742,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
> INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
> vblk->config_enable = true;
> vdev->indirect_thresh = indirect_thresh;
> + vdev->indirect_alloc_thresh = indirect_alloc_thresh;
>
> err = init_vq(vblk);
> if (err)
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index 02d8421..8475ece 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -28,6 +28,9 @@
> static unsigned int indirect_thresh;
> module_param(indirect_thresh, uint, S_IRUGO);
>
> +static unsigned int indirect_alloc_thresh;
> +module_param(indirect_alloc_thresh, uint, S_IRUGO);
> +
> static struct virtqueue *vq;
> static unsigned int data_avail;
> static DECLARE_COMPLETION(have_data);
> @@ -97,6 +100,7 @@ static int probe_common(struct virtio_device *vdev)
>
> /* We expect a single virtqueue. */
> vdev->indirect_thresh = indirect_thresh;
> + vdev->indirect_alloc_thresh = indirect_alloc_thresh;
> vq = virtio_find_single_vq(vdev, random_recv_done, "input");
> if (IS_ERR(vq))
> return PTR_ERR(vq);
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fc14e7f..c6f24a7 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -42,6 +42,9 @@
> static unsigned int indirect_thresh;
> module_param(indirect_thresh, uint, S_IRUGO);
>
> +static unsigned int indirect_alloc_thresh;
> +module_param(indirect_alloc_thresh, uint, S_IRUGO);
> +
> /*
> * This is a global struct for storing common data for all the devices
> * this driver handles.
> @@ -1891,6 +1894,7 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
> &portdev->config.max_nr_ports) == 0)
> multiport = true;
> vdev->indirect_thresh = indirect_thresh;
> + vdev->indirect_alloc_thresh = indirect_alloc_thresh;
>
> err = init_vqs(portdev);
> if (err < 0) {
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 64a8321..c091efd 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -37,6 +37,9 @@ module_param(gso, bool, 0444);
> static unsigned int indirect_thresh;
> module_param(indirect_thresh, uint, S_IRUGO);
>
> +static unsigned int indirect_alloc_thresh;
> +module_param(indirect_alloc_thresh, uint, S_IRUGO);
> +
> /* FIXME: MTU in config. */
> #define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> #define GOOD_COPY_LEN 128
> @@ -1132,6 +1135,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
> vi->mergeable_rx_bufs = true;
> vdev->indirect_thresh = indirect_thresh;
> + vdev->indirect_alloc_thresh = indirect_alloc_thresh;
>
> err = init_vqs(vi);
> if (err)
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index fce7347..ccf87db 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -38,6 +38,9 @@
> static unsigned int indirect_thresh;
> module_param(indirect_thresh, uint, S_IRUGO);
>
> +static unsigned int indirect_alloc_thresh;
> +module_param(indirect_alloc_thresh, uint, S_IRUGO);
> +
> struct virtio_balloon
> {
> struct virtio_device *vdev;
> @@ -360,6 +363,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> vb->vdev = vdev;
> vb->need_stats_update = 0;
> vdev->indirect_thresh = indirect_thresh;
> + vdev->indirect_alloc_thresh = indirect_alloc_thresh;
>
> err = init_vqs(vb);
> if (err)
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 99a64a7..e8b9c54 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -93,6 +93,10 @@ struct vring_virtqueue
> */
> unsigned int indirect_thresh;
>
> + /* Buffers below this size will be allocated from cache */
> + unsigned int indirect_alloc_thresh;
> + struct kmem_cache *indirect_cache;
> +
> /* Host publishes avail event idx */
> bool event;
>
> @@ -135,7 +139,10 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
> unsigned head;
> int i;
>
> - desc = kmalloc((out + in) * sizeof(struct vring_desc), gfp);
> + if ((out + in) <= vq->indirect_alloc_thresh)
> + desc = kmem_cache_alloc(vq->indirect_cache, gfp);
> + else
> + desc = kmalloc((out + in) * sizeof(struct vring_desc), gfp);
> if (!desc)
> return -ENOMEM;
So this means that drivers need to know how large each
descriptor is to avoid trying to allocate 32Mbyte chunks :)
>
> @@ -384,8 +391,13 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
> i = head;
>
> /* Free the indirect table */
> - if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT)
> - kfree(phys_to_virt(vq->vring.desc[i].addr));
> + if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT) {
> + if (vq->vring.desc[i].len > vq->indirect_alloc_thresh)
This looks wrong. indirect_alloc_thresh is in entries but len is in
bytes, isn't it?
> + kfree(phys_to_virt(vq->vring.desc[i].addr));
> + else
> + kmem_cache_free(vq->indirect_cache,
> + phys_to_virt(vq->vring.desc[i].addr));
> + }
>
> while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) {
> i = vq->vring.desc[i].next;
> @@ -654,14 +666,20 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
> vq->last_used_idx = 0;
> vq->num_added = 0;
> vq->indirect_thresh = 0;
> + vq->indirect_alloc_thresh = 0;
> + vq->indirect_cache = NULL;
> list_add_tail(&vq->vq.list, &vdev->vqs);
> #ifdef DEBUG
> vq->in_use = false;
> vq->last_add_time_valid = false;
> #endif
>
> - if (virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC))
> + if (virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
> vq->indirect_thresh = vdev->indirect_thresh;
> + vq->indirect_alloc_thresh = vdev->indirect_alloc_thresh;
> + if (vq->indirect_alloc_thresh)
> + vq->indirect_cache = KMEM_CACHE(vring_desc[vq->indirect_alloc_thresh],
> 0);
I am not a purist but this line looks way too long.
Also - no need to check cache creation succeeded?
On failure - disable caching?
Also - let's check that values are sane before passing them on?
They come from user after all.
Also - should not threshold be per VQ? E.g. for -net we do not
need the cache for RX unless in legacy big packet mode.
> + }
>
> vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
>
> @@ -685,6 +703,8 @@ EXPORT_SYMBOL_GPL(vring_new_virtqueue);
> void vring_del_virtqueue(struct virtqueue *vq)
> {
> list_del(&vq->list);
> + if (to_vvq(vq)->indirect_cache)
> + kmem_cache_destroy(to_vvq(vq)->indirect_cache);
> kfree(to_vvq(vq));
> }
> EXPORT_SYMBOL_GPL(vring_del_virtqueue);
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 48bc457..3261c02 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -70,6 +70,7 @@ struct virtio_device {
> unsigned long features[1];
> void *priv;
> unsigned int indirect_thresh;
> + unsigned int indirect_alloc_thresh;
> };
>
> #define dev_to_virtio(dev) container_of(dev, struct virtio_device, dev)
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index fc93962..3044c86 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -55,6 +55,9 @@
> static unsigned int indirect_thresh;
> module_param(indirect_thresh, uint, S_IRUGO);
>
> +static unsigned int indirect_alloc_thresh;
> +module_param(indirect_alloc_thresh, uint, S_IRUGO);
> +
> /* a single mutex to manage channel initialization and attachment */
> static DEFINE_MUTEX(virtio_9p_lock);
> static DECLARE_WAIT_QUEUE_HEAD(vp_wq);
> @@ -505,6 +508,8 @@ static int p9_virtio_probe(struct virtio_device *vdev)
>
> /* We expect one virtqueue, for requests. */
> vdev->indirect_thresh = indirect_thresh;
> + vdev->indirect_alloc_thresh = indirect_alloc_thresh;
> +
> chan->vq = virtio_find_single_vq(vdev, req_done, "requests");
> if (IS_ERR(chan->vq)) {
> err = PTR_ERR(chan->vq);
> --
> 1.7.12
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio-ring: Allocate indirect buffers from cache when possible
From: Michael S. Tsirkin @ 2012-08-29 15:38 UTC (permalink / raw)
To: Sasha Levin; +Cc: avi, linux-kernel, kvm, virtualization
In-Reply-To: <503E2F27.5060904@gmail.com>
On Wed, Aug 29, 2012 at 05:03:03PM +0200, Sasha Levin wrote:
> On 08/29/2012 01:07 PM, Michael S. Tsirkin wrote:
> > On Tue, Aug 28, 2012 at 03:35:00PM +0200, Sasha Levin wrote:
> >> 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
> >>
> >>
> >> Thanks,
> >> Sasha
> >
> > This is with both patches 1 + 2?
> > Sorry could you please also test what happens if you apply
> > - just patch 1
> > - just patch 2
> >
> > Thanks!
>
> Sure thing!
>
> 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.
>
>
> Thanks,
> Sasha
And which parameter did you use for patch 2?
^ 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