Re: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG

Re: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG

[Michael S. Tsirkin]

On Thu, Aug 03, 2017 at 02:38:17PM +0800, Wei Wang wrote:
> Add a new feature, VIRTIO_BALLOON_F_SG, which enables the transfer
> of balloon (i.e. inflated/deflated) pages using scatter-gather lists
> to the host.
> 
> The implementation of the previous virtio-balloon is not very
> efficient, because the balloon pages are transferred to the
> host one by one. Here is the breakdown of the time in percentage
> spent on each step of the balloon inflating process (inflating
> 7GB of an 8GB idle guest).
> 
> 1) allocating pages (6.5%)
> 2) sending PFNs to host (68.3%)
> 3) address translation (6.1%)
> 4) madvise (19%)
> 
> It takes about 4126ms for the inflating process to complete.
> The above profiling shows that the bottlenecks are stage 2)
> and stage 4).
> 
> This patch optimizes step 2) by transferring pages to the host in
> sgs. An sg describes a chunk of guest physically continuous pages.
> With this mechanism, step 4) can also be optimized by doing address
> translation and madvise() in chunks rather than page by page.
> 
> With this new feature, the above ballooning process takes ~541ms
> resulting in an improvement of ~87%.
> 
> TODO: optimize stage 1) by allocating/freeing a chunk of pages
> instead of a single page each time.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/virtio/virtio_balloon.c     | 150 ++++++++++++++++++++++++++++++++----
>  include/uapi/linux/virtio_balloon.h |   1 +
>  2 files changed, 134 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index f0b3a0b..29aca0c 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -32,6 +32,7 @@
>  #include <linux/mm.h>
>  #include <linux/mount.h>
>  #include <linux/magic.h>
> +#include <linux/xbitmap.h>
>  
>  /*
>   * Balloon device works in 4K page units.  So each page is pointed to by
> @@ -79,6 +80,9 @@ struct virtio_balloon {
>  	/* Synchronize access/update to this struct virtio_balloon elements */
>  	struct mutex balloon_lock;
>  
> +	/* The xbitmap used to record ballooned pages */
> +	struct xb page_xb;
> +
>  	/* The array of pfns we tell the Host about. */
>  	unsigned int num_pfns;
>  	__virtio32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
> @@ -141,13 +145,90 @@ static void set_page_pfns(struct virtio_balloon *vb,
>  					  page_to_balloon_pfn(page) + i);
>  }
>  
> +static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
> +			void *addr, uint32_t size)
> +{
> +	struct scatterlist sg;
> +	unsigned int len;
> +
> +	sg_init_one(&sg, addr, size);
> +	while (unlikely(virtqueue_add_inbuf(vq, &sg, 1, vb, GFP_KERNEL)
> +			== -ENOSPC)) {
> +		/*
> +		 * It is uncommon to see the vq is full, because the sg is sent
> +		 * one by one and the device is able to handle it in time. But
> +		 * if that happens, we kick and wait for an entry is released.

is released -> to get used.

> +		 */
> +		virtqueue_kick(vq);
> +		while (!virtqueue_get_buf(vq, &len) &&
> +		       !virtqueue_is_broken(vq))
> +			cpu_relax();

Please rework to use wait_event in that case too.


> +	}
> +	virtqueue_kick(vq);
> +	wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> +}
> +
> +/*
> + * Send balloon pages in sgs to host. The balloon pages are recorded in the
> + * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
> + * The page xbitmap is searched for continuous "1" bits, which correspond
> + * to continuous pages, to chunk into sgs.
> + *
> + * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
> + * need to be searched.
> + */
> +static void tell_host_sgs(struct virtio_balloon *vb,
> +			  struct virtqueue *vq,
> +			  unsigned long page_xb_start,
> +			  unsigned long page_xb_end)
> +{
> +	unsigned long sg_pfn_start, sg_pfn_end;
> +	void *sg_addr;
> +	uint32_t sg_len, sg_max_len = round_down(UINT_MAX, PAGE_SIZE);
> +
> +	sg_pfn_start = page_xb_start;
> +	while (sg_pfn_start < page_xb_end) {
> +		sg_pfn_start = xb_find_next_bit(&vb->page_xb, sg_pfn_start,
> +						page_xb_end, 1);
> +		if (sg_pfn_start == page_xb_end + 1)
> +			break;
> +		sg_pfn_end = xb_find_next_bit(&vb->page_xb, sg_pfn_start + 1,
> +					      page_xb_end, 0);
> +		sg_addr = pfn_to_kaddr(sg_pfn_start);
> +		sg_len = (sg_pfn_end - sg_pfn_start) << PAGE_SHIFT;
> +		while (sg_len > sg_max_len) {
> +			send_one_sg(vb, vq, sg_addr, sg_max_len);
> +			sg_addr += sg_max_len;
> +			sg_len -= sg_max_len;
> +		}
> +		send_one_sg(vb, vq, sg_addr, sg_len);
> +		xb_zero(&vb->page_xb, sg_pfn_start, sg_pfn_end);
> +		sg_pfn_start = sg_pfn_end + 1;
> +	}
> +}
> +
> +static inline void xb_set_page(struct virtio_balloon *vb,
> +			       struct page *page,
> +			       unsigned long *pfn_min,
> +			       unsigned long *pfn_max)
> +{
> +	unsigned long pfn = page_to_pfn(page);
> +
> +	*pfn_min = min(pfn, *pfn_min);
> +	*pfn_max = max(pfn, *pfn_max);
> +	xb_set_bit(&vb->page_xb, pfn);
> +}
> +
>  static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
>  {
>  	struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
>  	unsigned num_allocated_pages;
> +	bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> +	unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>  
>  	/* We can only do one array worth at a time. */
> -	num = min(num, ARRAY_SIZE(vb->pfns));
> +	if (!use_sg)
> +		num = min(num, ARRAY_SIZE(vb->pfns));
>  
>  	mutex_lock(&vb->balloon_lock);
>  	for (vb->num_pfns = 0; vb->num_pfns < num;
> @@ -162,7 +243,12 @@ 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_sg)
> +			xb_set_page(vb, page, &pfn_min, &pfn_max);
> +		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))
> @@ -171,8 +257,12 @@ 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) {
> +		if (use_sg)
> +			tell_host_sgs(vb, vb->inflate_vq, pfn_min, pfn_max);
> +		else
> +			tell_host(vb, vb->inflate_vq);
> +	}
>  	mutex_unlock(&vb->balloon_lock);
>  
>  	return num_allocated_pages;
> @@ -198,9 +288,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
>  	struct page *page;
>  	struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
>  	LIST_HEAD(pages);
> +	bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> +	unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>  
> -	/* We can only do one array worth at a time. */
> -	num = min(num, ARRAY_SIZE(vb->pfns));
> +	/* Traditionally, we can only do one array worth at a time. */
> +	if (!use_sg)
> +		num = min(num, ARRAY_SIZE(vb->pfns));
>  
>  	mutex_lock(&vb->balloon_lock);
>  	/* We can't release more pages than taken */
> @@ -210,7 +303,11 @@ 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_sg)
> +			xb_set_page(vb, page, &pfn_min, &pfn_max);
> +		else
> +			set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
>  		list_add(&page->lru, &pages);
>  		vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
>  	}
> @@ -221,8 +318,12 @@ 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) {
> +		if (use_sg)
> +			tell_host_sgs(vb, vb->deflate_vq, pfn_min, pfn_max);
> +		else
> +			tell_host(vb, vb->deflate_vq);
> +	}
>  	release_pages_balloon(vb, &pages);
>  	mutex_unlock(&vb->balloon_lock);
>  	return num_freed_pages;
> @@ -441,6 +542,7 @@ static int init_vqs(struct virtio_balloon *vb)
>  }
>  
>  #ifdef CONFIG_BALLOON_COMPACTION
> +
>  /*
>   * virtballoon_migratepage - perform the balloon page migration on behalf of
>   *			     a compation thread.     (called under page lock)
> @@ -464,6 +566,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
>  {
>  	struct virtio_balloon *vb = container_of(vb_dev_info,
>  			struct virtio_balloon, vb_dev_info);
> +	bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
>  	unsigned long flags;
>  
>  	/*
> @@ -485,16 +588,24 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
>  	vb_dev_info->isolated_pages--;
>  	__count_vm_event(BALLOON_MIGRATE);
>  	spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
> -	vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> -	set_page_pfns(vb, vb->pfns, newpage);
> -	tell_host(vb, vb->inflate_vq);
> -
> +	if (use_sg) {
> +		send_one_sg(vb, vb->inflate_vq, page_address(newpage),
> +			    PAGE_SIZE);
> +	} else {
> +		vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> +		set_page_pfns(vb, vb->pfns, newpage);
> +		tell_host(vb, vb->inflate_vq);
> +	}
>  	/* balloon's page migration 2nd step -- deflate "page" */
>  	balloon_page_delete(page);
> -	vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> -	set_page_pfns(vb, vb->pfns, page);
> -	tell_host(vb, vb->deflate_vq);
> -
> +	if (use_sg) {
> +		send_one_sg(vb, vb->deflate_vq, page_address(page),
> +			    PAGE_SIZE);
> +	} else {
> +		vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> +		set_page_pfns(vb, vb->pfns, page);
> +		tell_host(vb, vb->deflate_vq);
> +	}
>  	mutex_unlock(&vb->balloon_lock);
>  
>  	put_page(page); /* balloon reference */
> @@ -553,6 +664,9 @@ static int virtballoon_probe(struct virtio_device *vdev)
>  	if (err)
>  		goto out_free_vb;
>  
> +	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
> +		xb_init(&vb->page_xb);
> +
>  	vb->nb.notifier_call = virtballoon_oom_notify;
>  	vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
>  	err = register_oom_notifier(&vb->nb);
> @@ -618,6 +732,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
>  	cancel_work_sync(&vb->update_balloon_size_work);
>  	cancel_work_sync(&vb->update_balloon_stats_work);
>  
> +	xb_empty(&vb->page_xb);
>  	remove_common(vb);
>  #ifdef CONFIG_BALLOON_COMPACTION
>  	if (vb->vb_dev_info.inode)
> @@ -669,6 +784,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_SG,
>  };
>  
>  static struct virtio_driver virtio_balloon_driver = {
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 343d7dd..37780a7 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_SG		3 /* Use sg instead of PFN lists */
>  
>  /* Size of a PFN in the balloon interface. */
>  #define VIRTIO_BALLOON_PFN_SHIFT 12
> -- 
> 2.7.4

RE: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG

[Wang, Wei W]

On Thursday, August 3, 2017 10:23 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 03, 2017 at 02:38:17PM +0800, Wei Wang wrote:
> > +static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
> > +			void *addr, uint32_t size)
> > +{
> > +	struct scatterlist sg;
> > +	unsigned int len;
> > +
> > +	sg_init_one(&sg, addr, size);
> > +	while (unlikely(virtqueue_add_inbuf(vq, &sg, 1, vb, GFP_KERNEL)
> > +			== -ENOSPC)) {
> > +		/*
> > +		 * It is uncommon to see the vq is full, because the sg is sent
> > +		 * one by one and the device is able to handle it in time. But
> > +		 * if that happens, we kick and wait for an entry is released.
> 
> is released -> to get used.
> 
> > +		 */
> > +		virtqueue_kick(vq);
> > +		while (!virtqueue_get_buf(vq, &len) &&
> > +		       !virtqueue_is_broken(vq))
> > +			cpu_relax();
> 
> Please rework to use wait_event in that case too.

For the balloon page case here, it is fine to use wait_event. But for the free page
case, I think it might not be suitable because the mm lock is being held.

Best,
Wei

RE: [PATCH v13 4/5] mm: support reporting free page blocks

[Wang, Wei W]

On Thursday, August 3, 2017 9:51 PM, Michal Hocko: 
> As I've said earlier. Start simple optimize incrementally with some numbers to
> justify a more subtle code.
> --

OK. Let's start with the simple implementation as you suggested.

Best,
Wei

Re: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG

[Michael S. Tsirkin]

On Thu, Aug 03, 2017 at 03:17:59PM +0000, Wang, Wei W wrote:
> On Thursday, August 3, 2017 10:23 PM, Michael S. Tsirkin wrote:
> > On Thu, Aug 03, 2017 at 02:38:17PM +0800, Wei Wang wrote:
> > > +static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
> > > +			void *addr, uint32_t size)
> > > +{
> > > +	struct scatterlist sg;
> > > +	unsigned int len;
> > > +
> > > +	sg_init_one(&sg, addr, size);
> > > +	while (unlikely(virtqueue_add_inbuf(vq, &sg, 1, vb, GFP_KERNEL)
> > > +			== -ENOSPC)) {
> > > +		/*
> > > +		 * It is uncommon to see the vq is full, because the sg is sent
> > > +		 * one by one and the device is able to handle it in time. But
> > > +		 * if that happens, we kick and wait for an entry is released.
> > 
> > is released -> to get used.
> > 
> > > +		 */
> > > +		virtqueue_kick(vq);
> > > +		while (!virtqueue_get_buf(vq, &len) &&
> > > +		       !virtqueue_is_broken(vq))
> > > +			cpu_relax();
> > 
> > Please rework to use wait_event in that case too.
> 
> For the balloon page case here, it is fine to use wait_event. But for the free page
> case, I think it might not be suitable because the mm lock is being held.
> 
> Best,
> Wei

You will have to find a way to drop the lock and restart from where you
stopped then.

-- 
MST

Re: [PATCH v13 4/5] mm: support reporting free page blocks

[Michael S. Tsirkin]

On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote:
> On Thursday, August 3, 2017 9:51 PM, Michal Hocko: 
> > As I've said earlier. Start simple optimize incrementally with some numbers to
> > justify a more subtle code.
> > --
> 
> OK. Let's start with the simple implementation as you suggested.
> 
> Best,
> Wei

The tricky part is when you need to drop the lock and
then restart because the device is busy. Would it maybe
make sense to rotate the list so that new head
will consist of pages not yet sent to device?

-- 
MST

Re: [PATCH v13 4/5] mm: support reporting free page blocks

[Michal Hocko]

On Fri 04-08-17 00:02:01, Michael S. Tsirkin wrote:
> On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote:
> > On Thursday, August 3, 2017 9:51 PM, Michal Hocko: 
> > > As I've said earlier. Start simple optimize incrementally with some numbers to
> > > justify a more subtle code.
> > > --
> > 
> > OK. Let's start with the simple implementation as you suggested.
> > 
> > Best,
> > Wei
> 
> The tricky part is when you need to drop the lock and
> then restart because the device is busy. Would it maybe
> make sense to rotate the list so that new head
> will consist of pages not yet sent to device?

No, I this should be strictly non-modifying API.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v13 4/5] mm: support reporting free page blocks

[Wei Wang]

On 08/04/2017 03:53 PM, Michal Hocko wrote:
> On Fri 04-08-17 00:02:01, Michael S. Tsirkin wrote:
>> On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote:
>>> On Thursday, August 3, 2017 9:51 PM, Michal Hocko:
>>>> As I've said earlier. Start simple optimize incrementally with some numbers to
>>>> justify a more subtle code.
>>>> --
>>> OK. Let's start with the simple implementation as you suggested.
>>>
>>> Best,
>>> Wei
>> The tricky part is when you need to drop the lock and
>> then restart because the device is busy. Would it maybe
>> make sense to rotate the list so that new head
>> will consist of pages not yet sent to device?
> No, I this should be strictly non-modifying API.


Just get the context here for discussion:

     spin_lock_irqsave(&zone->lock, flags);
     ...
     visit(opaque2, pfn, 1<<order);
     spin_unlock_irqrestore(&zone->lock, flags);

The concern is that the callback may cause the lock be
taken too long.


I think here we can have two options:
- Option 1: Put a Note for the callback: the callback function
     should not block and it should finish as soon as possible.
     (when implementing an interrupt handler, we also have
     such similar rules in mind, right?).

For our use case, the callback just puts the reported page
block to the ring, then returns. If the ring is full as the host
is busy, then I think it should skip this one, and just return.
Because:
     A. This is an optimization feature, losing a couple of free
          pages to report isn't that important;
     B. In reality, I think it's uncommon to see this ring getting
         full (I didn't observe ring full in the tests), since the host
         (consumer) is notified to take out the page block right
         after it is added.

- Option 2: Put the callback function outside the lock
     What's input into the callback is just a pfn, and the callback
     won't access the corresponding pages. So, I still think it won't
     be an issue no matter what status of the pages is after they
     are reported (even they doesn't exit due to hot-remove).


What would you guys think?

Best,
Wei

Re: [PATCH v13 4/5] mm: support reporting free page blocks

[Michal Hocko]

On Fri 04-08-17 16:15:24, Wei Wang wrote:
> On 08/04/2017 03:53 PM, Michal Hocko wrote:
> >On Fri 04-08-17 00:02:01, Michael S. Tsirkin wrote:
> >>On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote:
> >>>On Thursday, August 3, 2017 9:51 PM, Michal Hocko:
> >>>>As I've said earlier. Start simple optimize incrementally with some numbers to
> >>>>justify a more subtle code.
> >>>>--
> >>>OK. Let's start with the simple implementation as you suggested.
> >>>
> >>>Best,
> >>>Wei
> >>The tricky part is when you need to drop the lock and
> >>then restart because the device is busy. Would it maybe
> >>make sense to rotate the list so that new head
> >>will consist of pages not yet sent to device?
> >No, I this should be strictly non-modifying API.
> 
> 
> Just get the context here for discussion:
> 
>     spin_lock_irqsave(&zone->lock, flags);
>     ...
>     visit(opaque2, pfn, 1<<order);
>     spin_unlock_irqrestore(&zone->lock, flags);
> 
> The concern is that the callback may cause the lock be
> taken too long.
> 
> 
> I think here we can have two options:
> - Option 1: Put a Note for the callback: the callback function
>     should not block and it should finish as soon as possible.
>     (when implementing an interrupt handler, we also have
>     such similar rules in mind, right?).

absolutely

> For our use case, the callback just puts the reported page
> block to the ring, then returns. If the ring is full as the host
> is busy, then I think it should skip this one, and just return.
> Because:
>     A. This is an optimization feature, losing a couple of free
>          pages to report isn't that important;
>     B. In reality, I think it's uncommon to see this ring getting
>         full (I didn't observe ring full in the tests), since the host
>         (consumer) is notified to take out the page block right
>         after it is added.

I thought you only updated a pre allocated bitmat... Anyway, I cannot
comment on this part much as I am not familiar with your usecase.
 
> - Option 2: Put the callback function outside the lock
>     What's input into the callback is just a pfn, and the callback
>     won't access the corresponding pages. So, I still think it won't
>     be an issue no matter what status of the pages is after they
>     are reported (even they doesn't exit due to hot-remove).

This would make the API implementation more complex and I am not yet
convinced we really need that.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v13 4/5] mm: support reporting free page blocks

[Wei Wang]

On 08/04/2017 04:24 PM, Michal Hocko wrote:
>
>> For our use case, the callback just puts the reported page
>> block to the ring, then returns. If the ring is full as the host
>> is busy, then I think it should skip this one, and just return.
>> Because:
>>      A. This is an optimization feature, losing a couple of free
>>           pages to report isn't that important;
>>      B. In reality, I think it's uncommon to see this ring getting
>>          full (I didn't observe ring full in the tests), since the host
>>          (consumer) is notified to take out the page block right
>>          after it is added.
> I thought you only updated a pre allocated bitmat... Anyway, I cannot
> comment on this part much as I am not familiar with your usecase.
>   

Actually the bitmap is in the hypervisor (host). The callback puts the
(pfn,size) on a ring which is shared with the hypervisor, then the
hypervisor takes that info from the ring and updates that bitmap.


Best,
Wei

Re: [PATCH v13 1/5] Introduce xbitmap

[Wei Wang]

On 08/03/2017 02:38 PM, Wei Wang wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
>
> The eXtensible Bitmap is a sparse bitmap representation which is
> efficient for set bits which tend to cluster.  It supports up to
> 'unsigned long' worth of bits, and this commit adds the bare bones --
> xb_set_bit(), xb_clear_bit() and xb_test_bit().
>
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>   include/linux/radix-tree.h |   2 +
>   include/linux/xbitmap.h    |  49 ++++++++++++++++
>   lib/radix-tree.c           | 139 ++++++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 188 insertions(+), 2 deletions(-)
>   create mode 100644 include/linux/xbitmap.h
>
> diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h
> index 3e57350..428ccc9 100644
> --- a/include/linux/radix-tree.h
> +++ b/include/linux/radix-tree.h


Hi Matthew,

Could you please help to upstream this patch?


Best,
Wei

Re: [PATCH v13 4/5] mm: support reporting free page blocks

[Wei Wang]

On 08/03/2017 05:11 PM, Michal Hocko wrote:
> On Thu 03-08-17 14:38:18, Wei Wang wrote:
> This is just too ugly and wrong actually. Never provide struct page
> pointers outside of the zone->lock. What I've had in mind was to simply
> walk free lists of the suitable order and call the callback for each one.
> Something as simple as
>
> 	for (i = 0; i < MAX_NR_ZONES; i++) {
> 		struct zone *zone = &pgdat->node_zones[i];
>
> 		if (!populated_zone(zone))
> 			continue;

Can we directly use for_each_populated_zone(zone) here?


> 		spin_lock_irqsave(&zone->lock, flags);
> 		for (order = min_order; order < MAX_ORDER; ++order) {


This appears to be covered by for_each_migratetype_order(order, mt) below.


> 			struct free_area *free_area = &zone->free_area[order];
> 			enum migratetype mt;
> 			struct page *page;
>
> 			if (!free_area->nr_pages)
> 				continue;
>
> 			for_each_migratetype_order(order, mt) {
> 				list_for_each_entry(page,
> 						&free_area->free_list[mt], lru) {
>
> 					pfn = page_to_pfn(page);
> 					visit(opaque2, prn, 1<<order);
> 				}
> 			}
> 		}
>
> 		spin_unlock_irqrestore(&zone->lock, flags);
> 	}
>
> [...]
>

What do you think if we further simply the above implementation like this:

for_each_populated_zone(zone) {
                 for_each_migratetype_order_decend(1, order, mt) {
                         spin_lock_irqsave(&zone->lock, flags);
                         list_for_each_entry(page,
&zone->free_area[order].free_list[mt], lru) {
                                 pfn = page_to_pfn(page);
                                 visit(opaque1, pfn, 1 << order);
                         }
                         spin_unlock_irqrestore(&zone->lock, flags);
                 }
         }


Best,
Wei

Re: [virtio-dev] Re: [PATCH v13 4/5] mm: support reporting free page blocks

[Wei Wang]

On 08/08/2017 02:12 PM, Wei Wang wrote:
> On 08/03/2017 05:11 PM, Michal Hocko wrote:
>> On Thu 03-08-17 14:38:18, Wei Wang wrote:
>> This is just too ugly and wrong actually. Never provide struct page
>> pointers outside of the zone->lock. What I've had in mind was to simply
>> walk free lists of the suitable order and call the callback for each 
>> one.
>> Something as simple as
>>
>>     for (i = 0; i < MAX_NR_ZONES; i++) {
>>         struct zone *zone = &pgdat->node_zones[i];
>>
>>         if (!populated_zone(zone))
>>             continue;
>
> Can we directly use for_each_populated_zone(zone) here?
>
>
>> spin_lock_irqsave(&zone->lock, flags);
>>         for (order = min_order; order < MAX_ORDER; ++order) {
>
>
> This appears to be covered by for_each_migratetype_order(order, mt) 
> below.
>
>
>>             struct free_area *free_area = &zone->free_area[order];
>>             enum migratetype mt;
>>             struct page *page;
>>
>>             if (!free_area->nr_pages)
>>                 continue;
>>
>>             for_each_migratetype_order(order, mt) {
>>                 list_for_each_entry(page,
>>                         &free_area->free_list[mt], lru) {
>>
>>                     pfn = page_to_pfn(page);
>>                     visit(opaque2, prn, 1<<order);
>>                 }
>>             }
>>         }
>>
>>         spin_unlock_irqrestore(&zone->lock, flags);
>>     }
>>
>> [...]
>>
>
> What do you think if we further simply the above implementation like 
> this:
>
> for_each_populated_zone(zone) {
>                 for_each_migratetype_order_decend(1, order, mt) {

here it will be min_order (passed by the caller), instead of "1",
that is, for_each_migratetype_order_decend(min_order, order, mt)


> spin_lock_irqsave(&zone->lock, flags);
>                         list_for_each_entry(page,
> &zone->free_area[order].free_list[mt], lru) {
>                                 pfn = page_to_pfn(page);
>                                 visit(opaque1, pfn, 1 << order);
>                         }
>                         spin_unlock_irqrestore(&zone->lock, flags);
>                 }
>         }
>
>


Best,
Wei

Re: [PATCH v13 1/5] Introduce xbitmap

[Andrew Morton]

On Thu,  3 Aug 2017 14:38:15 +0800 Wei Wang <wei.w.wang@intel.com> wrote:

> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> The eXtensible Bitmap is a sparse bitmap representation which is
> efficient for set bits which tend to cluster.  It supports up to
> 'unsigned long' worth of bits, and this commit adds the bare bones --
> xb_set_bit(), xb_clear_bit() and xb_test_bit().

Would like to see some additional details here justifying the change. 
The sole user is virtio-balloon, yes?  What alternatives were examined
and what are the benefits of this approach?

Have you identified any other subsystems which could utilize this?

>
> ...
>
> --- a/lib/radix-tree.c
> +++ b/lib/radix-tree.c
> @@ -37,6 +37,7 @@
>  #include <linux/rcupdate.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
> +#include <linux/xbitmap.h>
>  
>  
>  /* Number of nodes in fully populated tree of given height */
> @@ -78,6 +79,14 @@ static struct kmem_cache *radix_tree_node_cachep;
>  #define IDA_PRELOAD_SIZE	(IDA_MAX_PATH * 2 - 1)
>  
>  /*
> + * The XB can go up to unsigned long, but also uses a bitmap.

This comment is hard to understand.

> + */
> +#define XB_INDEX_BITS		(BITS_PER_LONG - ilog2(IDA_BITMAP_BITS))
> +#define XB_MAX_PATH		(DIV_ROUND_UP(XB_INDEX_BITS, \
> +					      RADIX_TREE_MAP_SHIFT))
> +#define XB_PRELOAD_SIZE		(XB_MAX_PATH * 2 - 1)
> +
>
> ...
>  
> +void xb_preload(gfp_t gfp)
> +{
> +	__radix_tree_preload(gfp, XB_PRELOAD_SIZE);
> +	if (!this_cpu_read(ida_bitmap)) {
> +		struct ida_bitmap *bitmap = kmalloc(sizeof(*bitmap), gfp);
> +
> +		if (!bitmap)
> +			return;
> +		bitmap = this_cpu_cmpxchg(ida_bitmap, NULL, bitmap);
> +		kfree(bitmap);
> +	}
> +}
> +EXPORT_SYMBOL(xb_preload);

Please document the exported API.  It's conventional to do this in
kerneldoc but for some reason kerneldoc makes people write
uninteresting and unuseful documentation.  Be sure to cover the
*useful* stuff: what it does, why it does it, under which circumstances
it should be used, what the caller-provided locking should look like,
what the return values mean, etc.  Stuff which programmers actually
will benefit from knowing.

> +int xb_set_bit(struct xb *xb, unsigned long bit)
>
> ...
>
> +int xb_clear_bit(struct xb *xb, unsigned long bit)

There's quite a lot of common code here.  Did you investigate factoring
that out in some fashion?

> +bool xb_test_bit(const struct xb *xb, unsigned long bit)
> +{
> +	unsigned long index = bit / IDA_BITMAP_BITS;
> +	const struct radix_tree_root *root = &xb->xbrt;
> +	struct ida_bitmap *bitmap = radix_tree_lookup(root, index);
> +
> +	bit %= IDA_BITMAP_BITS;
> +
> +	if (!bitmap)
> +		return false;
> +	if (radix_tree_exception(bitmap)) {
> +		bit += RADIX_TREE_EXCEPTIONAL_SHIFT;
> +		if (bit > BITS_PER_LONG)
> +			return false;
> +		return (unsigned long)bitmap & (1UL << bit);
> +	}
> +	return test_bit(bit, bitmap->bitmap);
> +}
> +

Missing EXPORT_SYMBOL?


Perhaps all this code should go into a new lib/xbitmap.c.

Re: [PATCH v13 1/5] Introduce xbitmap

[Wei Wang]

On 08/10/2017 05:36 AM, Andrew Morton wrote:
> On Thu,  3 Aug 2017 14:38:15 +0800 Wei Wang <wei.w.wang@intel.com> wrote:
>
>> From: Matthew Wilcox <mawilcox@microsoft.com>
>>
>> The eXtensible Bitmap is a sparse bitmap representation which is
>> efficient for set bits which tend to cluster.  It supports up to
>> 'unsigned long' worth of bits, and this commit adds the bare bones --
>> xb_set_bit(), xb_clear_bit() and xb_test_bit().
> Would like to see some additional details here justifying the change.
> The sole user is virtio-balloon, yes?  What alternatives were examined
> and what are the benefits of this approach?
>
> Have you identified any other subsystems which could utilize this?


The idea and implementation comes from Matthew, but I can share
my thought here (mostly from a user perspective):

This seems to be the first kind that uses bitmaps based on radix like
structures for id recording purposes.  The id is given by the user,
which is different from ida (ida is used for id allocation purpose). A
bitmap is allocated on demand when an id provided by the user to
record beyond the existing id range that the allocated bitmaps can
cover. Benefits are actually from the radix implementation - efficient
storage and quick lookup.

We use it in virtio-balloon to record the pfns of balloon pages, that is,
a pfn is an id to be recorded into the bitmap. The bitmaps are latter
searched for continuous "1" bits, which correspond to continuous pfns.

Virtio-ballon is the first user of it. I'm not sure about other subsystems,
but other developers may notice it and use it once it's available there.


>> ...
>>
>> --- a/lib/radix-tree.c
>> +++ b/lib/radix-tree.c
>> @@ -37,6 +37,7 @@
>>   #include <linux/rcupdate.h>
>>   #include <linux/slab.h>
>>   #include <linux/string.h>
>> +#include <linux/xbitmap.h>
>>   
>>   
>>   /* Number of nodes in fully populated tree of given height */
>> @@ -78,6 +79,14 @@ static struct kmem_cache *radix_tree_node_cachep;
>>   #define IDA_PRELOAD_SIZE	(IDA_MAX_PATH * 2 - 1)
>>   
>>   /*
>> + * The XB can go up to unsigned long, but also uses a bitmap.
> This comment is hard to understand.

Also not sure bout it.

>
>> + */
>> +#define XB_INDEX_BITS		(BITS_PER_LONG - ilog2(IDA_BITMAP_BITS))
>> +#define XB_MAX_PATH		(DIV_ROUND_UP(XB_INDEX_BITS, \
>> +					      RADIX_TREE_MAP_SHIFT))
>> +#define XB_PRELOAD_SIZE		(XB_MAX_PATH * 2 - 1)
>> +
>>
>> ...
>>   
>> +void xb_preload(gfp_t gfp)
>> +{
>> +	__radix_tree_preload(gfp, XB_PRELOAD_SIZE);
>> +	if (!this_cpu_read(ida_bitmap)) {
>> +		struct ida_bitmap *bitmap = kmalloc(sizeof(*bitmap), gfp);
>> +
>> +		if (!bitmap)
>> +			return;
>> +		bitmap = this_cpu_cmpxchg(ida_bitmap, NULL, bitmap);
>> +		kfree(bitmap);
>> +	}
>> +}
>> +EXPORT_SYMBOL(xb_preload);
> Please document the exported API.  It's conventional to do this in
> kerneldoc but for some reason kerneldoc makes people write
> uninteresting and unuseful documentation.  Be sure to cover the
> *useful* stuff: what it does, why it does it, under which circumstances
> it should be used, what the caller-provided locking should look like,
> what the return values mean, etc.  Stuff which programmers actually
> will benefit from knowing.

OK.

>
>> +int xb_set_bit(struct xb *xb, unsigned long bit)
>>
>> ...
>>
>> +int xb_clear_bit(struct xb *xb, unsigned long bit)
> There's quite a lot of common code here.  Did you investigate factoring
> that out in some fashion?


If we combine the functions into one
xb_bit_ops(struct xb *xb, unsigned long bit, enum xb_ops ops),
it will be a big function with some if (ops == set/clear/test)-else,
not sure if that would look good.


>
>> +bool xb_test_bit(const struct xb *xb, unsigned long bit)
>> +{
>> +	unsigned long index = bit / IDA_BITMAP_BITS;
>> +	const struct radix_tree_root *root = &xb->xbrt;
>> +	struct ida_bitmap *bitmap = radix_tree_lookup(root, index);
>> +
>> +	bit %= IDA_BITMAP_BITS;
>> +
>> +	if (!bitmap)
>> +		return false;
>> +	if (radix_tree_exception(bitmap)) {
>> +		bit += RADIX_TREE_EXCEPTIONAL_SHIFT;
>> +		if (bit > BITS_PER_LONG)
>> +			return false;
>> +		return (unsigned long)bitmap & (1UL << bit);
>> +	}
>> +	return test_bit(bit, bitmap->bitmap);
>> +}
>> +
> Missing EXPORT_SYMBOL?

Yes, will add that, thanks.

>
> Perhaps all this code should go into a new lib/xbitmap.c.

Ok, will relocate.


Best,
Wei

Re: [virtio-dev] Re: [PATCH v13 4/5] mm: support reporting free page blocks

[Michal Hocko]

On Tue 08-08-17 14:34:25, Wei Wang wrote:
> On 08/08/2017 02:12 PM, Wei Wang wrote:
> >On 08/03/2017 05:11 PM, Michal Hocko wrote:
> >>On Thu 03-08-17 14:38:18, Wei Wang wrote:
> >>This is just too ugly and wrong actually. Never provide struct page
> >>pointers outside of the zone->lock. What I've had in mind was to simply
> >>walk free lists of the suitable order and call the callback for each
> >>one.
> >>Something as simple as
> >>
> >>    for (i = 0; i < MAX_NR_ZONES; i++) {
> >>        struct zone *zone = &pgdat->node_zones[i];
> >>
> >>        if (!populated_zone(zone))
> >>            continue;
> >
> >Can we directly use for_each_populated_zone(zone) here?

yes, my example couldn't because I was still assuming per-node API

> >>spin_lock_irqsave(&zone->lock, flags);
> >>        for (order = min_order; order < MAX_ORDER; ++order) {
> >
> >
> >This appears to be covered by for_each_migratetype_order(order, mt) below.

yes but
#define for_each_migratetype_order(order, type) \
	for (order = 0; order < MAX_ORDER; order++) \
		for (type = 0; type < MIGRATE_TYPES; type++)

so you would have to skip orders < min_order
-- 
Michal Hocko
SUSE Labs

Re: [virtio-dev] Re: [PATCH v13 4/5] mm: support reporting free page blocks

[Wei Wang]

On 08/10/2017 03:05 PM, Michal Hocko wrote:
> On Tue 08-08-17 14:34:25, Wei Wang wrote:
>> On 08/08/2017 02:12 PM, Wei Wang wrote:
>>> On 08/03/2017 05:11 PM, Michal Hocko wrote:
>>>> On Thu 03-08-17 14:38:18, Wei Wang wrote:
>>>> This is just too ugly and wrong actually. Never provide struct page
>>>> pointers outside of the zone->lock. What I've had in mind was to simply
>>>> walk free lists of the suitable order and call the callback for each
>>>> one.
>>>> Something as simple as
>>>>
>>>>     for (i = 0; i < MAX_NR_ZONES; i++) {
>>>>         struct zone *zone = &pgdat->node_zones[i];
>>>>
>>>>         if (!populated_zone(zone))
>>>>             continue;
>>> Can we directly use for_each_populated_zone(zone) here?
> yes, my example couldn't because I was still assuming per-node API
>
>>>> spin_lock_irqsave(&zone->lock, flags);
>>>>         for (order = min_order; order < MAX_ORDER; ++order) {
>>>
>>> This appears to be covered by for_each_migratetype_order(order, mt) below.
> yes but
> #define for_each_migratetype_order(order, type) \
> 	for (order = 0; order < MAX_ORDER; order++) \
> 		for (type = 0; type < MIGRATE_TYPES; type++)
>
> so you would have to skip orders < min_order

Yes, that's why we have a new macro

#define for_each_migratetype_order_decend(min_order, order, type) \
  for (order = MAX_ORDER - 1; order < MAX_ORDER && order >= min_order; \
  order--) \
     for (type = 0; type < MIGRATE_TYPES; type++)

If you don't like the macro, we can also directly use it in the code.

I think it would be better to report the larger free page block first, since
the callback has an opportunity (though just a theoretical possibility, 
good to
take that into consideration if possible) to skip reporting the given 
free page
block to the hypervisor as the ring gets full. Losing the small block is 
better
than losing the larger one, in terms of the optimization work.


Best,
Wei

Re: [virtio-dev] Re: [PATCH v13 4/5] mm: support reporting free page blocks

[Michal Hocko]

On Thu 10-08-17 15:38:34, Wei Wang wrote:
> On 08/10/2017 03:05 PM, Michal Hocko wrote:
> >On Tue 08-08-17 14:34:25, Wei Wang wrote:
> >>On 08/08/2017 02:12 PM, Wei Wang wrote:
> >>>On 08/03/2017 05:11 PM, Michal Hocko wrote:
> >>>>On Thu 03-08-17 14:38:18, Wei Wang wrote:
> >>>>This is just too ugly and wrong actually. Never provide struct page
> >>>>pointers outside of the zone->lock. What I've had in mind was to simply
> >>>>walk free lists of the suitable order and call the callback for each
> >>>>one.
> >>>>Something as simple as
> >>>>
> >>>>    for (i = 0; i < MAX_NR_ZONES; i++) {
> >>>>        struct zone *zone = &pgdat->node_zones[i];
> >>>>
> >>>>        if (!populated_zone(zone))
> >>>>            continue;
> >>>Can we directly use for_each_populated_zone(zone) here?
> >yes, my example couldn't because I was still assuming per-node API
> >
> >>>>spin_lock_irqsave(&zone->lock, flags);
> >>>>        for (order = min_order; order < MAX_ORDER; ++order) {
> >>>
> >>>This appears to be covered by for_each_migratetype_order(order, mt) below.
> >yes but
> >#define for_each_migratetype_order(order, type) \
> >	for (order = 0; order < MAX_ORDER; order++) \
> >		for (type = 0; type < MIGRATE_TYPES; type++)
> >
> >so you would have to skip orders < min_order
> 
> Yes, that's why we have a new macro
> 
> #define for_each_migratetype_order_decend(min_order, order, type) \
>  for (order = MAX_ORDER - 1; order < MAX_ORDER && order >= min_order; \
>  order--) \
>     for (type = 0; type < MIGRATE_TYPES; type++)
> 
> If you don't like the macro, we can also directly use it in the code.
> 
> I think it would be better to report the larger free page block first, since
> the callback has an opportunity (though just a theoretical possibility, good
> to
> take that into consideration if possible) to skip reporting the given free
> page
> block to the hypervisor as the ring gets full. Losing the small block is
> better
> than losing the larger one, in terms of the optimization work.

I see. But I think this is so specialized that opencoding the macro
would be easier to read.

-- 
Michal Hocko
SUSE Labs

Re: [virtio-dev] [PATCH v13 0/5] Virtio-balloon Enhancement

[Adam Tao]

On Thu, Aug 03, 2017 at 02:38:14PM +0800, Wei Wang wrote:
> This patch series enhances the existing virtio-balloon with the following
> new features:
> 1) fast ballooning: transfer ballooned pages between the guest and host in
> chunks using sgs, instead of one by one; and
> 2) free_page_vq: a new virtqueue to report guest free pages to the host.
> 
Hi wei,
The reason we add the new vq for the migration feature is based on
what(original design based on inflate and deflate vq)?
I am wondering if we add new feature in the future do we still need to add new type
of vq?
Do we need to add one command queue for the common purpose(including
different type of requests except the in/deflate ones)?
Thanks
Adam
> The second feature can be used to accelerate live migration of VMs. Here
> are some details:
> 
> Live migration needs to transfer the VM's memory from the source machine
> to the destination round by round. For the 1st round, all the VM's memory
> is transferred. From the 2nd round, only the pieces of memory that were
> written by the guest (after the 1st round) are transferred. One method
> that is popularly used by the hypervisor to track which part of memory is
> written is to write-protect all the guest memory.
> 
> The second feature  enables the optimization of the 1st round memory
> transfer - the hypervisor can skip the transfer of guest free pages in the
> 1st round. It is not concerned that the memory pages are used after they
> are given to the hypervisor as a hint of the free pages, because they will
> be tracked by the hypervisor and transferred in the next round if they are
> used and written.
> 
> Change Log:
> v12->v13:
> 1) mm: use a callback function to handle the the free page blocks from the
> report function. This avoids exposing the zone internal to a kernel module.
> 2) virtio-balloon: send balloon pages or a free page block using a single sg
> each time. This has the benefits of simpler implementation with no new APIs.
> 3) virtio-balloon: the free_page_vq is used to report free pages only (no
> multiple usages interleaving)
> 4) virtio-balloon: Balloon pages and free page blocks are sent via input sgs,
> and the completion signal to the host is sent via an output sg.
> 
> v11->v12:
> 1) xbitmap: use the xbitmap from Matthew Wilcox to record ballooned pages.
> 2) virtio-ring: enable the driver to build up a desc chain using vring desc.
> 3) virtio-ring: Add locking to the existing START_USE() and END_USE() macro
> to lock/unlock the vq when a vq operation starts/ends.
> 4) virtio-ring: add virtqueue_kick_sync() and virtqueue_kick_async()
> 5) virtio-balloon: describe chunks of ballooned pages and free pages blocks
> directly using one or more chains of desc from the vq.
> 
> v10->v11:
> 1) virtio_balloon: use vring_desc to describe a chunk;
> 2) virtio_ring: support to add an indirect desc table to virtqueue;
> 3)  virtio_balloon: use cmdq to report guest memory statistics.
> 
> v9->v10:
> 1) mm: put report_unused_page_block() under CONFIG_VIRTIO_BALLOON;
> 2) virtio-balloon: add virtballoon_validate();
> 3) virtio-balloon: msg format change;
> 4) virtio-balloon: move miscq handling to a task on system_freezable_wq;
> 5) virtio-balloon: code cleanup.
> 
> v8->v9:
> 1) Split the two new features, VIRTIO_BALLOON_F_BALLOON_CHUNKS and
> VIRTIO_BALLOON_F_MISC_VQ, which were mixed together in the previous
> implementation;
> 2) Simpler function to get the free page block.
> 
> v7->v8:
> 1) Use only one chunk format, instead of two.
> 2) re-write the virtio-balloon implementation patch.
> 3) commit changes
> 4) patch re-org
> 
> Matthew Wilcox (1):
>   Introduce xbitmap
> 
> Wei Wang (4):
>   xbitmap: add xb_find_next_bit() and xb_zero()
>   virtio-balloon: VIRTIO_BALLOON_F_SG
>   mm: support reporting free page blocks
>   virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
> 
>  drivers/virtio/virtio_balloon.c     | 302 +++++++++++++++++++++++++++++++-----
>  include/linux/mm.h                  |   7 +
>  include/linux/mmzone.h              |   5 +
>  include/linux/radix-tree.h          |   2 +
>  include/linux/xbitmap.h             |  53 +++++++
>  include/uapi/linux/virtio_balloon.h |   2 +
>  lib/radix-tree.c                    | 167 +++++++++++++++++++-
>  mm/page_alloc.c                     | 109 +++++++++++++
>  8 files changed, 609 insertions(+), 38 deletions(-)
>  create mode 100644 include/linux/xbitmap.h
> 
> -- 
> 2.7.4
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH v13 0/5] Virtio-balloon Enhancement

