Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1505500 > unrolled thread

[RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration

Back to article view | Back to linux.kernel

Contents

  [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration Liang Li <liang.z.li@intel.com> - 2016-10-21 08:40 +0200
    [RESEND PATCH v3 kernel 4/7] virtio-balloon: speed up inflate/deflate process Liang Li <liang.z.li@intel.com> - 2016-10-21 08:40 +0200
      Re: [RESEND PATCH v3 kernel 4/7] virtio-balloon: speed up  inflate/deflate process "Michael S. Tsirkin" <mst@redhat.com> - 2016-10-25 08:40 +0200
        RE: [RESEND PATCH v3 kernel 4/7] virtio-balloon: speed up  inflate/deflate process "Li, Liang Z" <liang.z.li@intel.com> - 2016-10-25 11:50 +0200
    [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature bit and page bitmap head Liang Li <liang.z.li@intel.com> - 2016-10-21 08:40 +0200
      Re: [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature  bit and page bitmap head Dave Hansen <dave.hansen@intel.com> - 2016-10-24 19:00 +0200
        RE: [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature  bit and page bitmap head "Li, Liang Z" <liang.z.li@intel.com> - 2016-10-25 03:30 +0200
      Re: [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature  bit and page bitmap head "Michael S. Tsirkin" <mst@redhat.com> - 2016-10-26 17:50 +0200
    [RESEND PATCH v3 kernel 1/7] virtio-balloon: rework deflate to add page to a list Liang Li <liang.z.li@intel.com> - 2016-10-21 08:40 +0200
      Re: [RESEND PATCH v3 kernel 1/7] virtio-balloon: rework deflate to  add page to a list Dave Hansen <dave.hansen@intel.com> - 2016-10-24 18:50 +0200
        RE: [RESEND PATCH v3 kernel 1/7] virtio-balloon: rework deflate to  add page to a list "Li, Liang Z" <liang.z.li@intel.com> - 2016-10-25 03:20 +0200
    Re: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast  (de)inflating & fast live migration Dave Hansen <dave.hansen@intel.com> - 2016-10-21 19:30 +0200
      Re: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast  (de)inflating & fast live migration "Michael S. Tsirkin" <mst@redhat.com> - 2016-10-21 21:50 +0200
        RE: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast  (de)inflating & fast live migration "Li, Liang Z" <liang.z.li@intel.com> - 2016-10-23 13:30 +0200
          Re: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast  (de)inflating & fast live migration "Michael S. Tsirkin" <mst@redhat.com> - 2016-10-25 06:00 +0200
      RE: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast  (de)inflating & fast live migration "Li, Liang Z" <liang.z.li@intel.com> - 2016-10-26 12:10 +0200
        RE: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast  (de)inflating & fast live migration "Li, Liang Z" <liang.z.li@intel.com> - 2016-10-26 12:20 +0200
          Re: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast  (de)inflating & fast live migration Dave Hansen <dave.hansen@intel.com> - 2016-10-26 20:20 +0200
            RE: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast  (de)inflating & fast live migration "Li, Liang Z" <liang.z.li@intel.com> - 2016-10-27 02:40 +0200
        Re: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast  (de)inflating & fast live migration Dave Hansen <dave.hansen@intel.com> - 2016-10-26 20:20 +0200
          RE: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast  (de)inflating & fast live migration "Li, Liang Z" <liang.z.li@intel.com> - 2016-10-27 03:00 +0200

Page 1 of 2  [1] 2  Next page →

#1505500 — [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration

This patch set contains two parts of changes to the virtio-balloon.
 
One is the change for speeding up the inflating & deflating process,
the main idea of this optimization is to use bitmap to send the page
information to host instead of the PFNs, to reduce the overhead of
virtio data transmission, address translation and madvise(). This can
help to improve the performance by about 85%.
 
Another change is for speeding up live migration. By skipping process
guest's free pages in the first round of data copy, to reduce needless
data processing, this can help to save quite a lot of CPU cycles and
network bandwidth. We put guest's free page information in bitmap and
send it to host with the virt queue of virtio-balloon. For an idle 8GB
guest, this can help to shorten the total live migration time from 2Sec
to about 500ms in the 10Gbps network environment.
 
Dave Hansen suggested a new scheme to encode the data structure,
because of additional complexity, it's not implemented in v3.
 
Changes from v2 to v3:
    * Change the name of 'free page' to 'unused page'.
    * Use the scatter & gather bitmap instead of a 1MB page bitmap.
    * Fix overwriting the page bitmap after kicking.
    * Some of MST's comments for v2.
 
Changes from v1 to v2:
    * Abandon the patch for dropping page cache.
    * Put some structures to uapi head file.
    * Use a new way to determine the page bitmap size.
    * Use a unified way to send the free page information with the bitmap
    * Address the issues referred in MST's comments

Liang Li (7):
  virtio-balloon: rework deflate to add page to a list
  virtio-balloon: define new feature bit and page bitmap head
  mm: add a function to get the max pfn
  virtio-balloon: speed up inflate/deflate process
  mm: add the related functions to get unused page
  virtio-balloon: define feature bit and head for misc virt queue
  virtio-balloon: tell host vm's unused page info

 drivers/virtio/virtio_balloon.c     | 390 ++++++++++++++++++++++++++++++++----
 include/linux/mm.h                  |   3 +
 include/uapi/linux/virtio_balloon.h |  41 ++++
 mm/page_alloc.c                     |  94 +++++++++
 4 files changed, 485 insertions(+), 43 deletions(-)

-- 
1.8.3.1

[toc] | [next] | [standalone]

#1505501 — [RESEND PATCH v3 kernel 4/7] virtio-balloon: speed up inflate/deflate process

The implementation of the current virtio-balloon is not very
efficient, the time spends on different stages of inflating
the balloon to 7GB of a 8GB idle guest:

a. allocating pages (6.5%)
b. sending PFNs to host (68.3%)
c. address translation (6.1%)
d. madvise (19%)

It takes about 4126ms for the inflating process to complete.
Debugging shows that the bottle neck are the stage b and stage d.

If using a bitmap to send the page info instead of the PFNs, we
can reduce the overhead in stage b quite a lot. Furthermore, we
can do the address translation and call madvise() with a bulk of
RAM pages, instead of the current page per page way, 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. With this new feature, inflating the
balloon to 7GB of a 8GB idle guest only takes 590ms, the
performance improvement is about 85%.

TODO: optimize stage a by allocating/freeing a chunk of pages
instead of a single page at a time.

Signed-off-by: Liang Li <liang.z.li@intel.com>
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_balloon.c | 233 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 209 insertions(+), 24 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 59ffe5a..c31839c 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -42,6 +42,10 @@
 #define OOM_VBALLOON_DEFAULT_PAGES 256
 #define VIRTBALLOON_OOM_NOTIFY_PRIORITY 80
 
+#define BALLOON_BMAP_SIZE	(8 * PAGE_SIZE)
+#define PFNS_PER_BMAP		(BALLOON_BMAP_SIZE * BITS_PER_BYTE)
+#define BALLOON_BMAP_COUNT	32
+
 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");
@@ -67,6 +71,13 @@ struct virtio_balloon {
 
 	/* Number of balloon pages we've told the Host we're not using. */
 	unsigned int num_pages;
+	/* Pointer of the bitmap header. */
+	void *bmap_hdr;
+	/* Bitmap and bitmap count used to tell the host the pages */
+	unsigned long *page_bitmap[BALLOON_BMAP_COUNT];
+	unsigned int nr_page_bmap;
+	/* Used to record the processed pfn range */
+	unsigned long min_pfn, max_pfn, start_pfn, end_pfn;
 	/*
 	 * The pages we've told the Host we're not using are enqueued
 	 * at vb_dev_info->pages list.
@@ -110,16 +121,66 @@ static void balloon_ack(struct virtqueue *vq)
 	wake_up(&vb->acked);
 }
 
+static inline void init_pfn_range(struct virtio_balloon *vb)
+{
+	vb->min_pfn = ULONG_MAX;
+	vb->max_pfn = 0;
+}
+
+static inline void update_pfn_range(struct virtio_balloon *vb,
+				 struct page *page)
+{
+	unsigned long balloon_pfn = page_to_balloon_pfn(page);
+
+	if (balloon_pfn < vb->min_pfn)
+		vb->min_pfn = balloon_pfn;
+	if (balloon_pfn > vb->max_pfn)
+		vb->max_pfn = balloon_pfn;
+}
+
 static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
 {
-	struct scatterlist sg;
-	unsigned int len;
+	struct scatterlist sg, sg2[BALLOON_BMAP_COUNT + 1];
+	unsigned int len, i;
+
+	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP)) {
+		struct balloon_bmap_hdr *hdr = vb->bmap_hdr;
+		unsigned long bmap_len;
+		int nr_pfn, nr_used_bmap, nr_buf;
+
+		nr_pfn = vb->end_pfn - vb->start_pfn + 1;
+		nr_pfn = roundup(nr_pfn, BITS_PER_LONG);
+		nr_used_bmap = nr_pfn / PFNS_PER_BMAP;
+		bmap_len = nr_pfn / BITS_PER_BYTE;
+		nr_buf = nr_used_bmap + 1;
+
+		/* cmd, reserved and req_id are init to 0, unused here */
+		hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT);
+		hdr->start_pfn = cpu_to_virtio64(vb->vdev, vb->start_pfn);
+		hdr->bmap_len = cpu_to_virtio64(vb->vdev, bmap_len);
+		sg_init_table(sg2, nr_buf);
+		sg_set_buf(&sg2[0], hdr, sizeof(struct balloon_bmap_hdr));
+		for (i = 0; i < nr_used_bmap; i++) {
+			unsigned int  buf_len = BALLOON_BMAP_SIZE;
+
+			if (i + 1 == nr_used_bmap)
+				buf_len = bmap_len - BALLOON_BMAP_SIZE * i;
+			sg_set_buf(&sg2[i + 1], vb->page_bitmap[i], buf_len);
+		}
 
-	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
+		while (vq->num_free < nr_buf)
+			msleep(2);
+		if (virtqueue_add_outbuf(vq, sg2, nr_buf, vb, GFP_KERNEL) == 0)
+			virtqueue_kick(vq);
 
-	/* We should always be able to add one buffer to an empty queue. */
-	virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
-	virtqueue_kick(vq);
+	} else {
+		sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
+
+		/* We should always be able to add one buffer to an empty
+		 * queue. */
+		virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
+		virtqueue_kick(vq);
+	}
 
 	/* When host has read buffer, this completes via balloon_ack */
 	wait_event(vb->acked, virtqueue_get_buf(vq, &len));
@@ -138,13 +199,93 @@ static void set_page_pfns(struct virtio_balloon *vb,
 					  page_to_balloon_pfn(page) + i);
 }
 
-static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
+static void extend_page_bitmap(struct virtio_balloon *vb)
+{
+	int i;
+	unsigned long bmap_len, bmap_count;
+
+	bmap_len = ALIGN(get_max_pfn(), BITS_PER_LONG) / BITS_PER_BYTE;
+	bmap_count = bmap_len / BALLOON_BMAP_SIZE;
+	if (bmap_len % BALLOON_BMAP_SIZE)
+		bmap_count++;
+	if (bmap_count > BALLOON_BMAP_COUNT)
+		bmap_count = BALLOON_BMAP_COUNT;
+
+	for (i = 1; i < bmap_count; i++) {
+		vb->page_bitmap[i] = kmalloc(BALLOON_BMAP_SIZE, GFP_ATOMIC);
+		if (vb->page_bitmap[i])
+			vb->nr_page_bmap++;
+		else
+			break;
+	}
+}
+
+static void kfree_page_bitmap(struct virtio_balloon *vb)
+{
+	int i;
+
+	for (i = 0; i < vb->nr_page_bmap; i++)
+		kfree(vb->page_bitmap[i]);
+}
+
+static void clear_page_bitmap(struct virtio_balloon *vb)
+{
+	int i;
+
+	for (i = 0; i < vb->nr_page_bmap; i++)
+		memset(vb->page_bitmap[i], 0, BALLOON_BMAP_SIZE);
+}
+
+static void set_page_bitmap(struct virtio_balloon *vb,
+			 struct list_head *pages, struct virtqueue *vq)
+{
+	unsigned long pfn, pfn_limit;
+	struct page *page;
+	bool found;
+	int bmap_idx;
+
+	vb->min_pfn = rounddown(vb->min_pfn, BITS_PER_LONG);
+	vb->max_pfn = roundup(vb->max_pfn, BITS_PER_LONG);
+	pfn_limit = PFNS_PER_BMAP * vb->nr_page_bmap;
+
+	for (pfn = vb->min_pfn; pfn < vb->max_pfn; pfn += pfn_limit) {
+		unsigned long end_pfn;
+
+		clear_page_bitmap(vb);
+		vb->start_pfn = pfn;
+		end_pfn = pfn;
+		found = false;
+		list_for_each_entry(page, pages, lru) {
+			unsigned long pos, balloon_pfn;
+
+			balloon_pfn = page_to_balloon_pfn(page);
+			if (balloon_pfn < pfn || balloon_pfn >= pfn + pfn_limit)
+				continue;
+			bmap_idx = (balloon_pfn - pfn) / PFNS_PER_BMAP;
+			pos = (balloon_pfn - pfn) % PFNS_PER_BMAP;
+			set_bit(pos, vb->page_bitmap[bmap_idx]);
+			if (balloon_pfn > end_pfn)
+				end_pfn = balloon_pfn;
+			found = true;
+		}
+		if (found) {
+			vb->end_pfn = end_pfn;
+			tell_host(vb, vq);
+		}
+	}
+}
+
+static unsigned int fill_balloon(struct virtio_balloon *vb, size_t num,
+				 bool use_bmap)
 {
 	struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
-	unsigned num_allocated_pages;
+	unsigned int num_allocated_pages;
 
-	/* We can only do one array worth at a time. */
-	num = min(num, ARRAY_SIZE(vb->pfns));
+	if (use_bmap)
+		init_pfn_range(vb);
+	else
+		/* We can only do one array worth at a time. */
+		num = min(num, ARRAY_SIZE(vb->pfns));
 
 	mutex_lock(&vb->balloon_lock);
 	for (vb->num_pfns = 0; vb->num_pfns < num;
@@ -159,7 +300,10 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
 			msleep(200);
 			break;
 		}
-		set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
+		if (use_bmap)
+			update_pfn_range(vb, page);
+		else
+			set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
 		vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
 		if (!virtio_has_feature(vb->vdev,
 					VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
@@ -168,8 +312,13 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
 
 	num_allocated_pages = vb->num_pfns;
 	/* Did we get any? */
-	if (vb->num_pfns != 0)
-		tell_host(vb, vb->inflate_vq);
+	if (vb->num_pfns != 0) {
+		if (use_bmap)
+			set_page_bitmap(vb, &vb_dev_info->pages,
+					vb->inflate_vq);
+		else
+			tell_host(vb, vb->inflate_vq);
+	}
 	mutex_unlock(&vb->balloon_lock);
 
 	return num_allocated_pages;
@@ -189,15 +338,19 @@ static void release_pages_balloon(struct virtio_balloon *vb,
 	}
 }
 
-static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
+static unsigned int leak_balloon(struct virtio_balloon *vb, size_t num,
+				bool use_bmap)
 {
-	unsigned num_freed_pages;
+	unsigned int num_freed_pages;
 	struct page *page;
 	struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
 	LIST_HEAD(pages);
 
-	/* We can only do one array worth at a time. */
-	num = min(num, ARRAY_SIZE(vb->pfns));
+	if (use_bmap)
+		init_pfn_range(vb);
+	else
+		/* We can only do one array worth at a time. */
+		num = min(num, ARRAY_SIZE(vb->pfns));
 
 	mutex_lock(&vb->balloon_lock);
 	/* We can't release more pages than taken */
@@ -207,7 +360,10 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
 		page = balloon_page_dequeue(vb_dev_info);
 		if (!page)
 			break;
-		set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
+		if (use_bmap)
+			update_pfn_range(vb, page);
+		else
+			set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
 		list_add(&page->lru, &pages);
 		vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
 	}
@@ -218,8 +374,14 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
 	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
 	 * is true, we *have* to do it in this order
 	 */
-	if (vb->num_pfns != 0)
-		tell_host(vb, vb->deflate_vq);
+	if (vb->num_pfns != 0) {
+		if (use_bmap)
+			set_page_bitmap(vb, &pages, vb->deflate_vq);
+		else
+			tell_host(vb, vb->deflate_vq);
+
+		release_pages_balloon(vb, &pages);
+	}
 	release_pages_balloon(vb, &pages);
 	mutex_unlock(&vb->balloon_lock);
 	return num_freed_pages;
@@ -354,13 +516,15 @@ static int virtballoon_oom_notify(struct notifier_block *self,
 	struct virtio_balloon *vb;
 	unsigned long *freed;
 	unsigned num_freed_pages;
+	bool use_bmap;
 
 	vb = container_of(self, struct virtio_balloon, nb);
 	if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
 		return NOTIFY_OK;
 
 	freed = parm;
-	num_freed_pages = leak_balloon(vb, oom_pages);
+	use_bmap = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP);
+	num_freed_pages = leak_balloon(vb, oom_pages, use_bmap);
 	update_balloon_size(vb);
 	*freed += num_freed_pages;
 
@@ -380,15 +544,19 @@ static void update_balloon_size_func(struct work_struct *work)
 {
 	struct virtio_balloon *vb;
 	s64 diff;
+	bool use_bmap;
 
 	vb = container_of(work, struct virtio_balloon,
 			  update_balloon_size_work);
 	diff = towards_target(vb);
+	use_bmap = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP);
+	if (use_bmap && diff && vb->nr_page_bmap == 1)
+		extend_page_bitmap(vb);
 
 	if (diff > 0)
-		diff -= fill_balloon(vb, diff);
+		diff -= fill_balloon(vb, diff, use_bmap);
 	else if (diff < 0)
-		diff += leak_balloon(vb, -diff);
+		diff += leak_balloon(vb, -diff, use_bmap);
 	update_balloon_size(vb);
 
 	if (diff)
@@ -533,6 +701,17 @@ static int virtballoon_probe(struct virtio_device *vdev)
 	spin_lock_init(&vb->stop_update_lock);
 	vb->stop_update = false;
 	vb->num_pages = 0;
+	vb->bmap_hdr = kzalloc(sizeof(struct balloon_bmap_hdr), GFP_KERNEL);
+	/* Clear the feature bit if memory allocation fails */
+	if (!vb->bmap_hdr)
+		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_BITMAP);
+	else {
+		vb->page_bitmap[0] = kmalloc(BALLOON_BMAP_SIZE, GFP_KERNEL);
+		if (!vb->page_bitmap[0])
+			__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_BITMAP);
+		else
+			vb->nr_page_bmap = 1;
+	}
 	mutex_init(&vb->balloon_lock);
 	init_waitqueue_head(&vb->acked);
 	vb->vdev = vdev;
@@ -583,9 +762,12 @@ static int virtballoon_probe(struct virtio_device *vdev)
 
 static void remove_common(struct virtio_balloon *vb)
 {
+	bool use_bmap;
+
+	use_bmap = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP);
 	/* There might be pages left in the balloon: free them. */
 	while (vb->num_pages)
-		leak_balloon(vb, vb->num_pages);
+		leak_balloon(vb, vb->num_pages, use_bmap);
 	update_balloon_size(vb);
 
 	/* Now we reset the device so we can clean up the queues. */
@@ -609,6 +791,8 @@ static void virtballoon_remove(struct virtio_device *vdev)
 	remove_common(vb);
 	if (vb->vb_dev_info.inode)
 		iput(vb->vb_dev_info.inode);
+	kfree_page_bitmap(vb);
+	kfree(vb->bmap_hdr);
 	kfree(vb);
 }
 
@@ -647,6 +831,7 @@ static int virtballoon_restore(struct virtio_device *vdev)
 	VIRTIO_BALLOON_F_MUST_TELL_HOST,
 	VIRTIO_BALLOON_F_STATS_VQ,
 	VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
+	VIRTIO_BALLOON_F_PAGE_BITMAP,
 };
 
 static struct virtio_driver virtio_balloon_driver = {
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1508006 — Re: [RESEND PATCH v3 kernel 4/7] virtio-balloon: speed up inflate/deflate process

On Fri, Oct 21, 2016 at 02:24:37PM +0800, Liang Li wrote:
> The implementation of the current virtio-balloon is not very
> efficient, the time spends on different stages of inflating
> the balloon to 7GB of a 8GB idle guest:
> 
> a. allocating pages (6.5%)
> b. sending PFNs to host (68.3%)
> c. address translation (6.1%)
> d. madvise (19%)
> 
> It takes about 4126ms for the inflating process to complete.
> Debugging shows that the bottle neck are the stage b and stage d.
> 
> If using a bitmap to send the page info instead of the PFNs, we
> can reduce the overhead in stage b quite a lot. Furthermore, we
> can do the address translation and call madvise() with a bulk of
> RAM pages, instead of the current page per page way, 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. With this new feature, inflating the
> balloon to 7GB of a 8GB idle guest only takes 590ms, the
> performance improvement is about 85%.
> 
> TODO: optimize stage a by allocating/freeing a chunk of pages
> instead of a single page at a time.
> 
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/virtio/virtio_balloon.c | 233 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 209 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 59ffe5a..c31839c 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -42,6 +42,10 @@
>  #define OOM_VBALLOON_DEFAULT_PAGES 256
>  #define VIRTBALLOON_OOM_NOTIFY_PRIORITY 80
>  
> +#define BALLOON_BMAP_SIZE	(8 * PAGE_SIZE)
> +#define PFNS_PER_BMAP		(BALLOON_BMAP_SIZE * BITS_PER_BYTE)
> +#define BALLOON_BMAP_COUNT	32
> +
>  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");
> @@ -67,6 +71,13 @@ struct virtio_balloon {
>  
>  	/* Number of balloon pages we've told the Host we're not using. */
>  	unsigned int num_pages;
> +	/* Pointer of the bitmap header. */
> +	void *bmap_hdr;
> +	/* Bitmap and bitmap count used to tell the host the pages */
> +	unsigned long *page_bitmap[BALLOON_BMAP_COUNT];
> +	unsigned int nr_page_bmap;
> +	/* Used to record the processed pfn range */
> +	unsigned long min_pfn, max_pfn, start_pfn, end_pfn;
>  	/*
>  	 * The pages we've told the Host we're not using are enqueued
>  	 * at vb_dev_info->pages list.
> @@ -110,16 +121,66 @@ static void balloon_ack(struct virtqueue *vq)
>  	wake_up(&vb->acked);
>  }
>  
> +static inline void init_pfn_range(struct virtio_balloon *vb)
> +{
> +	vb->min_pfn = ULONG_MAX;
> +	vb->max_pfn = 0;
> +}
> +
> +static inline void update_pfn_range(struct virtio_balloon *vb,
> +				 struct page *page)
> +{
> +	unsigned long balloon_pfn = page_to_balloon_pfn(page);
> +
> +	if (balloon_pfn < vb->min_pfn)
> +		vb->min_pfn = balloon_pfn;
> +	if (balloon_pfn > vb->max_pfn)
> +		vb->max_pfn = balloon_pfn;
> +}
> +

rename to hint these are all bitmap related.


>  static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
>  {
> -	struct scatterlist sg;
> -	unsigned int len;
> +	struct scatterlist sg, sg2[BALLOON_BMAP_COUNT + 1];
> +	unsigned int len, i;
> +
> +	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP)) {
> +		struct balloon_bmap_hdr *hdr = vb->bmap_hdr;
> +		unsigned long bmap_len;
> +		int nr_pfn, nr_used_bmap, nr_buf;
> +
> +		nr_pfn = vb->end_pfn - vb->start_pfn + 1;
> +		nr_pfn = roundup(nr_pfn, BITS_PER_LONG);
> +		nr_used_bmap = nr_pfn / PFNS_PER_BMAP;
> +		bmap_len = nr_pfn / BITS_PER_BYTE;
> +		nr_buf = nr_used_bmap + 1;
> +
> +		/* cmd, reserved and req_id are init to 0, unused here */
> +		hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT);
> +		hdr->start_pfn = cpu_to_virtio64(vb->vdev, vb->start_pfn);
> +		hdr->bmap_len = cpu_to_virtio64(vb->vdev, bmap_len);
> +		sg_init_table(sg2, nr_buf);
> +		sg_set_buf(&sg2[0], hdr, sizeof(struct balloon_bmap_hdr));
> +		for (i = 0; i < nr_used_bmap; i++) {
> +			unsigned int  buf_len = BALLOON_BMAP_SIZE;
> +
> +			if (i + 1 == nr_used_bmap)
> +				buf_len = bmap_len - BALLOON_BMAP_SIZE * i;
> +			sg_set_buf(&sg2[i + 1], vb->page_bitmap[i], buf_len);
> +		}
>  
> -	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> +		while (vq->num_free < nr_buf)
> +			msleep(2);


What's going on here? Who is expected to update num_free?



> +		if (virtqueue_add_outbuf(vq, sg2, nr_buf, vb, GFP_KERNEL) == 0)
> +			virtqueue_kick(vq);
>  
> -	/* We should always be able to add one buffer to an empty queue. */
> -	virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> -	virtqueue_kick(vq);
> +	} else {
> +		sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> +
> +		/* We should always be able to add one buffer to an empty
> +		 * queue. */

Pls use a multiple comment style consistent with kernel coding style.

> +		virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> +		virtqueue_kick(vq);
> +	}
>  
>  	/* When host has read buffer, this completes via balloon_ack */
>  	wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> @@ -138,13 +199,93 @@ static void set_page_pfns(struct virtio_balloon *vb,
>  					  page_to_balloon_pfn(page) + i);
>  }
>  
> -static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> +static void extend_page_bitmap(struct virtio_balloon *vb)
> +{
> +	int i;
> +	unsigned long bmap_len, bmap_count;
> +
> +	bmap_len = ALIGN(get_max_pfn(), BITS_PER_LONG) / BITS_PER_BYTE;
> +	bmap_count = bmap_len / BALLOON_BMAP_SIZE;
> +	if (bmap_len % BALLOON_BMAP_SIZE)
> +		bmap_count++;
> +	if (bmap_count > BALLOON_BMAP_COUNT)
> +		bmap_count = BALLOON_BMAP_COUNT;
> +

This is doing simple things in tricky ways.
Please use macros such as ALIGN and max instead of if.


> +	for (i = 1; i < bmap_count; i++) {

why 1?

> +		vb->page_bitmap[i] = kmalloc(BALLOON_BMAP_SIZE, GFP_ATOMIC);

why GFP_ATOMIC?
and what will free the previous buffer?


> +		if (vb->page_bitmap[i])
> +			vb->nr_page_bmap++;
> +		else
> +			break;

and what will happen then?

> +	}
> +}
> +
> +static void kfree_page_bitmap(struct virtio_balloon *vb)
> +{
> +	int i;
> +
> +	for (i = 0; i < vb->nr_page_bmap; i++)
> +		kfree(vb->page_bitmap[i]);
> +}
> +
> +static void clear_page_bitmap(struct virtio_balloon *vb)
> +{
> +	int i;
> +
> +	for (i = 0; i < vb->nr_page_bmap; i++)
> +		memset(vb->page_bitmap[i], 0, BALLOON_BMAP_SIZE);
> +}
> +
> +static void set_page_bitmap(struct virtio_balloon *vb,
> +			 struct list_head *pages, struct virtqueue *vq)
> +{
> +	unsigned long pfn, pfn_limit;
> +	struct page *page;
> +	bool found;
> +	int bmap_idx;
> +
> +	vb->min_pfn = rounddown(vb->min_pfn, BITS_PER_LONG);
> +	vb->max_pfn = roundup(vb->max_pfn, BITS_PER_LONG);
> +	pfn_limit = PFNS_PER_BMAP * vb->nr_page_bmap;
> +
> +	for (pfn = vb->min_pfn; pfn < vb->max_pfn; pfn += pfn_limit) {
> +		unsigned long end_pfn;
> +
> +		clear_page_bitmap(vb);
> +		vb->start_pfn = pfn;
> +		end_pfn = pfn;
> +		found = false;
> +		list_for_each_entry(page, pages, lru) {
> +			unsigned long pos, balloon_pfn;
> +
> +			balloon_pfn = page_to_balloon_pfn(page);
> +			if (balloon_pfn < pfn || balloon_pfn >= pfn + pfn_limit)
> +				continue;
> +			bmap_idx = (balloon_pfn - pfn) / PFNS_PER_BMAP;
> +			pos = (balloon_pfn - pfn) % PFNS_PER_BMAP;
> +			set_bit(pos, vb->page_bitmap[bmap_idx]);
> +			if (balloon_pfn > end_pfn)
> +				end_pfn = balloon_pfn;
> +			found = true;
> +		}
> +		if (found) {
> +			vb->end_pfn = end_pfn;
> +			tell_host(vb, vq);
> +		}
> +	}
> +}
> +
> +static unsigned int fill_balloon(struct virtio_balloon *vb, size_t num,
> +				 bool use_bmap)
>  {
>  	struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> -	unsigned num_allocated_pages;
> +	unsigned int num_allocated_pages;
>  
> -	/* We can only do one array worth at a time. */
> -	num = min(num, ARRAY_SIZE(vb->pfns));
> +	if (use_bmap)
> +		init_pfn_range(vb);
> +	else
> +		/* We can only do one array worth at a time. */
> +		num = min(num, ARRAY_SIZE(vb->pfns));
>  
>  	mutex_lock(&vb->balloon_lock);
>  	for (vb->num_pfns = 0; vb->num_pfns < num;
> @@ -159,7 +300,10 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
>  			msleep(200);
>  			break;
>  		}
> -		set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +		if (use_bmap)
> +			update_pfn_range(vb, page);
> +		else
> +			set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
>  		vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
>  		if (!virtio_has_feature(vb->vdev,
>  					VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> @@ -168,8 +312,13 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
>  
>  	num_allocated_pages = vb->num_pfns;
>  	/* Did we get any? */
> -	if (vb->num_pfns != 0)
> -		tell_host(vb, vb->inflate_vq);
> +	if (vb->num_pfns != 0) {
> +		if (use_bmap)
> +			set_page_bitmap(vb, &vb_dev_info->pages,
> +					vb->inflate_vq);
> +		else
> +			tell_host(vb, vb->inflate_vq);
> +	}
>  	mutex_unlock(&vb->balloon_lock);
>  
>  	return num_allocated_pages;
> @@ -189,15 +338,19 @@ static void release_pages_balloon(struct virtio_balloon *vb,
>  	}
>  }
>  
> -static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> +static unsigned int leak_balloon(struct virtio_balloon *vb, size_t num,
> +				bool use_bmap)

this is just a feature bit - why not get it internally?

>  {
> -	unsigned num_freed_pages;
> +	unsigned int num_freed_pages;
>  	struct page *page;
>  	struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
>  	LIST_HEAD(pages);
>  
> -	/* We can only do one array worth at a time. */
> -	num = min(num, ARRAY_SIZE(vb->pfns));
> +	if (use_bmap)
> +		init_pfn_range(vb);
> +	else
> +		/* We can only do one array worth at a time. */
> +		num = min(num, ARRAY_SIZE(vb->pfns));
>  
>  	mutex_lock(&vb->balloon_lock);
>  	/* We can't release more pages than taken */
> @@ -207,7 +360,10 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
>  		page = balloon_page_dequeue(vb_dev_info);
>  		if (!page)
>  			break;
> -		set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +		if (use_bmap)
> +			update_pfn_range(vb, page);
> +		else
> +			set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
>  		list_add(&page->lru, &pages);
>  		vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
>  	}
> @@ -218,8 +374,14 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
>  	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
>  	 * is true, we *have* to do it in this order
>  	 */
> -	if (vb->num_pfns != 0)
> -		tell_host(vb, vb->deflate_vq);
> +	if (vb->num_pfns != 0) {
> +		if (use_bmap)
> +			set_page_bitmap(vb, &pages, vb->deflate_vq);
> +		else
> +			tell_host(vb, vb->deflate_vq);
> +
> +		release_pages_balloon(vb, &pages);
> +	}
>  	release_pages_balloon(vb, &pages);
>  	mutex_unlock(&vb->balloon_lock);
>  	return num_freed_pages;
> @@ -354,13 +516,15 @@ static int virtballoon_oom_notify(struct notifier_block *self,
>  	struct virtio_balloon *vb;
>  	unsigned long *freed;
>  	unsigned num_freed_pages;
> +	bool use_bmap;
>  
>  	vb = container_of(self, struct virtio_balloon, nb);
>  	if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
>  		return NOTIFY_OK;
>  
>  	freed = parm;
> -	num_freed_pages = leak_balloon(vb, oom_pages);
> +	use_bmap = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP);
> +	num_freed_pages = leak_balloon(vb, oom_pages, use_bmap);
>  	update_balloon_size(vb);
>  	*freed += num_freed_pages;
>  
> @@ -380,15 +544,19 @@ static void update_balloon_size_func(struct work_struct *work)
>  {
>  	struct virtio_balloon *vb;
>  	s64 diff;
> +	bool use_bmap;
>  
>  	vb = container_of(work, struct virtio_balloon,
>  			  update_balloon_size_work);
>  	diff = towards_target(vb);
> +	use_bmap = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP);
> +	if (use_bmap && diff && vb->nr_page_bmap == 1)
> +		extend_page_bitmap(vb);

So you allocate it on first use, then keep it around until
device remove? Seems ugly.
Needs comments explaining the motivation for this.
Can't we free it immediately when it becomes unused?

>  
>  	if (diff > 0)
> -		diff -= fill_balloon(vb, diff);
> +		diff -= fill_balloon(vb, diff, use_bmap);
>  	else if (diff < 0)
> -		diff += leak_balloon(vb, -diff);
> +		diff += leak_balloon(vb, -diff, use_bmap);
>  	update_balloon_size(vb);
>  
>  	if (diff)
> @@ -533,6 +701,17 @@ static int virtballoon_probe(struct virtio_device *vdev)
>  	spin_lock_init(&vb->stop_update_lock);
>  	vb->stop_update = false;
>  	vb->num_pages = 0;
> +	vb->bmap_hdr = kzalloc(sizeof(struct balloon_bmap_hdr), GFP_KERNEL);
> +	/* Clear the feature bit if memory allocation fails */
> +	if (!vb->bmap_hdr)
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_BITMAP);
> +	else {
> +		vb->page_bitmap[0] = kmalloc(BALLOON_BMAP_SIZE, GFP_KERNEL);
> +		if (!vb->page_bitmap[0])
> +			__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_BITMAP);
> +		else
> +			vb->nr_page_bmap = 1;
> +	}
>  	mutex_init(&vb->balloon_lock);
>  	init_waitqueue_head(&vb->acked);
>  	vb->vdev = vdev;
> @@ -583,9 +762,12 @@ static int virtballoon_probe(struct virtio_device *vdev)
>  
>  static void remove_common(struct virtio_balloon *vb)
>  {
> +	bool use_bmap;
> +
> +	use_bmap = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP);
>  	/* There might be pages left in the balloon: free them. */
>  	while (vb->num_pages)
> -		leak_balloon(vb, vb->num_pages);
> +		leak_balloon(vb, vb->num_pages, use_bmap);
>  	update_balloon_size(vb);
>  
>  	/* Now we reset the device so we can clean up the queues. */
> @@ -609,6 +791,8 @@ static void virtballoon_remove(struct virtio_device *vdev)
>  	remove_common(vb);
>  	if (vb->vb_dev_info.inode)
>  		iput(vb->vb_dev_info.inode);
> +	kfree_page_bitmap(vb);
> +	kfree(vb->bmap_hdr);
>  	kfree(vb);
>  }
>  
> @@ -647,6 +831,7 @@ static int virtballoon_restore(struct virtio_device *vdev)
>  	VIRTIO_BALLOON_F_MUST_TELL_HOST,
>  	VIRTIO_BALLOON_F_STATS_VQ,
>  	VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
> +	VIRTIO_BALLOON_F_PAGE_BITMAP,
>  };
>  
>  static struct virtio_driver virtio_balloon_driver = {
> -- 
> 1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1508129 — RE: [RESEND PATCH v3 kernel 4/7] virtio-balloon: speed up inflate/deflate process

> > +static inline void init_pfn_range(struct virtio_balloon *vb) {
> > +	vb->min_pfn = ULONG_MAX;
> > +	vb->max_pfn = 0;
> > +}
> > +
> > +static inline void update_pfn_range(struct virtio_balloon *vb,
> > +				 struct page *page)
> > +{
> > +	unsigned long balloon_pfn = page_to_balloon_pfn(page);
> > +
> > +	if (balloon_pfn < vb->min_pfn)
> > +		vb->min_pfn = balloon_pfn;
> > +	if (balloon_pfn > vb->max_pfn)
> > +		vb->max_pfn = balloon_pfn;
> > +}
> > +
> 
> rename to hint these are all bitmap related.

Will change in v4.

> 
> 
> >  static void tell_host(struct virtio_balloon *vb, struct virtqueue
> > *vq)  {
> > -	struct scatterlist sg;
> > -	unsigned int len;
> > +	struct scatterlist sg, sg2[BALLOON_BMAP_COUNT + 1];
> > +	unsigned int len, i;
> > +
> > +	if (virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_PAGE_BITMAP)) {
> > +		struct balloon_bmap_hdr *hdr = vb->bmap_hdr;
> > +		unsigned long bmap_len;
> > +		int nr_pfn, nr_used_bmap, nr_buf;
> > +
> > +		nr_pfn = vb->end_pfn - vb->start_pfn + 1;
> > +		nr_pfn = roundup(nr_pfn, BITS_PER_LONG);
> > +		nr_used_bmap = nr_pfn / PFNS_PER_BMAP;
> > +		bmap_len = nr_pfn / BITS_PER_BYTE;
> > +		nr_buf = nr_used_bmap + 1;
> > +
> > +		/* cmd, reserved and req_id are init to 0, unused here */
> > +		hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT);
> > +		hdr->start_pfn = cpu_to_virtio64(vb->vdev, vb->start_pfn);
> > +		hdr->bmap_len = cpu_to_virtio64(vb->vdev, bmap_len);
> > +		sg_init_table(sg2, nr_buf);
> > +		sg_set_buf(&sg2[0], hdr, sizeof(struct balloon_bmap_hdr));
> > +		for (i = 0; i < nr_used_bmap; i++) {
> > +			unsigned int  buf_len = BALLOON_BMAP_SIZE;
> > +
> > +			if (i + 1 == nr_used_bmap)
> > +				buf_len = bmap_len - BALLOON_BMAP_SIZE
> * i;
> > +			sg_set_buf(&sg2[i + 1], vb->page_bitmap[i],
> buf_len);
> > +		}
> >
> > -	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> > +		while (vq->num_free < nr_buf)
> > +			msleep(2);
> 
> 
> What's going on here? Who is expected to update num_free?
> 

I just want to wait until the vq have enough space to write the bitmap, I thought qemu
side will update the vq->num_free, is it wrong?

> 
> 
> > +		if (virtqueue_add_outbuf(vq, sg2, nr_buf, vb, GFP_KERNEL)
> == 0)
> > +			virtqueue_kick(vq);
> >
> > -	/* We should always be able to add one buffer to an empty queue.
> */
> > -	virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> > -	virtqueue_kick(vq);
> > +	} else {
> > +		sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb-
> >num_pfns);
> > +
> > +		/* We should always be able to add one buffer to an empty
> > +		 * queue. */
> 
> Pls use a multiple comment style consistent with kernel coding style.

