RE: [PATCH RFC kernel] balloon: speed up inflating/deflating process
> On Fri, 20 May 2016 17:59:46 +0800
> Liang Li wrote:
>
> > The implementation of the current virtio-balloon is not very
> > efficient, Bellow is test result of time spends on inflating the
> > balloon to 3GB of a 4GB idle guest:
> >
> > a. allocating pages (6.5%, 103ms)
> > b. sending PFNs to host (68.3%, 787ms) c. address translation (6.1%,
> > 96ms) d. madvise (19%, 300ms)
> >
> > It takes about 1577ms for the whole inflating process to complete. The
> > test shows that the bottle neck is the stage b and stage d.
> >
> > If using a bitmap to send the page info instead of the PFNs, we can
> > reduce the overhead spends on stage b quite a lot. Furthermore, it's
> > possible to do the address translation and do the madvise with a bulk
> > of pages, instead of the current page per page way, so the overhead of
> > stage c and stage d can also be reduced a lot.
> >
> > This patch is the kernel side implementation which is intended to
> > speed up the inflating & deflating process by adding a new feature to
> > the virtio-balloon device. And now, inflating the balloon to 3GB of a
> > 4GB idle guest only takes 175ms, it's about 9 times as fast as before.
> >
> > TODO: optimize stage a by allocating/freeing a chunk of pages instead
> > of a single page at a time.
>
> Not commenting on the approach, but...
>
> >
> > Signed-off-by: Liang Li
> > ---
> > drivers/virtio/virtio_balloon.c | 199
> ++--
> > include/uapi/linux/virtio_balloon.h | 1 +
> > mm/page_alloc.c | 6 ++
> > 3 files changed, 198 insertions(+), 8 deletions(-)
> >
>
> > static void tell_host(struct virtio_balloon *vb, struct virtqueue
> > *vq) {
> > - struct scatterlist sg;
> > unsigned int len;
> >
> > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> > + if (virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_PAGE_BITMAP)) {
> > + u32 page_shift = PAGE_SHIFT;
> > + unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > + struct scatterlist sg[5];
> > +
> > + start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > + end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > + bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> sizeof(long);
> > +
> > + sg_init_table(sg, 5);
> > + sg_set_buf(&sg[0], &flags, sizeof(flags));
> > + sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> > + sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> > + sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > + sg_set_buf(&sg[4], vb->page_bitmap +
> > +(start_pfn / BITS_PER_LONG), bmap_len);
> > + virtqueue_add_outbuf(vq, sg, 5, vb, GFP_KERNEL);
> > +
>
> ...you need to take care of the endianness of the data you put on the queue,
> otherwise virtio-1 on big endian won't work. (There's just been a patch for
> that problem.)
OK, thanks for your reminding.
Liang
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH RFC kernel] balloon: speed up inflating/deflating process
> On 20/05/2016 11:59, Liang Li wrote: > > + > > + sg_init_table(sg, 5); > > + sg_set_buf(&sg[0], &flags, sizeof(flags)); > > + sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn)); > > + sg_set_buf(&sg[2], &page_shift, sizeof(page_shift)); > > + sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len)); > > These four should probably be placed in a single struct and therefore a single > sg entry. It might even be faster to place it together with the bitmap, thus > avoiding the use of indirect descriptors. > Yes, thanks for your suggestion. > You should also test ballooning of a 64GB guest after filling in the page > cache, > not just ballooning of a freshly booted 4GB guest. This will give you a much > more sparse bitmap. Still, the improvement in sending PFNs to the host are I will include the test result for that case in next version. Thanks, Liang > impressive. > > Thanks, > > Paolo > > > + sg_set_buf(&sg[4], vb->page_bitmap + > > +(start_pfn / BITS_PER_LONG), bmap_len); > > + virtqueue_add_outbuf(vq, sg, 5, vb, GFP_KERNEL); ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH V3 2/3] vhost: convert pre sorted vhost memory array to interval tree
Current pre-sorted memory region array has some limitations for future
device IOTLB conversion:
1) need extra work for adding and removing a single region, and it's
expected to be slow because of sorting or memory re-allocation.
2) need extra work of removing a large range which may intersect
several regions with different size.
3) need trick for a replacement policy like LRU
To overcome the above shortcomings, this patch convert it to interval
tree which can easily address the above issue with almost no extra
work.
The patch could be used for:
- Extend the current API and only let the userspace to send diffs of
memory table.
- Simplify Device IOTLB implementation.
Signed-off-by: Jason Wang
---
drivers/vhost/net.c | 8 +--
drivers/vhost/vhost.c | 182 --
drivers/vhost/vhost.h | 27 ++--
3 files changed, 128 insertions(+), 89 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index beaeb17..a584239 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -1037,20 +1037,20 @@ static long vhost_net_reset_owner(struct vhost_net *n)
struct socket *tx_sock = NULL;
struct socket *rx_sock = NULL;
long err;
- struct vhost_memory *memory;
+ struct vhost_umem *umem;
mutex_lock(&n->dev.mutex);
err = vhost_dev_check_owner(&n->dev);
if (err)
goto done;
- memory = vhost_dev_reset_owner_prepare();
- if (!memory) {
+ umem = vhost_dev_reset_owner_prepare();
+ if (!umem) {
err = -ENOMEM;
goto done;
}
vhost_net_stop(n, &tx_sock, &rx_sock);
vhost_net_flush(n);
- vhost_dev_reset_owner(&n->dev, memory);
+ vhost_dev_reset_owner(&n->dev, umem);
vhost_net_vq_reset(n);
done:
mutex_unlock(&n->dev.mutex);
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 9f2a63a..166e779 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "vhost.h"
@@ -42,6 +43,10 @@ enum {
#define vhost_used_event(vq) ((__virtio16 __user *)&vq->avail->ring[vq->num])
#define vhost_avail_event(vq) ((__virtio16 __user *)&vq->used->ring[vq->num])
+INTERVAL_TREE_DEFINE(struct vhost_umem_node,
+rb, __u64, __subtree_last,
+START, LAST, , vhost_umem_interval_tree);
+
#ifdef CONFIG_VHOST_CROSS_ENDIAN_LEGACY
static void vhost_disable_cross_endian(struct vhost_virtqueue *vq)
{
@@ -300,10 +305,10 @@ static void vhost_vq_reset(struct vhost_dev *dev,
vq->call_ctx = NULL;
vq->call = NULL;
vq->log_ctx = NULL;
- vq->memory = NULL;
vhost_reset_is_le(vq);
vhost_disable_cross_endian(vq);
vq->busyloop_timeout = 0;
+ vq->umem = NULL;
}
static int vhost_worker(void *data)
@@ -407,7 +412,7 @@ void vhost_dev_init(struct vhost_dev *dev,
mutex_init(&dev->mutex);
dev->log_ctx = NULL;
dev->log_file = NULL;
- dev->memory = NULL;
+ dev->umem = NULL;
dev->mm = NULL;
spin_lock_init(&dev->work_lock);
INIT_LIST_HEAD(&dev->work_list);
@@ -512,27 +517,36 @@ err_mm:
}
EXPORT_SYMBOL_GPL(vhost_dev_set_owner);
-struct vhost_memory *vhost_dev_reset_owner_prepare(void)
+static void *vhost_kvzalloc(unsigned long size)
{
- return kmalloc(offsetof(struct vhost_memory, regions), GFP_KERNEL);
+ void *n = kzalloc(size, GFP_KERNEL | __GFP_NOWARN | __GFP_REPEAT);
+
+ if (!n)
+ n = vzalloc(size);
+ return n;
+}
+
+struct vhost_umem *vhost_dev_reset_owner_prepare(void)
+{
+ return vhost_kvzalloc(sizeof(struct vhost_umem));
}
EXPORT_SYMBOL_GPL(vhost_dev_reset_owner_prepare);
/* Caller should have device mutex */
-void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_memory *memory)
+void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_umem *umem)
{
int i;
vhost_dev_cleanup(dev, true);
/* Restore memory to default empty mapping. */
- memory->nregions = 0;
- dev->memory = memory;
+ INIT_LIST_HEAD(&umem->umem_list);
+ dev->umem = umem;
/* We don't need VQ locks below since vhost_dev_cleanup makes sure
* VQs aren't running.
*/
for (i = 0; i < dev->nvqs; ++i)
- dev->vqs[i]->memory = memory;
+ dev->vqs[i]->umem = umem;
}
EXPORT_SYMBOL_GPL(vhost_dev_reset_owner);
@@ -549,6 +563,21 @@ void vhost_dev_stop(struct vhost_dev *dev)
}
EXPORT_SYMBOL_GPL(vhost_dev_stop);
+static void vhost_umem_clean(struct vhost_umem *umem)
+{
+ struct vhost_umem_node *node, *tmp;
+
+ if (!umem)
+ return;
+
+ list_for_each_entry_safe(node, tmp, &umem->umem_list, link) {
+ vhost_umem_interval_tree_remove(node, &umem->umem_tree);
+ list_del(&node->link);
+
[RFC PATCH V3 1/3] vhost: introduce vhost memory accessors
This patch introduces vhost memory accessors which were just wrappers
for userspace address access helpers. This is a requirement for vhost
device iotlb implementation which will add iotlb translations in those
accessors.
Signed-off-by: Jason Wang
---
drivers/vhost/net.c | 1 +
drivers/vhost/vhost.c | 50 +++---
2 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index f744eeb..beaeb17 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -985,6 +985,7 @@ static long vhost_net_set_backend(struct vhost_net *n,
unsigned index, int fd)
vhost_net_disable_vq(n, vq);
vq->private_data = sock;
+ /* FIXME: iotlb prefetch here? */
r = vhost_vq_init_access(vq);
if (r)
goto err_used;
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 669fef1..9f2a63a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -651,6 +651,22 @@ static int memory_access_ok(struct vhost_dev *d, struct
vhost_memory *mem,
return 1;
}
+#define vhost_put_user(vq, x, ptr) __put_user(x, ptr)
+
+static int vhost_copy_to_user(struct vhost_virtqueue *vq, void *to,
+ const void *from, unsigned size)
+{
+ return __copy_to_user(to, from, size);
+}
+
+#define vhost_get_user(vq, x, ptr) __get_user(x, ptr)
+
+static int vhost_copy_from_user(struct vhost_virtqueue *vq, void *to,
+ void *from, unsigned size)
+{
+ return __copy_from_user(to, from, size);
+}
+
static int vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
struct vring_desc __user *desc,
struct vring_avail __user *avail,
@@ -1156,7 +1172,8 @@ EXPORT_SYMBOL_GPL(vhost_log_write);
static int vhost_update_used_flags(struct vhost_virtqueue *vq)
{
void __user *used;
- if (__put_user(cpu_to_vhost16(vq, vq->used_flags), &vq->used->flags) <
0)
+ if (vhost_put_user(vq, cpu_to_vhost16(vq, vq->used_flags),
+ &vq->used->flags) < 0)
return -EFAULT;
if (unlikely(vq->log_used)) {
/* Make sure the flag is seen before log. */
@@ -1174,7 +1191,8 @@ static int vhost_update_used_flags(struct vhost_virtqueue
*vq)
static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16
avail_event)
{
- if (__put_user(cpu_to_vhost16(vq, vq->avail_idx),
vhost_avail_event(vq)))
+ if (vhost_put_user(vq, cpu_to_vhost16(vq, vq->avail_idx),
+ vhost_avail_event(vq)))
return -EFAULT;
if (unlikely(vq->log_used)) {
void __user *used;
@@ -1212,7 +1230,7 @@ int vhost_vq_init_access(struct vhost_virtqueue *vq)
r = -EFAULT;
goto err;
}
- r = __get_user(last_used_idx, &vq->used->idx);
+ r = vhost_get_user(vq, last_used_idx, &vq->used->idx);
if (r)
goto err;
vq->last_used_idx = vhost16_to_cpu(vq, last_used_idx);
@@ -1392,7 +1410,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
/* Check it isn't doing very strange things with descriptor numbers. */
last_avail_idx = vq->last_avail_idx;
- if (unlikely(__get_user(avail_idx, &vq->avail->idx))) {
+ if (unlikely(vhost_get_user(vq, avail_idx, &vq->avail->idx))) {
vq_err(vq, "Failed to access avail idx at %p\n",
&vq->avail->idx);
return -EFAULT;
@@ -1414,8 +1432,8 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
/* Grab the next descriptor number they're advertising, and increment
* the index we've seen. */
- if (unlikely(__get_user(ring_head,
- &vq->avail->ring[last_avail_idx & (vq->num -
1)]))) {
+ if (unlikely(vhost_get_user(vq, ring_head,
+&vq->avail->ring[last_avail_idx & (vq->num - 1)]))) {
vq_err(vq, "Failed to read head: idx %d address %p\n",
last_avail_idx,
&vq->avail->ring[last_avail_idx % vq->num]);
@@ -1450,7 +1468,8 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
i, vq->num, head);
return -EINVAL;
}
- ret = __copy_from_user(&desc, vq->desc + i, sizeof desc);
+ ret = vhost_copy_from_user(vq, &desc, vq->desc + i,
+ sizeof desc);
if (unlikely(ret)) {
vq_err(vq, "Failed to get descriptor: idx %d addr %p\n",
i, vq->desc + i);
@@ -1538,15 +1557,15 @@ static int __vhost_add_used_n(struct vhost_virtqueue
*vq,
start = vq->last_used_idx & (vq->num - 1);
used = vq->used->ring + start;
if (count == 1)
[RFC PATCH V3 0/3] basic device IOTLB support
This patch tries to implement an device IOTLB for vhost. This could be used with for co-operation with userspace IOMMU implementation (qemu) for a secure DMA environment (DMAR) in guest. The idea is simple. When vhost meets an IOTLB miss, it will request the assistance of userspace to do the translation, this is done through: - when there's a IOTLB miss, it will notify userspace through vhost_net fd and then userspace read the fault address, size and access from vhost fd. - userspace write the translation result back to vhost fd, vhost can then update its IOTLB. The codes were optimized for fixed mapping users e.g dpdk in guest. It will be slow if dynamic mappings were used in guest. We could do optimizations on top. The codes were designed to be architecture independent. It should be easily ported to any architecture. Stress tested with l2fwd/vfio in guest with 4K/2M/1G page size. On 1G hugepage case, 100% TLB hit rate were noticed. Changes from V2: - introduce memory accessors for vhost - switch from ioctls to oridinary file read/write for iotlb miss and updating - do not assume virtqueue were virtually mapped contiguously, all virtqueue access were done throug IOTLB - verify memory access during IOTLB update and fail early - introduce a module parameter for the size of IOTLB Changes from V1: - support any size/range of updating and invalidation through introducing the interval tree. - convert from per device iotlb request to per virtqueue iotlb request, this solves the possible deadlock in V1. - read/write permission check support. Please review. Jason Wang (3): vhost: introduce vhost memory accessors vhost: convert pre sorted vhost memory array to interval tree vhost: device IOTLB API drivers/vhost/net.c| 63 +++- drivers/vhost/vhost.c | 760 ++--- drivers/vhost/vhost.h | 60 +++- include/uapi/linux/vhost.h | 28 ++ 4 files changed, 790 insertions(+), 121 deletions(-) -- 2.7.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH V3 3/3] vhost: device IOTLB API
This patch tries to implement an device IOTLB for vhost. This could be
used with for co-operation with userspace(qemu) implementation of DMA
remapping.
The idea is simple, cache the translation in a software device IOTLB
(interval tree) implementation in vhost and make vhost_net file
descriptor could be read or wrote by a process. When vhost meets an
IOTLB miss, the fault address, size and access could be read from the
file. After userspace finishes the translation, it write the
translated address to the vhost_net file to update the device IOTLB.
When device IOTLB (VHOST_F_DEVICE_IOTLB) is enabled all vq address
set by ioctl were treated as iova instead of virtual address and the
accessing could only be done through IOTLB instead of direct
userspace memory access. Before each rounds or vq processing, all vq
metadata were prefetched in device IOTLB to make sure no translation
fault happens during vq processing.
In most cases, virtqueue were mapped contiguous even in virtual
address. So the IOTLB translation for virtqueue itself maybe a little
bit slower. We can add fast path on top of this patch.
Signed-off-by: Jason Wang
---
drivers/vhost/net.c| 54 -
drivers/vhost/vhost.c | 586 +
drivers/vhost/vhost.h | 35 ++-
include/uapi/linux/vhost.h | 28 +++
4 files changed, 656 insertions(+), 47 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index a584239..0f45e3d 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -61,7 +61,8 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
enum {
VHOST_NET_FEATURES = VHOST_FEATURES |
(1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
-(1ULL << VIRTIO_NET_F_MRG_RXBUF)
+(1ULL << VIRTIO_NET_F_MRG_RXBUF) |
+(1ULL << VHOST_F_DEVICE_IOTLB)
};
enum {
@@ -308,7 +309,8 @@ static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
{
unsigned long uninitialized_var(endtime);
int r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
- out_num, in_num, NULL, NULL);
+ out_num, in_num, NULL, NULL,
+ VHOST_ACCESS_RO);
if (r == vq->num && vq->busyloop_timeout) {
preempt_disable();
@@ -318,7 +320,8 @@ static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
cpu_relax_lowlatency();
preempt_enable();
r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
- out_num, in_num, NULL, NULL);
+ out_num, in_num, NULL, NULL,
+ VHOST_ACCESS_RO);
}
return r;
@@ -351,6 +354,9 @@ static void handle_tx(struct vhost_net *net)
if (!sock)
goto out;
+ if (!vq_iotlb_prefetch(vq))
+ goto out;
+
vhost_disable_notify(&net->dev, vq);
hdr_size = nvq->vhost_hlen;
@@ -538,7 +544,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
}
r = vhost_get_vq_desc(vq, vq->iov + seg,
ARRAY_SIZE(vq->iov) - seg, &out,
- &in, log, log_num);
+ &in, log, log_num, VHOST_ACCESS_WO);
if (unlikely(r < 0))
goto err;
@@ -612,6 +618,10 @@ static void handle_rx(struct vhost_net *net)
sock = vq->private_data;
if (!sock)
goto out;
+
+ if (!vq_iotlb_prefetch(vq))
+ goto out;
+
vhost_disable_notify(&net->dev, vq);
vhost_hlen = nvq->vhost_hlen;
@@ -1085,6 +1095,11 @@ static int vhost_net_set_features(struct vhost_net *n,
u64 features)
mutex_unlock(&n->dev.mutex);
return -EFAULT;
}
+ if ((features & (1ULL << VHOST_F_DEVICE_IOTLB))) {
+ if (vhost_init_device_iotlb(&n->dev, true))
+ return -EFAULT;
+ }
+
for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
mutex_lock(&n->vqs[i].vq.mutex);
n->vqs[i].vq.acked_features = features;
@@ -1167,9 +1182,40 @@ static long vhost_net_compat_ioctl(struct file *f,
unsigned int ioctl,
}
#endif
+static ssize_t vhost_net_chr_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+ struct file *file = iocb->ki_filp;
+ struct vhost_net *n = file->private_data;
+ struct vhost_dev *dev = &n->dev;
+ int noblock = file->f_flags & O_NONBLOCK;
+
+ return vhost_chr_read_iter(dev, to, noblock);
+}
+
+static ssize_t vhost_net_chr_write_iter(struct kiocb *iocb,
+ struct iov_iter *from)
+{
+ struct file *file = iocb->ki_filp;
+ struct vhost_net *n = file->private_data;
+ struc
RE: [PATCH RFC kernel] balloon: speed up inflating/deflating process
> On Fri, May 20, 2016 at 05:59:46PM +0800, Liang Li wrote:
> > The implementation of the current virtio-balloon is not very
> > efficient, Bellow is test result of time spends on inflating the
> > balloon to 3GB of a 4GB idle guest:
> >
> > a. allocating pages (6.5%, 103ms)
> > b. sending PFNs to host (68.3%, 787ms) c. address translation (6.1%,
> > 96ms) d. madvise (19%, 300ms)
> >
> > It takes about 1577ms for the whole inflating process to complete. The
> > test shows that the bottle neck is the stage b and stage d.
> >
> > If using a bitmap to send the page info instead of the PFNs, we can
> > reduce the overhead spends on stage b quite a lot. Furthermore, it's
> > possible to do the address translation and do the madvise with a bulk
> > of pages, instead of the current page per page way, so the overhead of
> > stage c and stage d can also be reduced a lot.
> >
> > This patch is the kernel side implementation which is intended to
> > speed up the inflating & deflating process by adding a new feature to
> > the virtio-balloon device. And now, inflating the balloon to 3GB of a
> > 4GB idle guest only takes 175ms, it's about 9 times as fast as before.
> >
> > TODO: optimize stage a by allocating/freeing a chunk of pages instead
> > of a single page at a time.
> >
> > Signed-off-by: Liang Li
> > ---
> > drivers/virtio/virtio_balloon.c | 199
> ++--
> > include/uapi/linux/virtio_balloon.h | 1 +
> > mm/page_alloc.c | 6 ++
> > 3 files changed, 198 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c
> > b/drivers/virtio/virtio_balloon.c index 7b6d74f..5330b6f 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -45,6 +45,8 @@ static int oom_pages =
> OOM_VBALLOON_DEFAULT_PAGES;
> > module_param(oom_pages, int, S_IRUSR | S_IWUSR);
> > MODULE_PARM_DESC(oom_pages, "pages to free on OOM");
> >
> > +extern unsigned long get_max_pfn(void);
> > +
> > struct virtio_balloon {
> > struct virtio_device *vdev;
> > struct virtqueue *inflate_vq, *deflate_vq, *stats_vq; @@ -62,6 +64,9
> > @@ struct virtio_balloon {
> >
> > /* Number of balloon pages we've told the Host we're not using. */
> > unsigned int num_pages;
> > + unsigned long *page_bitmap;
> > + unsigned long start_pfn, end_pfn;
> > + unsigned long bmap_len;
> > /*
> > * The pages we've told the Host we're not using are enqueued
> > * at vb_dev_info->pages list.
> > @@ -111,15 +116,66 @@ static void balloon_ack(struct virtqueue *vq)
> > wake_up(&vb->acked);
> > }
> >
> > +static int balloon_page_bitmap_init(struct virtio_balloon *vb) {
> > + unsigned long max_pfn, bmap_bytes;
> > +
> > + max_pfn = get_max_pfn();
>
> This is racy. max_pfn could be increased by memory hotplug after you got it.
>
>
> > + bmap_bytes = ALIGN(max_pfn, BITS_PER_LONG) / BITS_PER_BYTE;
> > + if (!vb->page_bitmap)
> > + vb->page_bitmap = kzalloc(bmap_bytes, GFP_KERNEL);
>
> Likely to fail for a huge busy guest.
> Why not init on device probe?
> this way
> - probe will fail, or we can clear the feature bit
> - free memory is more likely to be available
>
Very good suggestion!
>
> > + else {
> > + if (bmap_bytes <= vb->bmap_len)
> > + memset(vb->page_bitmap, 0, bmap_bytes);
> > + else {
> > + kfree(vb->page_bitmap);
> > + vb->page_bitmap = kzalloc(bmap_bytes,
> GFP_KERNEL);
> > + }
> > + }
> > + if (!vb->page_bitmap) {
> > + dev_err(&vb->vdev->dev, "%s failure: allocate page
> bitmap\n",
> > +__func__);
> > + return -ENOMEM;
> > + }
> > + vb->bmap_len = bmap_bytes;
> > + vb->start_pfn = max_pfn;
> > + vb->end_pfn = 0;
> > +
> > + return 0;
> > +}
> > +
>
> > {
> > - struct scatterlist sg;
> > unsigned int len;
> >
> > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> > + if (virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_PAGE_BITMAP)) {
> > + u32 page_shift = PAGE_SHIFT;
> > + unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > + struct scatterlist sg[5];
> > +
> > + start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > + end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > + bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> sizeof(long);
> > +
> > + sg_init_table(sg, 5);
> > + sg_set_buf(&sg[0], &flags, sizeof(flags));
> > + sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> > + sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> > + sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > + sg_set_buf(&sg[4], vb->page_bitmap +
> > +(start_pfn / BITS_PER_LONG), bmap_len);
>
> This can be pre-initialized, correct?
pre-initialized? I am not quite unde
RE: [PATCH RFC kernel] balloon: speed up inflating/deflating process
> > On Fri, May 20, 2016 at 05:59:46PM +0800, Liang Li wrote:
> > > The implementation of the current virtio-balloon is not very
> > > efficient, Bellow is test result of time spends on inflating the
> > > balloon to 3GB of a 4GB idle guest:
> > >
> > > a. allocating pages (6.5%, 103ms)
> > > b. sending PFNs to host (68.3%, 787ms) c. address translation (6.1%,
> > > 96ms) d. madvise (19%, 300ms)
> > >
> > > It takes about 1577ms for the whole inflating process to complete.
> > > The test shows that the bottle neck is the stage b and stage d.
> > >
> > > If using a bitmap to send the page info instead of the PFNs, we can
> > > reduce the overhead spends on stage b quite a lot. Furthermore, it's
> > > possible to do the address translation and do the madvise with a
> > > bulk of pages, instead of the current page per page way, so the
> > > overhead of stage c and stage d can also be reduced a lot.
> > >
> > > This patch is the kernel side implementation which is intended to
> > > speed up the inflating & deflating process by adding a new feature
> > > to the virtio-balloon device. And now, inflating the balloon to 3GB
> > > of a 4GB idle guest only takes 175ms, it's about 9 times as fast as
> > > before.
> > >
> > > TODO: optimize stage a by allocating/freeing a chunk of pages
> > > instead of a single page at a time.
> > >
> > > Signed-off-by: Liang Li
> > > ---
> > > drivers/virtio/virtio_balloon.c | 199
> > ++--
> > > include/uapi/linux/virtio_balloon.h | 1 +
> > > mm/page_alloc.c | 6 ++
> > > 3 files changed, 198 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_balloon.c
> > > b/drivers/virtio/virtio_balloon.c index 7b6d74f..5330b6f 100644
> > > --- a/drivers/virtio/virtio_balloon.c
> > > +++ b/drivers/virtio/virtio_balloon.c
> > > @@ -45,6 +45,8 @@ static int oom_pages =
> > OOM_VBALLOON_DEFAULT_PAGES;
> > > module_param(oom_pages, int, S_IRUSR | S_IWUSR);
> > > MODULE_PARM_DESC(oom_pages, "pages to free on OOM");
> > >
> > > +extern unsigned long get_max_pfn(void);
> > > +
> > > struct virtio_balloon {
> > > struct virtio_device *vdev;
> > > struct virtqueue *inflate_vq, *deflate_vq, *stats_vq; @@ -62,6
> > > +64,9 @@ struct virtio_balloon {
> > >
> > > /* Number of balloon pages we've told the Host we're not using. */
> > > unsigned int num_pages;
> > > + unsigned long *page_bitmap;
> > > + unsigned long start_pfn, end_pfn;
> > > + unsigned long bmap_len;
> > > /*
> > >* The pages we've told the Host we're not using are enqueued
> > >* at vb_dev_info->pages list.
> > > @@ -111,15 +116,66 @@ static void balloon_ack(struct virtqueue *vq)
> > > wake_up(&vb->acked);
> > > }
> > >
> > > +static int balloon_page_bitmap_init(struct virtio_balloon *vb) {
> > > + unsigned long max_pfn, bmap_bytes;
> > > +
> > > + max_pfn = get_max_pfn();
> >
> > This is racy. max_pfn could be increased by memory hotplug after you got it.
> >
> >
> > > + bmap_bytes = ALIGN(max_pfn, BITS_PER_LONG) / BITS_PER_BYTE;
> > > + if (!vb->page_bitmap)
> > > + vb->page_bitmap = kzalloc(bmap_bytes, GFP_KERNEL);
> >
> > Likely to fail for a huge busy guest.
> > Why not init on device probe?
> > this way
> > - probe will fail, or we can clear the feature bit
> > - free memory is more likely to be available
> >
>
> Very good suggestion!
>
> >
> > > + else {
> > > + if (bmap_bytes <= vb->bmap_len)
> > > + memset(vb->page_bitmap, 0, bmap_bytes);
> > > + else {
> > > + kfree(vb->page_bitmap);
> > > + vb->page_bitmap = kzalloc(bmap_bytes,
> > GFP_KERNEL);
> > > + }
> > > + }
> > > + if (!vb->page_bitmap) {
> > > + dev_err(&vb->vdev->dev, "%s failure: allocate page
> > bitmap\n",
> > > + __func__);
> > > + return -ENOMEM;
> > > + }
> > > + vb->bmap_len = bmap_bytes;
> > > + vb->start_pfn = max_pfn;
> > > + vb->end_pfn = 0;
> > > +
> > > + return 0;
> > > +}
> > > +
> >
> > > {
> > > - struct scatterlist sg;
> > > unsigned int len;
> > >
> > > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> > > + if (virtio_has_feature(vb->vdev,
> > VIRTIO_BALLOON_F_PAGE_BITMAP)) {
> > > + u32 page_shift = PAGE_SHIFT;
> > > + unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > > + struct scatterlist sg[5];
> > > +
> > > + start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > > + end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > > + bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> > sizeof(long);
> > > +
> > > + sg_init_table(sg, 5);
> > > + sg_set_buf(&sg[0], &flags, sizeof(flags));
> > > + sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> > > + sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> > > + sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > > + sg_set_buf(&sg[4], vb->page_bitmap
Re: [PATCH RFC kernel] balloon: speed up inflating/deflating process
On Tue, May 24, 2016 at 09:51:46AM +, Li, Liang Z wrote:
> > On Fri, May 20, 2016 at 05:59:46PM +0800, Liang Li wrote:
> > > The implementation of the current virtio-balloon is not very
> > > efficient, Bellow is test result of time spends on inflating the
> > > balloon to 3GB of a 4GB idle guest:
> > >
> > > a. allocating pages (6.5%, 103ms)
> > > b. sending PFNs to host (68.3%, 787ms) c. address translation (6.1%,
> > > 96ms) d. madvise (19%, 300ms)
> > >
> > > It takes about 1577ms for the whole inflating process to complete. The
> > > test shows that the bottle neck is the stage b and stage d.
> > >
> > > If using a bitmap to send the page info instead of the PFNs, we can
> > > reduce the overhead spends on stage b quite a lot. Furthermore, it's
> > > possible to do the address translation and do the madvise with a bulk
> > > of pages, instead of the current page per page way, so the overhead of
> > > stage c and stage d can also be reduced a lot.
> > >
> > > This patch is the kernel side implementation which is intended to
> > > speed up the inflating & deflating process by adding a new feature to
> > > the virtio-balloon device. And now, inflating the balloon to 3GB of a
> > > 4GB idle guest only takes 175ms, it's about 9 times as fast as before.
> > >
> > > TODO: optimize stage a by allocating/freeing a chunk of pages instead
> > > of a single page at a time.
> > >
> > > Signed-off-by: Liang Li
> > > ---
> > > drivers/virtio/virtio_balloon.c | 199
> > ++--
> > > include/uapi/linux/virtio_balloon.h | 1 +
> > > mm/page_alloc.c | 6 ++
> > > 3 files changed, 198 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_balloon.c
> > > b/drivers/virtio/virtio_balloon.c index 7b6d74f..5330b6f 100644
> > > --- a/drivers/virtio/virtio_balloon.c
> > > +++ b/drivers/virtio/virtio_balloon.c
> > > @@ -45,6 +45,8 @@ static int oom_pages =
> > OOM_VBALLOON_DEFAULT_PAGES;
> > > module_param(oom_pages, int, S_IRUSR | S_IWUSR);
> > > MODULE_PARM_DESC(oom_pages, "pages to free on OOM");
> > >
> > > +extern unsigned long get_max_pfn(void);
> > > +
> > > struct virtio_balloon {
> > > struct virtio_device *vdev;
> > > struct virtqueue *inflate_vq, *deflate_vq, *stats_vq; @@ -62,6 +64,9
> > > @@ struct virtio_balloon {
> > >
> > > /* Number of balloon pages we've told the Host we're not using. */
> > > unsigned int num_pages;
> > > + unsigned long *page_bitmap;
> > > + unsigned long start_pfn, end_pfn;
> > > + unsigned long bmap_len;
> > > /*
> > >* The pages we've told the Host we're not using are enqueued
> > >* at vb_dev_info->pages list.
> > > @@ -111,15 +116,66 @@ static void balloon_ack(struct virtqueue *vq)
> > > wake_up(&vb->acked);
> > > }
> > >
> > > +static int balloon_page_bitmap_init(struct virtio_balloon *vb) {
> > > + unsigned long max_pfn, bmap_bytes;
> > > +
> > > + max_pfn = get_max_pfn();
> >
> > This is racy. max_pfn could be increased by memory hotplug after you got it.
> >
> >
> > > + bmap_bytes = ALIGN(max_pfn, BITS_PER_LONG) / BITS_PER_BYTE;
> > > + if (!vb->page_bitmap)
> > > + vb->page_bitmap = kzalloc(bmap_bytes, GFP_KERNEL);
> >
> > Likely to fail for a huge busy guest.
> > Why not init on device probe?
> > this way
> > - probe will fail, or we can clear the feature bit
> > - free memory is more likely to be available
> >
>
> Very good suggestion!
>
> >
> > > + else {
> > > + if (bmap_bytes <= vb->bmap_len)
> > > + memset(vb->page_bitmap, 0, bmap_bytes);
> > > + else {
> > > + kfree(vb->page_bitmap);
> > > + vb->page_bitmap = kzalloc(bmap_bytes,
> > GFP_KERNEL);
> > > + }
> > > + }
> > > + if (!vb->page_bitmap) {
> > > + dev_err(&vb->vdev->dev, "%s failure: allocate page
> > bitmap\n",
> > > + __func__);
> > > + return -ENOMEM;
> > > + }
> > > + vb->bmap_len = bmap_bytes;
> > > + vb->start_pfn = max_pfn;
> > > + vb->end_pfn = 0;
> > > +
> > > + return 0;
> > > +}
> > > +
> >
> > > {
> > > - struct scatterlist sg;
> > > unsigned int len;
> > >
> > > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> > > + if (virtio_has_feature(vb->vdev,
> > VIRTIO_BALLOON_F_PAGE_BITMAP)) {
> > > + u32 page_shift = PAGE_SHIFT;
> > > + unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > > + struct scatterlist sg[5];
> > > +
> > > + start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > > + end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > > + bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> > sizeof(long);
> > > +
> > > + sg_init_table(sg, 5);
> > > + sg_set_buf(&sg[0], &flags, sizeof(flags));
> > > + sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> > > + sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> > > + sg_set_buf(&sg[3], &bmap_len, sizeof(b
RE: [PATCH RFC kernel] balloon: speed up inflating/deflating process
> > > > {
> > > > - struct scatterlist sg;
> > > > unsigned int len;
> > > >
> > > > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> > > > + if (virtio_has_feature(vb->vdev,
> > > VIRTIO_BALLOON_F_PAGE_BITMAP)) {
> > > > + u32 page_shift = PAGE_SHIFT;
> > > > + unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > > > + struct scatterlist sg[5];
> > > > +
> > > > + start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > > > + end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > > > + bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> > > sizeof(long);
> > > > +
> > > > + sg_init_table(sg, 5);
> > > > + sg_set_buf(&sg[0], &flags, sizeof(flags));
> > > > + sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> > > > + sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> > > > + sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > > > + sg_set_buf(&sg[4], vb->page_bitmap +
> > > > +(start_pfn / BITS_PER_LONG), bmap_len);
> > >
> > > This can be pre-initialized, correct?
> >
> > pre-initialized? I am not quite understand your mean.
>
> I think you can maintain sg as part of device state and init sg with the
> bitmap.
>
I got it.
> > > This is grossly inefficient if you only requested a single page.
> > > And it's also allocating memory very aggressively without ever
> > > telling the host what is going on.
> >
> > If only requested a single page, there is no need to send the entire
> > page bitmap, This RFC patch has already considered about this.
>
> where's that addressed in code?
>
By record the start_pfn and end_pfn.
The start_pfn & end_pfn will be updated in set_page_bitmap()
and will be used in the function tell_host():
-
+static void set_page_bitmap(struct virtio_balloon *vb, struct page
+*page) {
+ unsigned int i;
+ unsigned long *bitmap = vb->page_bitmap;
+ unsigned long balloon_pfn = page_to_balloon_pfn(page);
+
+ for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++)
+ set_bit(balloon_pfn + i, bitmap);
+ if (balloon_pfn < vb->start_pfn)
+ vb->start_pfn = balloon_pfn;
+ if (balloon_pfn > vb->end_pfn)
+ vb->end_pfn = balloon_pfn;
+}
+ unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
+ struct scatterlist sg[5];
+
+ start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
+ end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
+ bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG * sizeof(long);
+
+ sg_init_table(sg, 5);
+ sg_set_buf(&sg[0], &flags, sizeof(flags));
+ sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
+ sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
+ sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
+ sg_set_buf(&sg[4], vb->page_bitmap +
+(start_pfn / BITS_PER_LONG), bmap_len);
+ virtqueue_add_outbuf(vq, sg, 5, vb, GFP_KERNEL);
---
> > But it can works very well if requesting several pages which across a
> > large range.
>
> Some kind of limit on range would make sense though.
> It need not cover max pfn.
>
Yes, agree.
> > > Suggestion to address all above comments:
> > > 1. allocate a bunch of pages and link them up,
> > > calculating the min and the max pfn.
> > > if max-min exceeds the allocated bitmap size,
> > > tell host.
> >
> > I am not sure if it works well in some cases, e.g. The allocated pages
> > are across a wide range and the max-min > limit is very frequently to be
> true.
> > Then, there will be many times of virtio transmission and it's bad for
> > performance improvement. Right?
>
> It's a tradeoff for sure. Measure it, see what the overhead is.
OK, I will try and get back to you.
>
> >
> > > 2. limit allocated bitmap size to something reasonable.
> > > How about 32Kbytes? This is 256kilo bit in the map, which comes
> > > out to 1Giga bytes of memory in the balloon.
> >
> > So, even the VM has 1TB of RAM, the page bitmap will take 32MB of
> memory.
> > Maybe it's better to use a big page bitmap the save the pages
> > allocated by balloon, and split the big page bitmap to 32K bytes unit, then
> transfer one unit at a time.
>
> How is this different from what I said?
>
It's good if it's the same as you said.
Thanks!
Liang
> >
> > Should we use a page bitmap to replace 'vb->pages' ?
> >
> > How about rolling back to use PFNs if the count of requested pages is a
> small number?
> >
> > Liang
>
> That's why we have start pfn. you can us
Re: [PATCH RFC kernel] balloon: speed up inflating/deflating process
On Tue, May 24, 2016 at 10:38:43AM +, Li, Liang Z wrote:
> > > > > {
> > > > > - struct scatterlist sg;
> > > > > unsigned int len;
> > > > >
> > > > > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> > > > > + if (virtio_has_feature(vb->vdev,
> > > > VIRTIO_BALLOON_F_PAGE_BITMAP)) {
> > > > > + u32 page_shift = PAGE_SHIFT;
> > > > > + unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > > > > + struct scatterlist sg[5];
> > > > > +
> > > > > + start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > > > > + end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > > > > + bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> > > > sizeof(long);
> > > > > +
> > > > > + sg_init_table(sg, 5);
> > > > > + sg_set_buf(&sg[0], &flags, sizeof(flags));
> > > > > + sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> > > > > + sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> > > > > + sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > > > > + sg_set_buf(&sg[4], vb->page_bitmap +
> > > > > + (start_pfn / BITS_PER_LONG), bmap_len);
> > > >
> > > > This can be pre-initialized, correct?
> > >
> > > pre-initialized? I am not quite understand your mean.
> >
> > I think you can maintain sg as part of device state and init sg with the
> > bitmap.
> >
>
> I got it.
>
> > > > This is grossly inefficient if you only requested a single page.
> > > > And it's also allocating memory very aggressively without ever
> > > > telling the host what is going on.
> > >
> > > If only requested a single page, there is no need to send the entire
> > > page bitmap, This RFC patch has already considered about this.
> >
> > where's that addressed in code?
> >
>
> By record the start_pfn and end_pfn.
>
> The start_pfn & end_pfn will be updated in set_page_bitmap()
> and will be used in the function tell_host():
>
> -
> +static void set_page_bitmap(struct virtio_balloon *vb, struct page
> +*page) {
> + unsigned int i;
> + unsigned long *bitmap = vb->page_bitmap;
> + unsigned long balloon_pfn = page_to_balloon_pfn(page);
> +
> + for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++)
> + set_bit(balloon_pfn + i, bitmap);
BTW, there's a page size value in header so there
is no longer need to set multiple bits per page.
> + if (balloon_pfn < vb->start_pfn)
> + vb->start_pfn = balloon_pfn;
> + if (balloon_pfn > vb->end_pfn)
> + vb->end_pfn = balloon_pfn;
> +}
Sounds good, but you also need to limit by
allocated bitmap size.
>
> + unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> + struct scatterlist sg[5];
> +
> + start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> + end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> + bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG * sizeof(long);
> +
> + sg_init_table(sg, 5);
> + sg_set_buf(&sg[0], &flags, sizeof(flags));
> + sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> + sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> + sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> + sg_set_buf(&sg[4], vb->page_bitmap +
> + (start_pfn / BITS_PER_LONG), bmap_len);
Looks wrong. start_pfn should start at offset 0 I think ...
> + virtqueue_add_outbuf(vq, sg, 5, vb, GFP_KERNEL);
> ---
> > > But it can works very well if requesting several pages which across a
> > > large range.
> >
> > Some kind of limit on range would make sense though.
> > It need not cover max pfn.
> >
>
> Yes, agree.
>
> > > > Suggestion to address all above comments:
> > > > 1. allocate a bunch of pages and link them up,
> > > >calculating the min and the max pfn.
> > > >if max-min exceeds the allocated bitmap size,
> > > >tell host.
> > >
> > > I am not sure if it works well in some cases, e.g. The allocated pages
> > > are across a wide range and the max-min > limit is very frequently to be
> > true.
> > > Then, there will be many times of virtio transmission and it's bad for
> > > performance improvement. Right?
> >
> > It's a tradeoff for sure. Measure it, see what the overhead is.
>
> OK, I will try and get back to you.
>
> >
> > >
> > > > 2. limit allocated bitmap size to something reasonable.
> > > >How about 32Kbytes? This is 256kilo bit in the map, which
> > > > comes
> > > >out to 1Giga bytes of memory in the balloon.
> > >
> > > So, even the VM has 1TB of RAM, the page bitmap will take 32MB of
> > memory.
> > > Ma
[PULL] vhost: cleanups and fixes
The following changes since commit 2dcd0af568b0cf583645c8a317dd12e344b1c72a: Linux 4.6 (2016-05-15 15:43:13 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus for you to fetch changes up to bb991288728e6a47a6f0fac6a4e9dfaeecc27956: ringtest: pass buf != NULL (2016-05-22 19:44:14 +0300) virtio: patches for 4.7 Looks like a quiet cycle for virtio. There's a new inorder option for the ringtest tool, and a bugfix for balloon for ppc platforms when using virtio 1 mode. Signed-off-by: Michael S. Tsirkin Michael S. Tsirkin (3): virtio: add inorder option virtio_balloon: fix PFN format for virtio-1 ringtest: pass buf != NULL drivers/virtio/virtio_balloon.c | 20 +++- tools/virtio/ringtest/main.c| 2 +- tools/virtio/ringtest/virtio_ring_0_9.c | 49 - tools/virtio/ringtest/virtio_ring_inorder.c | 2 ++ tools/virtio/ringtest/Makefile | 5 ++- 5 files changed, 67 insertions(+), 11 deletions(-) create mode 100644 tools/virtio/ringtest/virtio_ring_inorder.c ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] tools/virtio: add noring tool
Useful to measure testing framework overhead.
Cc: Jesper Dangaard Brouer
Signed-off-by: Michael S. Tsirkin
---
tools/virtio/ringtest/noring.c | 69 ++
tools/virtio/ringtest/Makefile | 4 ++-
2 files changed, 72 insertions(+), 1 deletion(-)
create mode 100644 tools/virtio/ringtest/noring.c
diff --git a/tools/virtio/ringtest/noring.c b/tools/virtio/ringtest/noring.c
new file mode 100644
index 000..eda2f48
--- /dev/null
+++ b/tools/virtio/ringtest/noring.c
@@ -0,0 +1,69 @@
+#define _GNU_SOURCE
+#include "main.h"
+#include
+
+/* stub implementation: useful for measuring overhead */
+void alloc_ring(void)
+{
+}
+
+/* guest side */
+int add_inbuf(unsigned len, void *buf, void *datap)
+{
+ return 0;
+}
+
+/*
+ * skb_array API provides no way for producer to find out whether a given
+ * buffer was consumed. Our tests merely require that a successful get_buf
+ * implies that add_inbuf succeed in the past, and that add_inbuf will succeed,
+ * fake it accordingly.
+ */
+void *get_buf(unsigned *lenp, void **bufp)
+{
+ return "Buffer";
+}
+
+void poll_used(void)
+{
+}
+
+void disable_call()
+{
+ assert(0);
+}
+
+bool enable_call()
+{
+ assert(0);
+}
+
+void kick_available(void)
+{
+ assert(0);
+}
+
+/* host side */
+void disable_kick()
+{
+ assert(0);
+}
+
+bool enable_kick()
+{
+ assert(0);
+}
+
+void poll_avail(void)
+{
+}
+
+bool use_buf(unsigned *lenp, void **bufp)
+{
+ return true;
+}
+
+void call_used(void)
+{
+ assert(0);
+}
diff --git a/tools/virtio/ringtest/Makefile b/tools/virtio/ringtest/Makefile
index a8356d8..b3d5bc8 100644
--- a/tools/virtio/ringtest/Makefile
+++ b/tools/virtio/ringtest/Makefile
@@ -1,6 +1,6 @@
all:
-all: ring virtio_ring_0_9 virtio_ring_poll virtio_ring_inorder skb_array
+all: ring virtio_ring_0_9 virtio_ring_poll virtio_ring_inorder skb_array noring
CFLAGS += -Wall
CFLAGS += -pthread -O2 -ggdb
@@ -17,6 +17,7 @@ virtio_ring_0_9: virtio_ring_0_9.o main.o
virtio_ring_poll: virtio_ring_poll.o main.o
virtio_ring_inorder: virtio_ring_inorder.o main.o
skb_array: skb_array.o main.o
+noring: noring.o main.o
clean:
-rm main.o
-rm ring.o ring
@@ -24,5 +25,6 @@ clean:
-rm virtio_ring_poll.o virtio_ring_poll
-rm virtio_ring_inorder.o virtio_ring_inorder
-rm skb_array.o skb_array
+ -rm noring.o noring
.PHONY: all clean
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] tools/virtio/ringtest: add usage example to README
Michael, Any updates on this? On Wed, May 04, 2016 at 09:12:55AM +0300, Mike Rapoport wrote: > Having typical usage example in the README file is more convinient than in > the git history... > > Signed-off-by: Mike Rapoport > --- > tools/virtio/ringtest/README | 4 > 1 file changed, 4 insertions(+) > > diff --git a/tools/virtio/ringtest/README b/tools/virtio/ringtest/README > index 34e94c4..d83707a 100644 > --- a/tools/virtio/ringtest/README > +++ b/tools/virtio/ringtest/README > @@ -1,2 +1,6 @@ > Partial implementation of various ring layouts, useful to tune virtio design. > Uses shared memory heavily. > + > +Typical use: > + > +# sh run-on-all.sh perf stat -r 10 --log-fd 1 -- ./ring > -- > 1.9.1 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3] tools/virtio/ringtest: fix run-on-all.sh to work without /dev/cpu
Michael, Any updates on this? On Wed, May 04, 2016 at 01:15:50PM +0300, Mike Rapoport wrote: > /dev/cpu is only available on x86 with certain modules (e.g. msr) enabled. > Using lscpu to get processors count is more portable. > > Signed-off-by: Mike Rapoport > --- > v3: simplify by using lscpu -p=cpu > v2: use lspcu instead of /proc/cpuinfo as per Cornelia's suggestion > > tools/virtio/ringtest/run-on-all.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/virtio/ringtest/run-on-all.sh > b/tools/virtio/ringtest/run-on-all.sh > index 52b0f71..2e69ca8 100755 > --- a/tools/virtio/ringtest/run-on-all.sh > +++ b/tools/virtio/ringtest/run-on-all.sh > @@ -3,10 +3,10 @@ > #use last CPU for host. Why not the first? > #many devices tend to use cpu0 by default so > #it tends to be busier > -HOST_AFFINITY=$(cd /dev/cpu; ls|grep -v '[a-z]'|sort -n|tail -1) > +HOST_AFFINITY=$(lscpu -p=cpu | tail -1) > > #run command on all cpus > -for cpu in $(cd /dev/cpu; ls|grep -v '[a-z]'|sort -n); > +for cpu in $(seq 0 $HOST_AFFINITY) > do > #Don't run guest and host on same CPU > #It actually works ok if using signalling > -- > 1.9.1 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH RFC kernel] balloon: speed up inflating/deflating process
> > > > > This can be pre-initialized, correct?
> > > >
> > > > pre-initialized? I am not quite understand your mean.
> > >
> > > I think you can maintain sg as part of device state and init sg with the
> bitmap.
> > >
> >
> > I got it.
> >
> > > > > This is grossly inefficient if you only requested a single page.
> > > > > And it's also allocating memory very aggressively without ever
> > > > > telling the host what is going on.
> > > >
> > > > If only requested a single page, there is no need to send the
> > > > entire page bitmap, This RFC patch has already considered about this.
> > >
> > > where's that addressed in code?
> > >
> >
> > By record the start_pfn and end_pfn.
> >
> > The start_pfn & end_pfn will be updated in set_page_bitmap() and will
> > be used in the function tell_host():
> >
> > --
> > ---
> > +static void set_page_bitmap(struct virtio_balloon *vb, struct page
> > +*page) {
> > + unsigned int i;
> > + unsigned long *bitmap = vb->page_bitmap;
> > + unsigned long balloon_pfn = page_to_balloon_pfn(page);
> > +
> > + for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++)
> > + set_bit(balloon_pfn + i, bitmap);
>
> BTW, there's a page size value in header so there is no longer need to set
> multiple bits per page.
Yes, you are right.
>
> > + if (balloon_pfn < vb->start_pfn)
> > + vb->start_pfn = balloon_pfn;
> > + if (balloon_pfn > vb->end_pfn)
> > + vb->end_pfn = balloon_pfn;
> > +}
>
> Sounds good, but you also need to limit by allocated bitmap size.
Why should we limit the page bitmap size? Is it no good to send a large page
bitmap?
or to save the memory used for page bitmap? Or some other reason?
>
> >
> > + unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > + struct scatterlist sg[5];
> > +
> > + start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > + end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > + bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> sizeof(long);
> > +
> > + sg_init_table(sg, 5);
> > + sg_set_buf(&sg[0], &flags, sizeof(flags));
> > + sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> > + sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> > + sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > + sg_set_buf(&sg[4], vb->page_bitmap +
> > +(start_pfn / BITS_PER_LONG), bmap_len);
>
> Looks wrong. start_pfn should start at offset 0 I think ...
I don't know what is wrong here, could you tell me why?
Thanks!
Liang
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC kernel] balloon: speed up inflating/deflating process
On Tue, May 24, 2016 at 02:36:08PM +, Li, Liang Z wrote:
> > > > > > This can be pre-initialized, correct?
> > > > >
> > > > > pre-initialized? I am not quite understand your mean.
> > > >
> > > > I think you can maintain sg as part of device state and init sg with the
> > bitmap.
> > > >
> > >
> > > I got it.
> > >
> > > > > > This is grossly inefficient if you only requested a single page.
> > > > > > And it's also allocating memory very aggressively without ever
> > > > > > telling the host what is going on.
> > > > >
> > > > > If only requested a single page, there is no need to send the
> > > > > entire page bitmap, This RFC patch has already considered about this.
> > > >
> > > > where's that addressed in code?
> > > >
> > >
> > > By record the start_pfn and end_pfn.
> > >
> > > The start_pfn & end_pfn will be updated in set_page_bitmap() and will
> > > be used in the function tell_host():
> > >
> > > --
> > > ---
> > > +static void set_page_bitmap(struct virtio_balloon *vb, struct page
> > > +*page) {
> > > + unsigned int i;
> > > + unsigned long *bitmap = vb->page_bitmap;
> > > + unsigned long balloon_pfn = page_to_balloon_pfn(page);
> > > +
> > > + for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++)
> > > + set_bit(balloon_pfn + i, bitmap);
> >
> > BTW, there's a page size value in header so there is no longer need to set
> > multiple bits per page.
>
> Yes, you are right.
>
> >
> > > + if (balloon_pfn < vb->start_pfn)
> > > + vb->start_pfn = balloon_pfn;
> > > + if (balloon_pfn > vb->end_pfn)
> > > + vb->end_pfn = balloon_pfn;
> > > +}
> >
> > Sounds good, but you also need to limit by allocated bitmap size.
>
> Why should we limit the page bitmap size? Is it no good to send a large page
> bitmap?
> or to save the memory used for page bitmap? Or some other reason?
To save memory. First allocating a large bitmap can fail, second this is
pinned memory that is wasted - it's unused most of the time while guest
is running.
> >
> > >
> > > + unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > > + struct scatterlist sg[5];
> > > +
> > > + start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > > + end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > > + bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> > sizeof(long);
> > > +
> > > + sg_init_table(sg, 5);
> > > + sg_set_buf(&sg[0], &flags, sizeof(flags));
> > > + sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> > > + sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> > > + sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > > + sg_set_buf(&sg[4], vb->page_bitmap +
> > > + (start_pfn / BITS_PER_LONG), bmap_len);
> >
> > Looks wrong. start_pfn should start at offset 0 I think ...
>
> I don't know what is wrong here, could you tell me why?
>
> Thanks!
>
> Liang
start_pfn should mean "bit 0 in bitmap refers to pfn X".
So it does not make sense to also add it as offset within bitmap.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH RFC kernel] balloon: speed up inflating/deflating process
> On Tue, May 24, 2016 at 02:36:08PM +, Li, Liang Z wrote:
> > > > > > > This can be pre-initialized, correct?
> > > > > >
> > > > > > pre-initialized? I am not quite understand your mean.
> > > > >
> > > > > I think you can maintain sg as part of device state and init sg
> > > > > with the
> > > bitmap.
> > > > >
> > > >
> > > > I got it.
> > > >
> > > > > > > This is grossly inefficient if you only requested a single page.
> > > > > > > And it's also allocating memory very aggressively without
> > > > > > > ever telling the host what is going on.
> > > > > >
> > > > > > If only requested a single page, there is no need to send the
> > > > > > entire page bitmap, This RFC patch has already considered about
> this.
> > > > >
> > > > > where's that addressed in code?
> > > > >
> > > >
> > > > By record the start_pfn and end_pfn.
> > > >
> > > > The start_pfn & end_pfn will be updated in set_page_bitmap() and
> > > > will be used in the function tell_host():
> > > >
> > > > --
> > > >
> > > > ---
> > > > +static void set_page_bitmap(struct virtio_balloon *vb, struct
> > > > +page
> > > > +*page) {
> > > > + unsigned int i;
> > > > + unsigned long *bitmap = vb->page_bitmap;
> > > > + unsigned long balloon_pfn = page_to_balloon_pfn(page);
> > > > +
> > > > + for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++)
> > > > + set_bit(balloon_pfn + i, bitmap);
> > >
> > > BTW, there's a page size value in header so there is no longer need
> > > to set multiple bits per page.
> >
> > Yes, you are right.
> >
> > >
> > > > + if (balloon_pfn < vb->start_pfn)
> > > > + vb->start_pfn = balloon_pfn;
> > > > + if (balloon_pfn > vb->end_pfn)
> > > > + vb->end_pfn = balloon_pfn;
> > > > +}
> > >
> > > Sounds good, but you also need to limit by allocated bitmap size.
> >
> > Why should we limit the page bitmap size? Is it no good to send a large
> page bitmap?
> > or to save the memory used for page bitmap? Or some other reason?
>
> To save memory. First allocating a large bitmap can fail, second this is
> pinned
> memory that is wasted - it's unused most of the time while guest is running.
>
> > >
> > > >
> > > > + unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > > > + struct scatterlist sg[5];
> > > > +
> > > > + start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > > > + end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > > > + bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> > > sizeof(long);
> > > > +
> > > > + sg_init_table(sg, 5);
> > > > + sg_set_buf(&sg[0], &flags, sizeof(flags));
> > > > + sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> > > > + sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> > > > + sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > > > + sg_set_buf(&sg[4], vb->page_bitmap +
> > > > +(start_pfn / BITS_PER_LONG), bmap_len);
> > >
> > > Looks wrong. start_pfn should start at offset 0 I think ...
> >
> > I don't know what is wrong here, could you tell me why?
> >
> > Thanks!
> >
> > Liang
>
> start_pfn should mean "bit 0 in bitmap refers to pfn X".
> So it does not make sense to also add it as offset within bitmap.
>
> --
> MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH RFC kernel] balloon: speed up inflating/deflating process
> > > > > > > This is grossly inefficient if you only requested a single page.
> > > > > > > And it's also allocating memory very aggressively without
> > > > > > > ever telling the host what is going on.
> > > > > >
> > > > > > If only requested a single page, there is no need to send the
> > > > > > entire page bitmap, This RFC patch has already considered about
> this.
> > > > >
> > > > > where's that addressed in code?
> > > > >
> > > >
> > > > By record the start_pfn and end_pfn.
> > > >
> > > > The start_pfn & end_pfn will be updated in set_page_bitmap() and
> > > > will be used in the function tell_host():
> > > >
> > > > --
> > > >
> > > > ---
> > > > +static void set_page_bitmap(struct virtio_balloon *vb, struct
> > > > +page
> > > > +*page) {
> > > > + unsigned int i;
> > > > + unsigned long *bitmap = vb->page_bitmap;
> > > > + unsigned long balloon_pfn = page_to_balloon_pfn(page);
> > > > +
> > > > + for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++)
> > > > + set_bit(balloon_pfn + i, bitmap);
> > >
> > > BTW, there's a page size value in header so there is no longer need
> > > to set multiple bits per page.
> >
> > Yes, you are right.
> >
> > >
> > > > + if (balloon_pfn < vb->start_pfn)
> > > > + vb->start_pfn = balloon_pfn;
> > > > + if (balloon_pfn > vb->end_pfn)
> > > > + vb->end_pfn = balloon_pfn;
> > > > +}
> > >
> > > Sounds good, but you also need to limit by allocated bitmap size.
> >
> > Why should we limit the page bitmap size? Is it no good to send a large
> page bitmap?
> > or to save the memory used for page bitmap? Or some other reason?
>
> To save memory. First allocating a large bitmap can fail, second this is
> pinned
> memory that is wasted - it's unused most of the time while guest is running.
Make sense.
>
> > >
> > > >
> > > > + unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > > > + struct scatterlist sg[5];
> > > > +
> > > > + start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > > > + end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > > > + bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> > > sizeof(long);
> > > > +
> > > > + sg_init_table(sg, 5);
> > > > + sg_set_buf(&sg[0], &flags, sizeof(flags));
> > > > + sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> > > > + sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> > > > + sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > > > + sg_set_buf(&sg[4], vb->page_bitmap +
> > > > +(start_pfn / BITS_PER_LONG), bmap_len);
> > >
> > > Looks wrong. start_pfn should start at offset 0 I think ...
> >
> > I don't know what is wrong here, could you tell me why?
> >
> > Thanks!
> >
> > Liang
>
> start_pfn should mean "bit 0 in bitmap refers to pfn X".
> So it does not make sense to also add it as offset within bitmap.
I got it, but in my code, it's correct, because the page_bitmap is
range from pfn 0 to max_pfn.
It should be changed if we are going to use a small page bitmap.
Thanks!
Liang
>
> --
> MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
