| Message ID | 1469582616-5729-5-git-send-email-liang.z.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/26/2016 06:23 PM, Liang Li wrote: > + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; > + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); > + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / > + BITS_PER_BYTE + 2 * sizeof(unsigned long); > + hdr_len = sizeof(struct balloon_bmap_hdr); > + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); This ends up doing a 1MB kmalloc() right? That seems a _bit_ big. How big was the pfn buffer before? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 27, 2016 at 09:23:33AM +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: Andrew Morton <akpm@linux-foundation.org> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Mel Gorman <mgorman@techsingularity.net> > 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 | 184 +++++++++++++++++++++++++++++++++++----- > 1 file changed, 162 insertions(+), 22 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 8d649a2..2d18ff6 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -41,10 +41,28 @@ > #define OOM_VBALLOON_DEFAULT_PAGES 256 > #define VIRTBALLOON_OOM_NOTIFY_PRIORITY 80 > > +/* > + * VIRTIO_BALLOON_PFNS_LIMIT is used to limit the size of page bitmap > + * to prevent a very large page bitmap, there are two reasons for this: > + * 1) to save memory. > + * 2) allocate a large bitmap may fail. > + * > + * The actual limit of pfn is determined by: > + * pfn_limit = min(max_pfn, VIRTIO_BALLOON_PFNS_LIMIT); > + * > + * If system has more pages than VIRTIO_BALLOON_PFNS_LIMIT, we will scan > + * the page list and send the PFNs with several times. To reduce the > + * overhead of scanning the page list. VIRTIO_BALLOON_PFNS_LIMIT should > + * be set with a value which can cover most cases. So what if it covers 1/32 of the memory? We'll do 32 exits and not 1, still not a big deal for a big guest. > + */ > +#define VIRTIO_BALLOON_PFNS_LIMIT ((32 * (1ULL << 30)) >> PAGE_SHIFT) /* 32GB */ I already said this with a smaller limit. 2<< 30 is 2G but that is not a useful comment. pls explain what is the reason for this selection. Still applies here. > + > 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 +80,15 @@ 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 length used to tell the host the pages */ > + unsigned long *page_bitmap; > + unsigned long bmap_len; > + /* Pfn limit */ > + unsigned long pfn_limit; > + /* 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. > @@ -105,12 +132,45 @@ 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; > > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns); > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP)) { > + struct balloon_bmap_hdr *hdr = vb->bmap_hdr; > + unsigned long bmap_len; > + > + /* cmd and req_id are not used here, set them to 0 */ > + hdr->cmd = cpu_to_virtio16(vb->vdev, 0); > + hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT); > + hdr->reserved = cpu_to_virtio16(vb->vdev, 0); > + hdr->req_id = cpu_to_virtio64(vb->vdev, 0); no need to swap 0, just fill it in. in fact you allocated all 0s so no need to touch these fields at all. > + hdr->start_pfn = cpu_to_virtio64(vb->vdev, vb->start_pfn); > + bmap_len = min(vb->bmap_len, > + (vb->end_pfn - vb->start_pfn) / BITS_PER_BYTE); > + hdr->bmap_len = cpu_to_virtio64(vb->vdev, bmap_len); > + sg_init_one(&sg, hdr, > + sizeof(struct balloon_bmap_hdr) + bmap_len); > + } 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); > @@ -118,7 +178,6 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq) > > /* When host has read buffer, this completes via balloon_ack */ > wait_event(vb->acked, virtqueue_get_buf(vq, &len)); > - > } > > static void set_page_pfns(struct virtio_balloon *vb, > @@ -133,13 +192,53 @@ 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 set_page_bitmap(struct virtio_balloon *vb, > + struct list_head *pages, struct virtqueue *vq) > +{ > + unsigned long pfn; > + struct page *page; > + bool found; > + > + vb->min_pfn = rounddown(vb->min_pfn, BITS_PER_LONG); > + vb->max_pfn = roundup(vb->max_pfn, BITS_PER_LONG); > + for (pfn = vb->min_pfn; pfn < vb->max_pfn; > + pfn += vb->pfn_limit) { > + vb->start_pfn = pfn + vb->pfn_limit; > + vb->end_pfn = pfn; > + memset(vb->page_bitmap, 0, vb->bmap_len); > + found = false; > + list_for_each_entry(page, pages, lru) { > + unsigned long balloon_pfn = page_to_balloon_pfn(page); > + > + if (balloon_pfn < pfn || > + balloon_pfn >= pfn + vb->pfn_limit) > + continue; > + set_bit(balloon_pfn - pfn, vb->page_bitmap); > + if (balloon_pfn > vb->end_pfn) > + vb->end_pfn = balloon_pfn; > + if (balloon_pfn < vb->start_pfn) > + vb->start_pfn = balloon_pfn; > + found = true; > + } > + if (found) { > + vb->start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG); > + vb->end_pfn = roundup(vb->end_pfn, BITS_PER_LONG); > + 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; > @@ -154,7 +253,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)) > @@ -163,8 +265,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; > @@ -184,15 +291,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); > for (vb->num_pfns = 0; vb->num_pfns < num; > @@ -200,7 +311,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; > } > @@ -211,9 +325,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); > - release_pages_balloon(vb, &pages); > + 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); > + } > mutex_unlock(&vb->balloon_lock); > return num_freed_pages; > } > @@ -347,13 +466,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; > > @@ -373,15 +494,17 @@ 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); > + use_bmap = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP); > diff = towards_target(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) > @@ -489,7 +612,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, > static int virtballoon_probe(struct virtio_device *vdev) > { > struct virtio_balloon *vb; > - int err; > + int err, hdr_len; > > if (!vdev->config->get) { > dev_err(&vdev->dev, "%s failure: config access disabled\n", > @@ -508,6 +631,18 @@ static int virtballoon_probe(struct virtio_device *vdev) > spin_lock_init(&vb->stop_update_lock); > vb->stop_update = false; > vb->num_pages = 0; > + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; > + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); > + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / > + BITS_PER_BYTE + 2 * sizeof(unsigned long); What are these 2 longs in aid of? > + hdr_len = sizeof(struct balloon_bmap_hdr); > + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); So it can go up to 1MByte but adding header size etc you need a higher order allocation. This is a waste, there is no need to have a power of two allocation. Start from the other side. Say "I want to allocate 32KBytes for the bitmap". Subtract the header and you get bitmap size. Calculate the pfn limit from there. > + > + /* 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 = vb->bmap_hdr + hdr_len; > mutex_init(&vb->balloon_lock); > init_waitqueue_head(&vb->acked); > vb->vdev = vdev; > @@ -541,9 +676,12 @@ out: > > 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. */ > @@ -565,6 +703,7 @@ static void virtballoon_remove(struct virtio_device *vdev) > cancel_work_sync(&vb->update_balloon_stats_work); > > remove_common(vb); > + kfree(vb->page_bitmap); > kfree(vb); > } > > @@ -603,6 +742,7 @@ static unsigned int features[] = { > 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.9.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 27, 2016 at 09:03:21AM -0700, Dave Hansen wrote: > On 07/26/2016 06:23 PM, Liang Li wrote: > > + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; > > + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); > > + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / > > + BITS_PER_BYTE + 2 * sizeof(unsigned long); > > + hdr_len = sizeof(struct balloon_bmap_hdr); > > + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); > > This ends up doing a 1MB kmalloc() right? That seems a _bit_ big. How > big was the pfn buffer before? Yes I would limit this to 1G memory in a go, will result in a 32KByte bitmap.
On Wed, Jul 27, 2016 at 09:23:33AM +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: Andrew Morton <akpm@linux-foundation.org> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Mel Gorman <mgorman@techsingularity.net> > 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 | 184 +++++++++++++++++++++++++++++++++++----- > 1 file changed, 162 insertions(+), 22 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 8d649a2..2d18ff6 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -41,10 +41,28 @@ > #define OOM_VBALLOON_DEFAULT_PAGES 256 > #define VIRTBALLOON_OOM_NOTIFY_PRIORITY 80 > > +/* > + * VIRTIO_BALLOON_PFNS_LIMIT is used to limit the size of page bitmap > + * to prevent a very large page bitmap, there are two reasons for this: > + * 1) to save memory. > + * 2) allocate a large bitmap may fail. > + * > + * The actual limit of pfn is determined by: > + * pfn_limit = min(max_pfn, VIRTIO_BALLOON_PFNS_LIMIT); > + * > + * If system has more pages than VIRTIO_BALLOON_PFNS_LIMIT, we will scan > + * the page list and send the PFNs with several times. To reduce the > + * overhead of scanning the page list. VIRTIO_BALLOON_PFNS_LIMIT should > + * be set with a value which can cover most cases. > + */ > +#define VIRTIO_BALLOON_PFNS_LIMIT ((32 * (1ULL << 30)) >> PAGE_SHIFT) /* 32GB */ > + > 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); > + Please just include the correct header. No need for this hackery. > struct virtio_balloon { > struct virtio_device *vdev; > struct virtqueue *inflate_vq, *deflate_vq, *stats_vq; > @@ -62,6 +80,15 @@ 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 length used to tell the host the pages */ > + unsigned long *page_bitmap; > + unsigned long bmap_len; > + /* Pfn limit */ > + unsigned long pfn_limit; > + /* 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. > @@ -105,12 +132,45 @@ 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; > > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns); > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP)) { > + struct balloon_bmap_hdr *hdr = vb->bmap_hdr; > + unsigned long bmap_len; > + > + /* cmd and req_id are not used here, set them to 0 */ > + hdr->cmd = cpu_to_virtio16(vb->vdev, 0); > + hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT); > + hdr->reserved = cpu_to_virtio16(vb->vdev, 0); > + hdr->req_id = cpu_to_virtio64(vb->vdev, 0); > + hdr->start_pfn = cpu_to_virtio64(vb->vdev, vb->start_pfn); > + bmap_len = min(vb->bmap_len, > + (vb->end_pfn - vb->start_pfn) / BITS_PER_BYTE); > + hdr->bmap_len = cpu_to_virtio64(vb->vdev, bmap_len); > + sg_init_one(&sg, hdr, > + sizeof(struct balloon_bmap_hdr) + bmap_len); > + } 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); > @@ -118,7 +178,6 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq) > > /* When host has read buffer, this completes via balloon_ack */ > wait_event(vb->acked, virtqueue_get_buf(vq, &len)); > - > } > > static void set_page_pfns(struct virtio_balloon *vb, > @@ -133,13 +192,53 @@ 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 set_page_bitmap(struct virtio_balloon *vb, > + struct list_head *pages, struct virtqueue *vq) > +{ > + unsigned long pfn; > + struct page *page; > + bool found; > + > + vb->min_pfn = rounddown(vb->min_pfn, BITS_PER_LONG); > + vb->max_pfn = roundup(vb->max_pfn, BITS_PER_LONG); > + for (pfn = vb->min_pfn; pfn < vb->max_pfn; > + pfn += vb->pfn_limit) { > + vb->start_pfn = pfn + vb->pfn_limit; > + vb->end_pfn = pfn; > + memset(vb->page_bitmap, 0, vb->bmap_len); > + found = false; > + list_for_each_entry(page, pages, lru) { > + unsigned long balloon_pfn = page_to_balloon_pfn(page); > + > + if (balloon_pfn < pfn || > + balloon_pfn >= pfn + vb->pfn_limit) > + continue; > + set_bit(balloon_pfn - pfn, vb->page_bitmap); > + if (balloon_pfn > vb->end_pfn) > + vb->end_pfn = balloon_pfn; > + if (balloon_pfn < vb->start_pfn) > + vb->start_pfn = balloon_pfn; > + found = true; > + } > + if (found) { > + vb->start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG); > + vb->end_pfn = roundup(vb->end_pfn, BITS_PER_LONG); > + 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; > @@ -154,7 +253,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)) > @@ -163,8 +265,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; > @@ -184,15 +291,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); > for (vb->num_pfns = 0; vb->num_pfns < num; > @@ -200,7 +311,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; > } > @@ -211,9 +325,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); > - release_pages_balloon(vb, &pages); > + 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); > + } > mutex_unlock(&vb->balloon_lock); > return num_freed_pages; > } > @@ -347,13 +466,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; > > @@ -373,15 +494,17 @@ 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); > + use_bmap = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP); > diff = towards_target(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) > @@ -489,7 +612,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, > static int virtballoon_probe(struct virtio_device *vdev) > { > struct virtio_balloon *vb; > - int err; > + int err, hdr_len; > > if (!vdev->config->get) { > dev_err(&vdev->dev, "%s failure: config access disabled\n", > @@ -508,6 +631,18 @@ static int virtballoon_probe(struct virtio_device *vdev) > spin_lock_init(&vb->stop_update_lock); > vb->stop_update = false; > vb->num_pages = 0; > + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; > + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); > + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / > + BITS_PER_BYTE + 2 * sizeof(unsigned long); > + hdr_len = sizeof(struct balloon_bmap_hdr); > + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, 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 = vb->bmap_hdr + hdr_len; > mutex_init(&vb->balloon_lock); > init_waitqueue_head(&vb->acked); > vb->vdev = vdev; > @@ -541,9 +676,12 @@ out: > > 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. */ > @@ -565,6 +703,7 @@ static void virtballoon_remove(struct virtio_device *vdev) > cancel_work_sync(&vb->update_balloon_stats_work); > > remove_common(vb); > + kfree(vb->page_bitmap); > kfree(vb); > } > > @@ -603,6 +742,7 @@ static unsigned int features[] = { > 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.9.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Subject: Re: [PATCH v2 repost 4/7] virtio-balloon: speed up inflate/deflate > process > > On 07/26/2016 06:23 PM, Liang Li wrote: > > + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; > > + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); > > + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / > > + BITS_PER_BYTE + 2 * sizeof(unsigned long); > > + hdr_len = sizeof(struct balloon_bmap_hdr); > > + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); > > This ends up doing a 1MB kmalloc() right? That seems a _bit_ big. How big > was the pfn buffer before? Yes, it is if the max pfn is more than 32GB. The size of the pfn buffer use before is 256*4 = 1024 Bytes, it's too small, and it's the main reason for bad performance. Use the max 1MB kmalloc is a balance between performance and flexibility, a large page bitmap covers the range of all the memory is no good for a system with huge amount of memory. If the bitmap is too small, it means we have to traverse a long list for many times, and it's bad for performance. Thanks! Liang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 28, 2016 at 01:13:35AM +0000, Li, Liang Z wrote: > > Subject: Re: [PATCH v2 repost 4/7] virtio-balloon: speed up inflate/deflate > > process > > > > On 07/26/2016 06:23 PM, Liang Li wrote: > > > + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; > > > + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); > > > + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / > > > + BITS_PER_BYTE + 2 * sizeof(unsigned long); > > > + hdr_len = sizeof(struct balloon_bmap_hdr); > > > + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); > > > > This ends up doing a 1MB kmalloc() right? That seems a _bit_ big. How big > > was the pfn buffer before? > > Yes, it is if the max pfn is more than 32GB. > The size of the pfn buffer use before is 256*4 = 1024 Bytes, it's too small, > and it's the main reason for bad performance. > Use the max 1MB kmalloc is a balance between performance and flexibility, > a large page bitmap covers the range of all the memory is no good for a system > with huge amount of memory. If the bitmap is too small, it means we have > to traverse a long list for many times, and it's bad for performance. > > Thanks! > Liang There are all your implementation decisions though. If guest memory is so fragmented that you only have order 0 4k pages, then allocating a huge 1M contigious chunk is very problematic in and of itself. Most people rarely migrate and do not care how fast that happens. Wasting a large chunk of memory (and it's zeroed for no good reason, so you actually request host memory for it) for everyone to speed it up when it does happen is not really an option.
> > + * VIRTIO_BALLOON_PFNS_LIMIT is used to limit the size of page bitmap > > + * to prevent a very large page bitmap, there are two reasons for this: > > + * 1) to save memory. > > + * 2) allocate a large bitmap may fail. > > + * > > + * The actual limit of pfn is determined by: > > + * pfn_limit = min(max_pfn, VIRTIO_BALLOON_PFNS_LIMIT); > > + * > > + * If system has more pages than VIRTIO_BALLOON_PFNS_LIMIT, we will > > +scan > > + * the page list and send the PFNs with several times. To reduce the > > + * overhead of scanning the page list. VIRTIO_BALLOON_PFNS_LIMIT > > +should > > + * be set with a value which can cover most cases. > > So what if it covers 1/32 of the memory? We'll do 32 exits and not 1, still not a > big deal for a big guest. > The issue here is the overhead is too high for scanning the page list for 32 times. Limit the page bitmap size to a fixed value is better for a big guest? > > + */ > > +#define VIRTIO_BALLOON_PFNS_LIMIT ((32 * (1ULL << 30)) >> > PAGE_SHIFT) > > +/* 32GB */ > > I already said this with a smaller limit. > > 2<< 30 is 2G but that is not a useful comment. > pls explain what is the reason for this selection. > > Still applies here. > I will add the comment for this. > > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns); > > + if (virtio_has_feature(vb->vdev, > VIRTIO_BALLOON_F_PAGE_BITMAP)) { > > + struct balloon_bmap_hdr *hdr = vb->bmap_hdr; > > + unsigned long bmap_len; > > + > > + /* cmd and req_id are not used here, set them to 0 */ > > + hdr->cmd = cpu_to_virtio16(vb->vdev, 0); > > + hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT); > > + hdr->reserved = cpu_to_virtio16(vb->vdev, 0); > > + hdr->req_id = cpu_to_virtio64(vb->vdev, 0); > > no need to swap 0, just fill it in. in fact you allocated all 0s so no need to touch > these fields at all. > Will change in v3. > > @@ -489,7 +612,7 @@ static int virtballoon_migratepage(struct > > balloon_dev_info *vb_dev_info, static int virtballoon_probe(struct > > virtio_device *vdev) { > > struct virtio_balloon *vb; > > - int err; > > + int err, hdr_len; > > > > if (!vdev->config->get) { > > dev_err(&vdev->dev, "%s failure: config access disabled\n", > @@ > > -508,6 +631,18 @@ static int virtballoon_probe(struct virtio_device *vdev) > > spin_lock_init(&vb->stop_update_lock); > > vb->stop_update = false; > > vb->num_pages = 0; > > + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; > > + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); > > + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / > > + BITS_PER_BYTE + 2 * sizeof(unsigned long); > > What are these 2 longs in aid of? > The rounddown(vb->start_pfn, BITS_PER_LONG) and roundup(vb->end_pfn, BITS_PER_LONG) may cause (vb->end_pfn - vb->start_pfn) > vb->pfn_limit, so we need extra space to save the bitmap for this case. 2 longs are enough. > > + hdr_len = sizeof(struct balloon_bmap_hdr); > > + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); > > So it can go up to 1MByte but adding header size etc you need a higher order > allocation. This is a waste, there is no need to have a power of two allocation. > Start from the other side. Say "I want to allocate 32KBytes for the bitmap". > Subtract the header and you get bitmap size. > Calculate the pfn limit from there. > Indeed, will change. Thanks a lot! Liang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Subject: Re: [PATCH v2 repost 4/7] virtio-balloon: speed up inflate/deflate > process > > On Wed, Jul 27, 2016 at 09:03:21AM -0700, Dave Hansen wrote: > > On 07/26/2016 06:23 PM, Liang Li wrote: > > > + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; > > > + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); > > > + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / > > > + BITS_PER_BYTE + 2 * sizeof(unsigned long); > > > + hdr_len = sizeof(struct balloon_bmap_hdr); > > > + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); > > > > This ends up doing a 1MB kmalloc() right? That seems a _bit_ big. > > How big was the pfn buffer before? > > > Yes I would limit this to 1G memory in a go, will result in a 32KByte bitmap. > > -- > MST Limit to 1G is bad for the performance, I sent you the test result several weeks ago. Paste it bellow: ------------------------------------------------------------------------------------------------------------------------ About the size of page bitmap, I have test the performance of filling the balloon to 15GB with a 16GB RAM VM. =============================== 32K Byte (cover 1GB of RAM) Time spends on inflating: 2031ms --------------------------------------------- 64K Byte (cover 2GB of RAM) Time spends on inflating: 1507ms -------------------------------------------- 512K Byte (cover 16GB of RAM) Time spends on inflating: 1237ms ================================ If possible, a big bitmap is better for performance. Liang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > +/* > > + * VIRTIO_BALLOON_PFNS_LIMIT is used to limit the size of page bitmap > > + * to prevent a very large page bitmap, there are two reasons for this: > > + * 1) to save memory. > > + * 2) allocate a large bitmap may fail. > > + * > > + * The actual limit of pfn is determined by: > > + * pfn_limit = min(max_pfn, VIRTIO_BALLOON_PFNS_LIMIT); > > + * > > + * If system has more pages than VIRTIO_BALLOON_PFNS_LIMIT, we will > > +scan > > + * the page list and send the PFNs with several times. To reduce the > > + * overhead of scanning the page list. VIRTIO_BALLOON_PFNS_LIMIT > > +should > > + * be set with a value which can cover most cases. > > + */ > > +#define VIRTIO_BALLOON_PFNS_LIMIT ((32 * (1ULL << 30)) >> > PAGE_SHIFT) > > +/* 32GB */ > > + > > 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); > > + > > Please just include the correct header. No need for this hackery. > Will change. Thanks! Liang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > This ends up doing a 1MB kmalloc() right? That seems a _bit_ big. > > > How big was the pfn buffer before? > > > > Yes, it is if the max pfn is more than 32GB. > > The size of the pfn buffer use before is 256*4 = 1024 Bytes, it's too > > small, and it's the main reason for bad performance. > > Use the max 1MB kmalloc is a balance between performance and > > flexibility, a large page bitmap covers the range of all the memory is > > no good for a system with huge amount of memory. If the bitmap is too > > small, it means we have to traverse a long list for many times, and it's bad > for performance. > > > > Thanks! > > Liang > > There are all your implementation decisions though. > > If guest memory is so fragmented that you only have order 0 4k pages, then > allocating a huge 1M contigious chunk is very problematic in and of itself. > The memory is allocated in the probe stage. This will not happen if the driver is loaded when booting the guest. > Most people rarely migrate and do not care how fast that happens. > Wasting a large chunk of memory (and it's zeroed for no good reason, so you > actually request host memory for it) for everyone to speed it up when it > does happen is not really an option. > If people don't plan to do inflating/deflating, they should not enable the virtio-balloon at the beginning, once they decide to use it, the driver should provide better performance as much as possible. 1MB is a very small portion for a VM with more than 32GB memory and it's the *worst case*, for VM with less than 32GB memory, the amount of RAM depends on VM's memory size and will be less than 1MB. If 1MB is too big, how about 512K, or 256K? 32K seems too small. Liang > -- > MST > > --------------------------------------------------------------------- > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 28, 2016 at 06:36:18AM +0000, Li, Liang Z wrote: > > > > This ends up doing a 1MB kmalloc() right? That seems a _bit_ big. > > > > How big was the pfn buffer before? > > > > > > Yes, it is if the max pfn is more than 32GB. > > > The size of the pfn buffer use before is 256*4 = 1024 Bytes, it's too > > > small, and it's the main reason for bad performance. > > > Use the max 1MB kmalloc is a balance between performance and > > > flexibility, a large page bitmap covers the range of all the memory is > > > no good for a system with huge amount of memory. If the bitmap is too > > > small, it means we have to traverse a long list for many times, and it's bad > > for performance. > > > > > > Thanks! > > > Liang > > > > There are all your implementation decisions though. > > > > If guest memory is so fragmented that you only have order 0 4k pages, then > > allocating a huge 1M contigious chunk is very problematic in and of itself. > > > > The memory is allocated in the probe stage. This will not happen if the driver is > loaded when booting the guest. > > > Most people rarely migrate and do not care how fast that happens. > > Wasting a large chunk of memory (and it's zeroed for no good reason, so you > > actually request host memory for it) for everyone to speed it up when it > > does happen is not really an option. > > > If people don't plan to do inflating/deflating, they should not enable the virtio-balloon > at the beginning, once they decide to use it, the driver should provide better performance > as much as possible. The reason people inflate/deflate is so they can overcommit memory. Do they need to overcommit very quickly? I don't see why. So let's get what we can for free but I don't really believe people would want to pay for it. > 1MB is a very small portion for a VM with more than 32GB memory and it's the *worst case*, > for VM with less than 32GB memory, the amount of RAM depends on VM's memory size > and will be less than 1MB. It's guest memmory so might all be in swap and never touched, your memset at probe time will fault it in and make hypervisor actually pay for it. > If 1MB is too big, how about 512K, or 256K? 32K seems too small. > > Liang It's only small because it makes you rescan the free list. So maybe you should do something else. I looked at it a bit. Instead of scanning the free list, how about scanning actual page structures? If page is unused, pass it to host. Solves the problem of rescanning multiple times, does it not? Another idea: allocate a small bitmap at probe time (e.g. for deflate), allocate a bunch more on each request. Use something like GFP_ATOMIC and a scatter/gather, if that fails use the smaller bitmap. > > -- > > MST > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 28, 2016 at 03:30:09AM +0000, Li, Liang Z wrote: > > Subject: Re: [PATCH v2 repost 4/7] virtio-balloon: speed up inflate/deflate > > process > > > > On Wed, Jul 27, 2016 at 09:03:21AM -0700, Dave Hansen wrote: > > > On 07/26/2016 06:23 PM, Liang Li wrote: > > > > + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; > > > > + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); > > > > + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / > > > > + BITS_PER_BYTE + 2 * sizeof(unsigned long); > > > > + hdr_len = sizeof(struct balloon_bmap_hdr); > > > > + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); > > > > > > This ends up doing a 1MB kmalloc() right? That seems a _bit_ big. > > > How big was the pfn buffer before? > > > > > > Yes I would limit this to 1G memory in a go, will result in a 32KByte bitmap. > > > > -- > > MST > > Limit to 1G is bad for the performance, I sent you the test result several weeks ago. > > Paste it bellow: > ------------------------------------------------------------------------------------------------------------------------ > About the size of page bitmap, I have test the performance of filling the balloon to 15GB with a > 16GB RAM VM. > > =============================== > 32K Byte (cover 1GB of RAM) > > Time spends on inflating: 2031ms > --------------------------------------------- > 64K Byte (cover 2GB of RAM) > > Time spends on inflating: 1507ms > -------------------------------------------- > 512K Byte (cover 16GB of RAM) > > Time spends on inflating: 1237ms > ================================ > > If possible, a big bitmap is better for performance. > > Liang Earlier you said: a. allocating pages (6.5%) b. sending PFNs to host (68.3%) c. address translation (6.1%) d. madvise (19%) Here sending PFNs to host with 512K Byte map should be almost free. So is something else taking up the time?
On Thu, Jul 28, 2016 at 03:06:37AM +0000, Li, Liang Z wrote: > > > + * VIRTIO_BALLOON_PFNS_LIMIT is used to limit the size of page bitmap > > > + * to prevent a very large page bitmap, there are two reasons for this: > > > + * 1) to save memory. > > > + * 2) allocate a large bitmap may fail. > > > + * > > > + * The actual limit of pfn is determined by: > > > + * pfn_limit = min(max_pfn, VIRTIO_BALLOON_PFNS_LIMIT); > > > + * > > > + * If system has more pages than VIRTIO_BALLOON_PFNS_LIMIT, we will > > > +scan > > > + * the page list and send the PFNs with several times. To reduce the > > > + * overhead of scanning the page list. VIRTIO_BALLOON_PFNS_LIMIT > > > +should > > > + * be set with a value which can cover most cases. > > > > So what if it covers 1/32 of the memory? We'll do 32 exits and not 1, still not a > > big deal for a big guest. > > > > The issue here is the overhead is too high for scanning the page list for 32 times. > Limit the page bitmap size to a fixed value is better for a big guest? > I'd say avoid scanning free lists completely. Scan pages themselves and check the refcount to see whether they are free. This way each page needs to be tested once. And skip the whole optimization if less than e.g. 10% is free. > > > + */ > > > +#define VIRTIO_BALLOON_PFNS_LIMIT ((32 * (1ULL << 30)) >> > > PAGE_SHIFT) > > > +/* 32GB */ > > > > I already said this with a smaller limit. > > > > 2<< 30 is 2G but that is not a useful comment. > > pls explain what is the reason for this selection. > > > > Still applies here. > > > > I will add the comment for this. > > > > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns); > > > + if (virtio_has_feature(vb->vdev, > > VIRTIO_BALLOON_F_PAGE_BITMAP)) { > > > + struct balloon_bmap_hdr *hdr = vb->bmap_hdr; > > > + unsigned long bmap_len; > > > + > > > + /* cmd and req_id are not used here, set them to 0 */ > > > + hdr->cmd = cpu_to_virtio16(vb->vdev, 0); > > > + hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT); > > > + hdr->reserved = cpu_to_virtio16(vb->vdev, 0); > > > + hdr->req_id = cpu_to_virtio64(vb->vdev, 0); > > > > no need to swap 0, just fill it in. in fact you allocated all 0s so no need to touch > > these fields at all. > > > > Will change in v3. > > > > @@ -489,7 +612,7 @@ static int virtballoon_migratepage(struct > > > balloon_dev_info *vb_dev_info, static int virtballoon_probe(struct > > > virtio_device *vdev) { > > > struct virtio_balloon *vb; > > > - int err; > > > + int err, hdr_len; > > > > > > if (!vdev->config->get) { > > > dev_err(&vdev->dev, "%s failure: config access disabled\n", > > @@ > > > -508,6 +631,18 @@ static int virtballoon_probe(struct virtio_device *vdev) > > > spin_lock_init(&vb->stop_update_lock); > > > vb->stop_update = false; > > > vb->num_pages = 0; > > > + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; > > > + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); > > > + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / > > > + BITS_PER_BYTE + 2 * sizeof(unsigned long); > > > > What are these 2 longs in aid of? > > > The rounddown(vb->start_pfn, BITS_PER_LONG) and roundup(vb->end_pfn, BITS_PER_LONG) > may cause (vb->end_pfn - vb->start_pfn) > vb->pfn_limit, so we need extra space to save the > bitmap for this case. 2 longs are enough. > > > > + hdr_len = sizeof(struct balloon_bmap_hdr); > > > + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); > > > > So it can go up to 1MByte but adding header size etc you need a higher order > > allocation. This is a waste, there is no need to have a power of two allocation. > > Start from the other side. Say "I want to allocate 32KBytes for the bitmap". > > Subtract the header and you get bitmap size. > > Calculate the pfn limit from there. > > > > Indeed, will change. Thanks a lot! > > Liang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Thu, Jul 28, 2016 at 03:06:37AM +0000, Li, Liang Z wrote: > > > > + * VIRTIO_BALLOON_PFNS_LIMIT is used to limit the size of page > > > > +bitmap > > > > + * to prevent a very large page bitmap, there are two reasons for this: > > > > + * 1) to save memory. > > > > + * 2) allocate a large bitmap may fail. > > > > + * > > > > + * The actual limit of pfn is determined by: > > > > + * pfn_limit = min(max_pfn, VIRTIO_BALLOON_PFNS_LIMIT); > > > > + * > > > > + * If system has more pages than VIRTIO_BALLOON_PFNS_LIMIT, we > > > > +will scan > > > > + * the page list and send the PFNs with several times. To reduce > > > > +the > > > > + * overhead of scanning the page list. VIRTIO_BALLOON_PFNS_LIMIT > > > > +should > > > > + * be set with a value which can cover most cases. > > > > > > So what if it covers 1/32 of the memory? We'll do 32 exits and not > > > 1, still not a big deal for a big guest. > > > > > > > The issue here is the overhead is too high for scanning the page list for 32 > times. > > Limit the page bitmap size to a fixed value is better for a big guest? > > > > I'd say avoid scanning free lists completely. Scan pages themselves and check > the refcount to see whether they are free. > This way each page needs to be tested once. > > And skip the whole optimization if less than e.g. 10% is free. That's better than rescanning the free list. Will change in next version. Thanks! Liang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Thu, Jul 28, 2016 at 06:36:18AM +0000, Li, Liang Z wrote: > > > > > This ends up doing a 1MB kmalloc() right? That seems a _bit_ big. > > > > > How big was the pfn buffer before? > > > > > > > > Yes, it is if the max pfn is more than 32GB. > > > > The size of the pfn buffer use before is 256*4 = 1024 Bytes, it's > > > > too small, and it's the main reason for bad performance. > > > > Use the max 1MB kmalloc is a balance between performance and > > > > flexibility, a large page bitmap covers the range of all the > > > > memory is no good for a system with huge amount of memory. If the > > > > bitmap is too small, it means we have to traverse a long list for > > > > many times, and it's bad > > > for performance. > > > > > > > > Thanks! > > > > Liang > > > > > > There are all your implementation decisions though. > > > > > > If guest memory is so fragmented that you only have order 0 4k > > > pages, then allocating a huge 1M contigious chunk is very problematic in > and of itself. > > > > > > > The memory is allocated in the probe stage. This will not happen if > > the driver is loaded when booting the guest. > > > > > Most people rarely migrate and do not care how fast that happens. > > > Wasting a large chunk of memory (and it's zeroed for no good reason, > > > so you actually request host memory for it) for everyone to speed it > > > up when it does happen is not really an option. > > > > > If people don't plan to do inflating/deflating, they should not enable > > the virtio-balloon at the beginning, once they decide to use it, the > > driver should provide better performance as much as possible. > > The reason people inflate/deflate is so they can overcommit memory. > Do they need to overcommit very quickly? I don't see why. > So let's get what we can for free but I don't really believe people would want > to pay for it. > > > 1MB is a very small portion for a VM with more than 32GB memory and > > it's the *worst case*, for VM with less than 32GB memory, the amount > > of RAM depends on VM's memory size and will be less than 1MB. > > It's guest memmory so might all be in swap and never touched, your memset > at probe time will fault it in and make hypervisor actually pay for it. > > > If 1MB is too big, how about 512K, or 256K? 32K seems too small. > > > > Liang > > It's only small because it makes you rescan the free list. > So maybe you should do something else. > I looked at it a bit. Instead of scanning the free list, how about scanning actual > page structures? If page is unused, pass it to host. > Solves the problem of rescanning multiple times, does it not? > Yes, agree. > > Another idea: allocate a small bitmap at probe time (e.g. for deflate), allocate > a bunch more on each request. Use something like GFP_ATOMIC and a > scatter/gather, if that fails use the smaller bitmap. > So, the aim of v3 is to use a smaller bitmap without too heavy performance penalty. Thanks a lot! Liang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > On Wed, Jul 27, 2016 at 09:03:21AM -0700, Dave Hansen wrote: > > > > On 07/26/2016 06:23 PM, Liang Li wrote: > > > > > + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; > > > > > + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); > > > > > + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / > > > > > + BITS_PER_BYTE + 2 * sizeof(unsigned long); > > > > > + hdr_len = sizeof(struct balloon_bmap_hdr); > > > > > + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, > GFP_KERNEL); > > > > > > > > This ends up doing a 1MB kmalloc() right? That seems a _bit_ big. > > > > How big was the pfn buffer before? > > > > > > > > > Yes I would limit this to 1G memory in a go, will result in a 32KByte bitmap. > > > > > > -- > > > MST > > > > Limit to 1G is bad for the performance, I sent you the test result several > weeks ago. > > > > Paste it bellow: > > ---------------------------------------------------------------------- > > -------------------------------------------------- > > About the size of page bitmap, I have test the performance of filling > > the balloon to 15GB with a 16GB RAM VM. > > > > =============================== > > 32K Byte (cover 1GB of RAM) > > > > Time spends on inflating: 2031ms > > --------------------------------------------- > > 64K Byte (cover 2GB of RAM) > > > > Time spends on inflating: 1507ms > > -------------------------------------------- > > 512K Byte (cover 16GB of RAM) > > > > Time spends on inflating: 1237ms > > ================================ > > > > If possible, a big bitmap is better for performance. > > > > Liang > > Earlier you said: > a. allocating pages (6.5%) > b. sending PFNs to host (68.3%) > c. address translation (6.1%) > d. madvise (19%) > > Here sending PFNs to host with 512K Byte map should be almost free. > > So is something else taking up the time? > I just want to show you the benefits of using a big bitmap. :) I did not measure the time spend on each stage after optimization(I will do it later), but I have tried to allocate the page with big chunk and found it can make things faster. Without allocating big chunk page, the performance improvement is about 85%, and with allocating big chunk page, the improvement is about 94%. Liang > > -- > MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/28/2016 02:51 PM, Michael S. Tsirkin wrote: >> > If 1MB is too big, how about 512K, or 256K? 32K seems too small. >> > > It's only small because it makes you rescan the free list. > So maybe you should do something else. > I looked at it a bit. Instead of scanning the free list, how about > scanning actual page structures? If page is unused, pass it to host. > Solves the problem of rescanning multiple times, does it not? FWIW, I think the new data structure needs some work. Before, we had a potentially very long list of 4k areas. Now, we've just got a very large bitmap. The bitmap might not even be very dense if we are ballooning relatively few things. Can I suggest an alternate scheme? I think you actually need a hybrid scheme that has bitmaps but also allows more flexibility in the pfn ranges. The payload could be a number of records each containing 3 things: pfn, page order, length of bitmap (maybe in powers of 2) Each record is followed by the bitmap. Or, if the bitmap length is 0, immediately followed by another record. A bitmap length of 0 implies a bitmap with the least significant bit set. Page order specifies how many pages each bit represents. This scheme could easily encode the new data structure you are proposing by just setting pfn=0, order=0, and a very long bitmap length. But, it could handle sparse bitmaps much better *and* represent large pages much more efficiently. There's plenty of space to fit a whole record in 64 bits. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > It's only small because it makes you rescan the free list. > > So maybe you should do something else. > > I looked at it a bit. Instead of scanning the free list, how about > > scanning actual page structures? If page is unused, pass it to host. > > Solves the problem of rescanning multiple times, does it not? > > FWIW, I think the new data structure needs some work. > > Before, we had a potentially very long list of 4k areas. Now, we've just got a > very large bitmap. The bitmap might not even be very dense if we are > ballooning relatively few things. > > Can I suggest an alternate scheme? I think you actually need a hybrid > scheme that has bitmaps but also allows more flexibility in the pfn ranges. > The payload could be a number of records each containing 3 things: > > pfn, page order, length of bitmap (maybe in powers of 2) > > Each record is followed by the bitmap. Or, if the bitmap length is 0, > immediately followed by another record. A bitmap length of 0 implies a > bitmap with the least significant bit set. Page order specifies how many > pages each bit represents. > > This scheme could easily encode the new data structure you are proposing > by just setting pfn=0, order=0, and a very long bitmap length. But, it could > handle sparse bitmaps much better *and* represent large pages much more > efficiently. > > There's plenty of space to fit a whole record in 64 bits. I like your idea and it's more flexible, and it's very useful if we want to optimize the page allocating stage further. I believe the memory fragmentation will not be very serious, so the performance won't be too bad in the worst case. Thanks! Liang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 8d649a2..2d18ff6 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -41,10 +41,28 @@ #define OOM_VBALLOON_DEFAULT_PAGES 256 #define VIRTBALLOON_OOM_NOTIFY_PRIORITY 80 +/* + * VIRTIO_BALLOON_PFNS_LIMIT is used to limit the size of page bitmap + * to prevent a very large page bitmap, there are two reasons for this: + * 1) to save memory. + * 2) allocate a large bitmap may fail. + * + * The actual limit of pfn is determined by: + * pfn_limit = min(max_pfn, VIRTIO_BALLOON_PFNS_LIMIT); + * + * If system has more pages than VIRTIO_BALLOON_PFNS_LIMIT, we will scan + * the page list and send the PFNs with several times. To reduce the + * overhead of scanning the page list. VIRTIO_BALLOON_PFNS_LIMIT should + * be set with a value which can cover most cases. + */ +#define VIRTIO_BALLOON_PFNS_LIMIT ((32 * (1ULL << 30)) >> PAGE_SHIFT) /* 32GB */ + 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 +80,15 @@ 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 length used to tell the host the pages */ + unsigned long *page_bitmap; + unsigned long bmap_len; + /* Pfn limit */ + unsigned long pfn_limit; + /* 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. @@ -105,12 +132,45 @@ 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; - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns); + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP)) { + struct balloon_bmap_hdr *hdr = vb->bmap_hdr; + unsigned long bmap_len; + + /* cmd and req_id are not used here, set them to 0 */ + hdr->cmd = cpu_to_virtio16(vb->vdev, 0); + hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT); + hdr->reserved = cpu_to_virtio16(vb->vdev, 0); + hdr->req_id = cpu_to_virtio64(vb->vdev, 0); + hdr->start_pfn = cpu_to_virtio64(vb->vdev, vb->start_pfn); + bmap_len = min(vb->bmap_len, + (vb->end_pfn - vb->start_pfn) / BITS_PER_BYTE); + hdr->bmap_len = cpu_to_virtio64(vb->vdev, bmap_len); + sg_init_one(&sg, hdr, + sizeof(struct balloon_bmap_hdr) + bmap_len); + } 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); @@ -118,7 +178,6 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq) /* When host has read buffer, this completes via balloon_ack */ wait_event(vb->acked, virtqueue_get_buf(vq, &len)); - } static void set_page_pfns(struct virtio_balloon *vb, @@ -133,13 +192,53 @@ 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 set_page_bitmap(struct virtio_balloon *vb, + struct list_head *pages, struct virtqueue *vq) +{ + unsigned long pfn; + struct page *page; + bool found; + + vb->min_pfn = rounddown(vb->min_pfn, BITS_PER_LONG); + vb->max_pfn = roundup(vb->max_pfn, BITS_PER_LONG); + for (pfn = vb->min_pfn; pfn < vb->max_pfn; + pfn += vb->pfn_limit) { + vb->start_pfn = pfn + vb->pfn_limit; + vb->end_pfn = pfn; + memset(vb->page_bitmap, 0, vb->bmap_len); + found = false; + list_for_each_entry(page, pages, lru) { + unsigned long balloon_pfn = page_to_balloon_pfn(page); + + if (balloon_pfn < pfn || + balloon_pfn >= pfn + vb->pfn_limit) + continue; + set_bit(balloon_pfn - pfn, vb->page_bitmap); + if (balloon_pfn > vb->end_pfn) + vb->end_pfn = balloon_pfn; + if (balloon_pfn < vb->start_pfn) + vb->start_pfn = balloon_pfn; + found = true; + } + if (found) { + vb->start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG); + vb->end_pfn = roundup(vb->end_pfn, BITS_PER_LONG); + 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; @@ -154,7 +253,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)) @@ -163,8 +265,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; @@ -184,15 +291,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); for (vb->num_pfns = 0; vb->num_pfns < num; @@ -200,7 +311,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; } @@ -211,9 +325,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); - release_pages_balloon(vb, &pages); + 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); + } mutex_unlock(&vb->balloon_lock); return num_freed_pages; } @@ -347,13 +466,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; @@ -373,15 +494,17 @@ 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); + use_bmap = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_PAGE_BITMAP); diff = towards_target(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) @@ -489,7 +612,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info, static int virtballoon_probe(struct virtio_device *vdev) { struct virtio_balloon *vb; - int err; + int err, hdr_len; if (!vdev->config->get) { dev_err(&vdev->dev, "%s failure: config access disabled\n", @@ -508,6 +631,18 @@ static int virtballoon_probe(struct virtio_device *vdev) spin_lock_init(&vb->stop_update_lock); vb->stop_update = false; vb->num_pages = 0; + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / + BITS_PER_BYTE + 2 * sizeof(unsigned long); + hdr_len = sizeof(struct balloon_bmap_hdr); + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, 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 = vb->bmap_hdr + hdr_len; mutex_init(&vb->balloon_lock); init_waitqueue_head(&vb->acked); vb->vdev = vdev; @@ -541,9 +676,12 @@ out: 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. */ @@ -565,6 +703,7 @@ static void virtballoon_remove(struct virtio_device *vdev) cancel_work_sync(&vb->update_balloon_stats_work); remove_common(vb); + kfree(vb->page_bitmap); kfree(vb); } @@ -603,6 +742,7 @@ static unsigned int features[] = { 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 = {
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: Andrew Morton <akpm@linux-foundation.org> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Mel Gorman <mgorman@techsingularity.net> 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 | 184 +++++++++++++++++++++++++++++++++++----- 1 file changed, 162 insertions(+), 22 deletions(-)