Will change in next version.

> 
> > +		virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> > +		virtqueue_kick(vq);
> > +	}
> >
> >  	/* When host has read buffer, this completes via balloon_ack */
> >  	wait_event(vb->acked, virtqueue_get_buf(vq, &len)); @@ -138,13
> > +199,93 @@ static void set_page_pfns(struct virtio_balloon *vb,
> >  					  page_to_balloon_pfn(page) + i);  }
> >
> > -static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> > +static void extend_page_bitmap(struct virtio_balloon *vb) {
> > +	int i;
> > +	unsigned long bmap_len, bmap_count;
> > +
> > +	bmap_len = ALIGN(get_max_pfn(), BITS_PER_LONG) /
> BITS_PER_BYTE;
> > +	bmap_count = bmap_len / BALLOON_BMAP_SIZE;
> > +	if (bmap_len % BALLOON_BMAP_SIZE)
> > +		bmap_count++;
> > +	if (bmap_count > BALLOON_BMAP_COUNT)
> > +		bmap_count = BALLOON_BMAP_COUNT;
> > +
> 
> This is doing simple things in tricky ways.
> Please use macros such as ALIGN and max instead of if.
> 

Will change.

> 
> > +	for (i = 1; i < bmap_count; i++) {
> 
> why 1?

In probe stage, already allocated one bitmap.

> 
> > +		vb->page_bitmap[i] = kmalloc(BALLOON_BMAP_SIZE,
> GFP_ATOMIC);
> 
> why GFP_ATOMIC?

Yes, GFP_ATOMIC is not necessary.

> and what will free the previous buffer?

The previous buffer will not be freed.

> 
> 
> > +		if (vb->page_bitmap[i])
> > +			vb->nr_page_bmap++;
> > +		else
> > +			break;
> 
> and what will happen then?

I plan to use the previous allocated buffer to save the bitmap, need more code for kmalloc failure?

> > -static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> > +static unsigned int leak_balloon(struct virtio_balloon *vb, size_t num,
> > +				bool use_bmap)
> 
> this is just a feature bit - why not get it internally?

Indeed.

> > @@ -218,8 +374,14 @@ static unsigned leak_balloon(struct virtio_balloon
> *vb, size_t num)
> >  	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> >  	 * is true, we *have* to do it in this order
> >  	 */
> > -	if (vb->num_pfns != 0)
> > -		tell_host(vb, vb->deflate_vq);
> > +	if (vb->num_pfns != 0) {
> > +		if (use_bmap)
> > +			set_page_bitmap(vb, &pages, vb->deflate_vq);
> > +		else
> > +			tell_host(vb, vb->deflate_vq);
> > +
> > +		release_pages_balloon(vb, &pages);
> > +	}
> >  	release_pages_balloon(vb, &pages);
> >  	mutex_unlock(&vb->balloon_lock);
> >  	return num_freed_pages;
> > @@ -354,13 +516,15 @@ static int virtballoon_oom_notify(struct
> notifier_block *self,
> >  	struct virtio_balloon *vb;
> >  	unsigned long *freed;
> >  	unsigned num_freed_pages;
> > +	bool use_bmap;
> >
> >  	vb = container_of(self, struct virtio_balloon, nb);
> >  	if (!virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> >  		return NOTIFY_OK;
> >
> >  	freed = parm;
> > -	num_freed_pages = leak_balloon(vb, oom_pages);
> > +	use_bmap = virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_PAGE_BITMAP);
> > +	num_freed_pages = leak_balloon(vb, oom_pages, use_bmap);
> >  	update_balloon_size(vb);
> >  	*freed += num_freed_pages;
> >
> > @@ -380,15 +544,19 @@ static void update_balloon_size_func(struct
> > work_struct *work)  {
> >  	struct virtio_balloon *vb;
> >  	s64 diff;
> > +	bool use_bmap;
> >
> >  	vb = container_of(work, struct virtio_balloon,
> >  			  update_balloon_size_work);
> >  	diff = towards_target(vb);
> > +	use_bmap = virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_PAGE_BITMAP);
> > +	if (use_bmap && diff && vb->nr_page_bmap == 1)
> > +		extend_page_bitmap(vb);
> 
> So you allocate it on first use, then keep it around until device remove?
> Seems ugly.

Yes, this version behave like this.

> Needs comments explaining the motivation for this.
> Can't we free it immediately when it becomes unused?
> 

Yes, it can be freed immediately, will change in v4.

Thanks for your time and your valuable comments! I will send out the v4 soon.

Liang

[toc] | [prev] | [next] | [standalone]

#1505502 — [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature bit and page bitmap head

Add a new feature which supports sending the page information with
a bitmap. The current implementation uses PFNs array, which is not
very efficient. Using bitmap can improve the performance of
inflating/deflating significantly

The page bitmap header will used to tell the host some information
about the page bitmap. e.g. the page size, page bitmap length and
start pfn.

Signed-off-by: Liang Li <liang.z.li@intel.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Amit Shah <amit.shah@redhat.com>
---
 include/uapi/linux/virtio_balloon.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index 343d7dd..d3b182a 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -34,6 +34,7 @@
 #define VIRTIO_BALLOON_F_MUST_TELL_HOST	0 /* Tell before reclaiming pages */
 #define VIRTIO_BALLOON_F_STATS_VQ	1 /* Memory Stats virtqueue */
 #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM	2 /* Deflate balloon on OOM */
+#define VIRTIO_BALLOON_F_PAGE_BITMAP	3 /* Send page info with bitmap */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12
@@ -82,4 +83,22 @@ struct virtio_balloon_stat {
 	__virtio64 val;
 } __attribute__((packed));
 
+/* Page bitmap header structure */
+struct balloon_bmap_hdr {
+	/* Used to distinguish different request */
+	__virtio16 cmd;
+	/* Shift width of page in the bitmap */
+	__virtio16 page_shift;
+	/* flag used to identify different status */
+	__virtio16 flag;
+	/* Reserved */
+	__virtio16 reserved;
+	/* ID of the request */
+	__virtio64 req_id;
+	/* The pfn of 0 bit in the bitmap */
+	__virtio64 start_pfn;
+	/* The length of the bitmap, in bytes */
+	__virtio64 bmap_len;
+};
+
 #endif /* _LINUX_VIRTIO_BALLOON_H */
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1507454 — Re: [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature bit and page bitmap head

On 10/20/2016 11:24 PM, Liang Li wrote:
> Add a new feature which supports sending the page information with
> a bitmap. The current implementation uses PFNs array, which is not
> very efficient. Using bitmap can improve the performance of
> inflating/deflating significantly

Why is it not efficient?  How is using a bitmap more efficient?  What
kinds of cases is the bitmap inefficient?

> The page bitmap header will used to tell the host some information
> about the page bitmap. e.g. the page size, page bitmap length and
> start pfn.

Why did you choose to add these features to the structure?  What
benefits do they add?

Could you describe your solution a bit here, and describe its strengths
and weaknesses?

The same comments apply, even if (especially if) you change the data
structure.

> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Amit Shah <amit.shah@redhat.com>
> ---
>  include/uapi/linux/virtio_balloon.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 343d7dd..d3b182a 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -34,6 +34,7 @@
>  #define VIRTIO_BALLOON_F_MUST_TELL_HOST	0 /* Tell before reclaiming pages */
>  #define VIRTIO_BALLOON_F_STATS_VQ	1 /* Memory Stats virtqueue */
>  #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM	2 /* Deflate balloon on OOM */
> +#define VIRTIO_BALLOON_F_PAGE_BITMAP	3 /* Send page info with bitmap */
>  
>  /* Size of a PFN in the balloon interface. */
>  #define VIRTIO_BALLOON_PFN_SHIFT 12
> @@ -82,4 +83,22 @@ struct virtio_balloon_stat {
>  	__virtio64 val;
>  } __attribute__((packed));
>  
> +/* Page bitmap header structure */
> +struct balloon_bmap_hdr {
> +	/* Used to distinguish different request */
> +	__virtio16 cmd;
> +	/* Shift width of page in the bitmap */
> +	__virtio16 page_shift;
> +	/* flag used to identify different status */
> +	__virtio16 flag;
> +	/* Reserved */
> +	__virtio16 reserved;
> +	/* ID of the request */
> +	__virtio64 req_id;
> +	/* The pfn of 0 bit in the bitmap */
> +	__virtio64 start_pfn;
> +	/* The length of the bitmap, in bytes */
> +	__virtio64 bmap_len;
> +};

FWIW this is totally unreadable.  Please do something like this:

> +struct balloon_bmap_hdr {
> +	__virtio16 cmd; 	/* Used to distinguish different ...
> +	__virtio16 page_shift; 	/* Shift width of page in the bitmap */
> +	__virtio16 flag; 	/* flag used to identify different...
> +	__virtio16 reserved;	/* Reserved */
> +	__virtio64 req_id;	/* ID of the request */
> +	__virtio64 start_pfn;	/* The pfn of 0 bit in the bitmap */
> +	__virtio64 bmap_len;	/* The length of the bitmap, in bytes */
> +};

and please make an effort to add useful comments.  "/* Reserved */"
seems like a waste of bytes to me.

[toc] | [prev] | [next] | [standalone]

#1507910 — RE: [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature bit and page bitmap head

> On 10/20/2016 11:24 PM, Liang Li wrote:
> > Add a new feature which supports sending the page information with a
> > bitmap. The current implementation uses PFNs array, which is not very
> > efficient. Using bitmap can improve the performance of
> > inflating/deflating significantly
> 
> Why is it not efficient?  How is using a bitmap more efficient?  What kinds of
> cases is the bitmap inefficient?
> 
> > The page bitmap header will used to tell the host some information
> > about the page bitmap. e.g. the page size, page bitmap length and
> > start pfn.
> 
> Why did you choose to add these features to the structure?  What benefits
> do they add?
> 
> Could you describe your solution a bit here, and describe its strengths and
> weaknesses?
> 

Will elaborate the solution in V4.

> >  /* Size of a PFN in the balloon interface. */  #define
> > VIRTIO_BALLOON_PFN_SHIFT 12 @@ -82,4 +83,22 @@ struct
> > virtio_balloon_stat {
> >  	__virtio64 val;
> >  } __attribute__((packed));
> >
> > +/* Page bitmap header structure */
> > +struct balloon_bmap_hdr {
> > +	/* Used to distinguish different request */
> > +	__virtio16 cmd;
> > +	/* Shift width of page in the bitmap */
> > +	__virtio16 page_shift;
> > +	/* flag used to identify different status */
> > +	__virtio16 flag;
> > +	/* Reserved */
> > +	__virtio16 reserved;
> > +	/* ID of the request */
> > +	__virtio64 req_id;
> > +	/* The pfn of 0 bit in the bitmap */
> > +	__virtio64 start_pfn;
> > +	/* The length of the bitmap, in bytes */
> > +	__virtio64 bmap_len;
> > +};
> 
> FWIW this is totally unreadable.  Please do something like this:
> 
> > +struct balloon_bmap_hdr {
> > +	__virtio16 cmd; 	/* Used to distinguish different ...
> > +	__virtio16 page_shift; 	/* Shift width of page in the bitmap */
> > +	__virtio16 flag; 	/* flag used to identify different...
> > +	__virtio16 reserved;	/* Reserved */
> > +	__virtio64 req_id;	/* ID of the request */
> > +	__virtio64 start_pfn;	/* The pfn of 0 bit in the bitmap */
> > +	__virtio64 bmap_len;	/* The length of the bitmap, in bytes */
> > +};
> 
> and please make an effort to add useful comments.  "/* Reserved */"
> seems like a waste of bytes to me.

OK. Maybe 'padding' is better than 'reserved' .

Thanks for your comments!

Liang

[toc] | [prev] | [next] | [standalone]

#1509535 — Re: [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature bit and page bitmap head

On Fri, Oct 21, 2016 at 02:24:35PM +0800, Liang Li wrote:
> Add a new feature which supports sending the page information with
> a bitmap. The current implementation uses PFNs array, which is not
> very efficient. Using bitmap can improve the performance of
> inflating/deflating significantly
> 
> The page bitmap header will used to tell the host some information
> about the page bitmap. e.g. the page size, page bitmap length and
> start pfn.
> 
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Amit Shah <amit.shah@redhat.com>
> ---
>  include/uapi/linux/virtio_balloon.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 343d7dd..d3b182a 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -34,6 +34,7 @@
>  #define VIRTIO_BALLOON_F_MUST_TELL_HOST	0 /* Tell before reclaiming pages */
>  #define VIRTIO_BALLOON_F_STATS_VQ	1 /* Memory Stats virtqueue */
>  #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM	2 /* Deflate balloon on OOM */
> +#define VIRTIO_BALLOON_F_PAGE_BITMAP	3 /* Send page info with bitmap */
>  
>  /* Size of a PFN in the balloon interface. */
>  #define VIRTIO_BALLOON_PFN_SHIFT 12
> @@ -82,4 +83,22 @@ struct virtio_balloon_stat {
>  	__virtio64 val;
>  } __attribute__((packed));
>  
> +/* Page bitmap header structure */
> +struct balloon_bmap_hdr {

Should be virtio_balloon.

> +	/* Used to distinguish different request */

different requests? what are the legal values?

> +	__virtio16 cmd;
> +	/* Shift width of page in the bitmap */

In which units?

> +	__virtio16 page_shift;
> +	/* flag used to identify different status */

this comment does not seem to add any value.

> +	__virtio16 flag;
> +	/* Reserved */

this too

> +	__virtio16 reserved;
> +	/* ID of the request */
> +	__virtio64 req_id;
> +	/* The pfn of 0 bit in the bitmap */
> +	__virtio64 start_pfn;
> +	/* The length of the bitmap, in bytes */

Why not in bits?

> +	__virtio64 bmap_len;
> +};
> +
>  #endif /* _LINUX_VIRTIO_BALLOON_H */
> -- 
> 1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1505504 — [RESEND PATCH v3 kernel 1/7] virtio-balloon: rework deflate to add page to a list

Will allow faster notifications using a bitmap down the road.
balloon_pfn_to_page() can be removed because it's useless.

Signed-off-by: Liang Li <liang.z.li@intel.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_balloon.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 4e7003d..59ffe5a 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -103,12 +103,6 @@ static u32 page_to_balloon_pfn(struct page *page)
 	return pfn * VIRTIO_BALLOON_PAGES_PER_PAGE;
 }
 
-static struct page *balloon_pfn_to_page(u32 pfn)
-{
-	BUG_ON(pfn % VIRTIO_BALLOON_PAGES_PER_PAGE);
-	return pfn_to_page(pfn / VIRTIO_BALLOON_PAGES_PER_PAGE);
-}
-
 static void balloon_ack(struct virtqueue *vq)
 {
 	struct virtio_balloon *vb = vq->vdev->priv;
@@ -181,18 +175,16 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
 	return num_allocated_pages;
 }
 
-static void release_pages_balloon(struct virtio_balloon *vb)
+static void release_pages_balloon(struct virtio_balloon *vb,
+				 struct list_head *pages)
 {
-	unsigned int i;
-	struct page *page;
+	struct page *page, *next;
 
-	/* Find pfns pointing at start of each page, get pages and free them. */
-	for (i = 0; i < vb->num_pfns; i += VIRTIO_BALLOON_PAGES_PER_PAGE) {
-		page = balloon_pfn_to_page(virtio32_to_cpu(vb->vdev,
-							   vb->pfns[i]));
+	list_for_each_entry_safe(page, next, pages, lru) {
 		if (!virtio_has_feature(vb->vdev,
 					VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
 			adjust_managed_page_count(page, 1);
+		list_del(&page->lru);
 		put_page(page); /* balloon reference */
 	}
 }
@@ -202,6 +194,7 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
 	unsigned num_freed_pages;
 	struct page *page;
 	struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
+	LIST_HEAD(pages);
 
 	/* We can only do one array worth at a time. */
 	num = min(num, ARRAY_SIZE(vb->pfns));
@@ -215,6 +208,7 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
 		if (!page)
 			break;
 		set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
+		list_add(&page->lru, &pages);
 		vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
 	}
 
@@ -226,7 +220,7 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
 	 */
 	if (vb->num_pfns != 0)
 		tell_host(vb, vb->deflate_vq);
-	release_pages_balloon(vb);
+	release_pages_balloon(vb, &pages);
 	mutex_unlock(&vb->balloon_lock);
 	return num_freed_pages;
 }
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1507439 — Re: [RESEND PATCH v3 kernel 1/7] virtio-balloon: rework deflate to add page to a list

On 10/20/2016 11:24 PM, Liang Li wrote:
> Will allow faster notifications using a bitmap down the road.
> balloon_pfn_to_page() can be removed because it's useless.

This is a pretty terse description of what's going on here.  Could you
try to elaborate a bit?  What *is* the current approach?  Why does it
not work going forward?  What do you propose instead?  Why is it better?

[toc] | [prev] | [next] | [standalone]

#1507902 — RE: [RESEND PATCH v3 kernel 1/7] virtio-balloon: rework deflate to add page to a list

> On 10/20/2016 11:24 PM, Liang Li wrote:
> > Will allow faster notifications using a bitmap down the road.
> > balloon_pfn_to_page() can be removed because it's useless.
> 
> This is a pretty terse description of what's going on here.  Could you try to
> elaborate a bit?  What *is* the current approach?  Why does it not work
> going forward?  What do you propose instead?  Why is it better?

Sure. The description will be more clear if it's described as you suggest. Thanks!

Liang 

[toc] | [prev] | [next] | [standalone]

#1506161 — Re: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration

On 10/20/2016 11:24 PM, Liang Li wrote:
> Dave Hansen suggested a new scheme to encode the data structure,
> because of additional complexity, it's not implemented in v3.

So, what do you want done with this patch set?  Do you want it applied
as-is so that we can introduce a new host/guest ABI that we must support
until the end of time?  Then, we go back in a year or two and add the
newer format that addresses the deficiencies that this ABI has with a
third version?

[toc] | [prev] | [next] | [standalone]

#1506252 — Re: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration

On Fri, Oct 21, 2016 at 10:25:21AM -0700, Dave Hansen wrote:
> On 10/20/2016 11:24 PM, Liang Li wrote:
> > Dave Hansen suggested a new scheme to encode the data structure,
> > because of additional complexity, it's not implemented in v3.
> 
> So, what do you want done with this patch set?  Do you want it applied
> as-is so that we can introduce a new host/guest ABI that we must support
> until the end of time?  Then, we go back in a year or two and add the
> newer format that addresses the deficiencies that this ABI has with a
> third version?
> 

Exactly my questions.

[toc] | [prev] | [next] | [standalone]

#1506675 — RE: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration

> On Fri, Oct 21, 2016 at 10:25:21AM -0700, Dave Hansen wrote:
> > On 10/20/2016 11:24 PM, Liang Li wrote:
> > > Dave Hansen suggested a new scheme to encode the data structure,
> > > because of additional complexity, it's not implemented in v3.
> >
> > So, what do you want done with this patch set?  Do you want it applied
> > as-is so that we can introduce a new host/guest ABI that we must
> > support until the end of time?  Then, we go back in a year or two and
> > add the newer format that addresses the deficiencies that this ABI has
> > with a third version?
> >
> 
> Exactly my questions.

Hi Dave & Michael,

In the V2, both of you thought that the memory I allocated for the bitmap is too large, and gave some
 suggestions about the solution, so I changed the implementation and used  scattered pages for the bitmap
instead of a large physical continued memory. I didn't get the comments about the changes, so I am not 
sure whether that is OK or not, that's the why I resend the V3, I just want your opinions about that part. 

I will implement the new schema as Dave suggested in V4. Before that, could you take a look at this version and
give some comments? 

Thanks!
Liang

[toc] | [prev] | [next] | [standalone]

#1507957 — Re: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration

On Sun, Oct 23, 2016 at 11:29:25AM +0000, Li, Liang Z wrote:
> > On Fri, Oct 21, 2016 at 10:25:21AM -0700, Dave Hansen wrote:
> > > On 10/20/2016 11:24 PM, Liang Li wrote:
> > > > Dave Hansen suggested a new scheme to encode the data structure,
> > > > because of additional complexity, it's not implemented in v3.
> > >
> > > So, what do you want done with this patch set?  Do you want it applied
> > > as-is so that we can introduce a new host/guest ABI that we must
> > > support until the end of time?  Then, we go back in a year or two and
> > > add the newer format that addresses the deficiencies that this ABI has
> > > with a third version?
> > >
> > 
> > Exactly my questions.
> 
> Hi Dave & Michael,
> 
> In the V2, both of you thought that the memory I allocated for the bitmap is too large, and gave some
>  suggestions about the solution, so I changed the implementation and used  scattered pages for the bitmap
> instead of a large physical continued memory. I didn't get the comments about the changes, so I am not 
> sure whether that is OK or not, that's the why I resend the V3, I just want your opinions about that part. 
> 
> I will implement the new schema as Dave suggested in V4. Before that, could you take a look at this version and
> give some comments? 
> 
> Thanks!
> Liang

Sure, I'll try to review just that part.

[toc] | [prev] | [next] | [standalone]

#1509026 — RE: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration

> On 10/20/2016 11:24 PM, Liang Li wrote:
> > Dave Hansen suggested a new scheme to encode the data structure,
> > because of additional complexity, it's not implemented in v3.
> 
> So, what do you want done with this patch set?  Do you want it applied as-is
> so that we can introduce a new host/guest ABI that we must support until
> the end of time?  Then, we go back in a year or two and add the newer
> format that addresses the deficiencies that this ABI has with a third version?
> 

Hi Dave & Michael,

I am working on Dave's new bitmap schema, I have finished the part of getting the 'hybrid scheme bitmap'
and found the complexity was more than I expected.  The main issue is more memory is required to
 save the 'hybrid scheme bitmap' beside that used to save the raw page bitmap, for the worst case, the
memory required is 3 times than that in the previous implementation. 

I am wondering if I should continue, as an alternative solution, how about using PFNs array when
inflating/deflating only a few pages? Things will be much more simple.


Thanks!
Liang 

[toc] | [prev] | [next] | [standalone]

#1509028 — RE: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration

> > On 10/20/2016 11:24 PM, Liang Li wrote:
> > > Dave Hansen suggested a new scheme to encode the data structure,
> > > because of additional complexity, it's not implemented in v3.
> >
> > So, what do you want done with this patch set?  Do you want it applied
> > as-is so that we can introduce a new host/guest ABI that we must
> > support until the end of time?  Then, we go back in a year or two and
> > add the newer format that addresses the deficiencies that this ABI has with
> a third version?
> >
> 
> Hi Dave & Michael,
> 
> I am working on Dave's new bitmap schema, I have finished the part of
> getting the 'hybrid scheme bitmap'
> and found the complexity was more than I expected.  The main issue is more
> memory is required to  save the 'hybrid scheme bitmap' beside that used to
> save the raw page bitmap, for the worst case, the memory required is 3
> times than that in the previous implementation.
> 

3 times memory required is not accurate, please ignore this. sorry ...
The complexity is the point. 

> I am wondering if I should continue, as an alternative solution, how about
> using PFNs array when inflating/deflating only a few pages? Things will be
> much more simple.
> 
> 
> Thanks!
> Liang
> 
> 
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[toc] | [prev] | [next] | [standalone]

#1509685 — Re: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration

On 10/26/2016 03:13 AM, Li, Liang Z wrote:
> 3 times memory required is not accurate, please ignore this. sorry ...
> The complexity is the point. 

What is making it so complex?  Can you describe the problems?

[toc] | [prev] | [next] | [standalone]

#1509996 — RE: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration

> Cc: linux-kernel@vger.kernel.org; virtualization@lists.linux-foundation.org;
> linux-mm@kvack.org; virtio-dev@lists.oasis-open.org; kvm@vger.kernel.org;
> qemu-devel@nongnu.org; quintela@redhat.com; dgilbert@redhat.com;
> pbonzini@redhat.com; cornelia.huck@de.ibm.com; amit.shah@redhat.com
> Subject: Re: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast
> (de)inflating & fast live migration
> 
> On 10/26/2016 03:13 AM, Li, Liang Z wrote:
> > 3 times memory required is not accurate, please ignore this. sorry ...
> > The complexity is the point.
> 
> What is making it so complex?  Can you describe the problems?

I plan to complete it first and send out the patch set,  then discuss if it worth.  I need some time.

Thanks!
Liang

[toc] | [prev] | [next] | [standalone]

#1509679 — Re: [RESEND PATCH v3 kernel 0/7] Extend virtio-balloon for fast (de)inflating & fast live migration

On 10/26/2016 03:06 AM, Li, Liang Z wrote:
> I am working on Dave's new bitmap schema, I have finished the part of
> getting the 'hybrid scheme bitmap' and found the complexity was more
> than I expected. The main issue is more memory is required to save
> the 'hybrid scheme bitmap' beside that used to save the raw page
> bitmap, for the worst case, the memory required is 3 times than that
> in the previous implementation.

Really?  Could you please describe the scenario where this occurs?

> I am wondering if I should continue, as an alternative solution, how about using PFNs array when
> inflating/deflating only a few pages? Things will be much more
> simple.

Yes, using pfn lists is more efficient than using bitmaps for sparse
bitmaps.  Yes, there will be cases where it is preferable to just use
pfn lists vs. any kind of bitmap.

But, what does it matter?  At least with your current scheme where we go
out and collect get_unused_pages(), we do the allocation up front.  The
space efficiency doesn't matter at all for small sizes since we do the
constant-size allocation *anyway*.

I'm also pretty sure you can pack the pfn and page order into a single
64-bit word and have no bitmap for a given record.  That would make it
pack just as well as the old pfns alone.  Right?

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web