[Wei Wang]

On 08/16/2017 01:57 PM, Adam Tao wrote:
> On Thu, Aug 03, 2017 at 02:38:14PM +0800, Wei Wang wrote:
>> This patch series enhances the existing virtio-balloon with the following
>> new features:
>> 1) fast ballooning: transfer ballooned pages between the guest and host in
>> chunks using sgs, instead of one by one; and
>> 2) free_page_vq: a new virtqueue to report guest free pages to the host.
>>
> Hi wei,
> The reason we add the new vq for the migration feature is based on
> what(original design based on inflate and deflate vq)?
> I am wondering if we add new feature in the future do we still need to add new type
> of vq?
> Do we need to add one command queue for the common purpose(including
> different type of requests except the in/deflate ones)?
> Thanks
> Adam

Hi Adam,

The the free_page_vq is added to report free pages to the hypervisor.
Neither inflate nor deflate vq was for this purpose.

Based on the current implementation, a vq dedicated to one usage (i.e. 
report
free pages) is better, since mixing with other usages, e.g. a command vq to
handle multiple commands at the same time, would have some issues (e.g. one
being delayed by another due to some resource control), and it also 
results in
more complex interfaces between the driver and device.

For future usages which are still unknown at present, I think we can discuss
them case by case in the future.

Best,
Wei