diff mbox

[v13,5/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ

Message ID 1501742299-4369-6-git-send-email-wei.w.wang@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wang, Wei W Aug. 3, 2017, 6:38 a.m. UTC 
Add a new vq to report hints of guest free pages to the host.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Signed-off-by: Liang Li <liang.z.li@intel.com>
---
 drivers/virtio/virtio_balloon.c     | 164 ++++++++++++++++++++++++++++++------
 include/uapi/linux/virtio_balloon.h |   1 +
 2 files changed, 140 insertions(+), 25 deletions(-)

Comments

Pankaj Gupta Aug. 3, 2017, 8:13 a.m. UTC | #1 
> 
> Add a new vq to report hints of guest free pages to the host.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> ---
>  drivers/virtio/virtio_balloon.c     | 164
>  ++++++++++++++++++++++++++++++------
>  include/uapi/linux/virtio_balloon.h |   1 +
>  2 files changed, 140 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c
> b/drivers/virtio/virtio_balloon.c
> index 29aca0c..29c4a61 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -54,11 +54,12 @@ static struct vfsmount *balloon_mnt;
>  
>  struct virtio_balloon {
>          struct virtio_device *vdev;
> -        struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
> +        struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
>  
>          /* The balloon servicing is delegated to a freezable workqueue. */
>          struct work_struct update_balloon_stats_work;
>          struct work_struct update_balloon_size_work;
> +        struct work_struct report_free_page_work;
>  
>          /* Prevent updating balloon when it is being canceled. */
>          spinlock_t stop_update_lock;
> @@ -90,6 +91,13 @@ struct virtio_balloon {
>          /* Memory statistics */
>          struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
>  
> +        /*
> +         * Used by the device and driver to signal each other.
> +         * device->driver: start the free page report.
> +         * driver->device: end the free page report.
> +         */
> +        __virtio32 report_free_page_signal;
> +
>          /* To register callback in oom notifier call chain */
>          struct notifier_block nb;
>  };
> @@ -146,7 +154,7 @@ static void set_page_pfns(struct virtio_balloon *vb,
>  }
>  
>  static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
> -                        void *addr, uint32_t size)
> +                        void *addr, uint32_t size, bool busywait)
>  {
>          struct scatterlist sg;
>          unsigned int len;
> @@ -165,7 +173,12 @@ static void send_one_sg(struct virtio_balloon *vb,
> struct virtqueue *vq,
>                          cpu_relax();
>          }
>          virtqueue_kick(vq);
> -        wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> +        if (busywait)
> +                while (!virtqueue_get_buf(vq, &len) &&
> +                       !virtqueue_is_broken(vq))
> +                        cpu_relax();
> +        else
> +                wait_event(vb->acked, virtqueue_get_buf(vq, &len));
>  }
>  
>  /*
> @@ -197,11 +210,11 @@ static void tell_host_sgs(struct virtio_balloon *vb,
>                  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);
> +                        send_one_sg(vb, vq, sg_addr, sg_max_len, 0);
>                          sg_addr += sg_max_len;
>                          sg_len -= sg_max_len;
>                  }
> -                send_one_sg(vb, vq, sg_addr, sg_len);
> +                send_one_sg(vb, vq, sg_addr, sg_len, 0);
>                  xb_zero(&vb->page_xb, sg_pfn_start, sg_pfn_end);
>                  sg_pfn_start = sg_pfn_end + 1;
>          }
> @@ -503,42 +516,138 @@ static void update_balloon_size_func(struct
> work_struct *work)
>                  queue_work(system_freezable_wq, work);
>  }
>  
> +static void virtio_balloon_send_free_pages(void *opaque, unsigned long pfn,
> +                                           unsigned long nr_pages)
> +{
> +        struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
> +        void *addr = pfn_to_kaddr(pfn);
> +        uint32_t len = nr_pages << PAGE_SHIFT;
> +
> +        send_one_sg(vb, vb->free_page_vq, addr, len, 1);
> +}
> +
> +static void report_free_page_completion(struct virtio_balloon *vb)
> +{
> +        struct virtqueue *vq = vb->free_page_vq;
> +        struct scatterlist sg;
> +        unsigned int len;
> +
> +        sg_init_one(&sg, &vb->report_free_page_signal, sizeof(__virtio32));
> +        while (unlikely(virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL)
> +                        == -ENOSPC)) {
> +                virtqueue_kick(vq);
> +                while (!virtqueue_get_buf(vq, &len) &&
> +                       !virtqueue_is_broken(vq))
> +                        cpu_relax();
> +        }
> +        virtqueue_kick(vq);
> +}
> +
> +static void report_free_page(struct work_struct *work)
> +{
> +        struct virtio_balloon *vb;
> +
> +        vb = container_of(work, struct virtio_balloon, report_free_page_work);
> +        walk_free_mem_block(vb, 1, &virtio_balloon_send_free_pages);
> +        report_free_page_completion(vb);
> +}
> +
> +static void free_page_request(struct virtqueue *vq)
> +{
> +        struct virtio_balloon *vb = vq->vdev->priv;
> +
> +        queue_work(system_freezable_wq, &vb->report_free_page_work);
> +}
> +
>  static int init_vqs(struct virtio_balloon *vb)
>  {
> -        struct virtqueue *vqs[3];
> -        vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
> -        static const char * const names[] = { "inflate", "deflate", "stats" };
> -        int err, nvqs;
> +        struct virtqueue **vqs;
> +        vq_callback_t **callbacks;
> +        const char **names;
> +        struct scatterlist sg;
> +        int i, nvqs, err = -ENOMEM;
> +
> +        /* Inflateq and deflateq are used unconditionally */
> +        nvqs = 2;
> +        if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ))
> +                nvqs++;
> +        if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ))
> +                nvqs++;
> +
> +        /* Allocate space for find_vqs parameters */
> +        vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL);
> +        if (!vqs)
> +                goto err_vq;
> +        callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);
> +        if (!callbacks)
> +                goto err_callback;
> +        names = kmalloc_array(nvqs, sizeof(*names), GFP_KERNEL);
                    
       is size here (integer) intentional?

> +        if (!names)
> +                goto err_names;
> +
> +        callbacks[0] = balloon_ack;
> +        names[0] = "inflate";
> +        callbacks[1] = balloon_ack;
> +        names[1] = "deflate";
> +
> +        i = 2;
> +        if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> +                callbacks[i] = stats_request;

just thinking if memory for callbacks[3] & names[3] is allocated?

> +                names[i] = "stats";
> +                i++;
> +        }
>  
> -        /*
> -         * We expect two virtqueues: inflate and deflate, and
> -         * optionally stat.
> -         */
> -        nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2;
> -        err = virtio_find_vqs(vb->vdev, nvqs, vqs, callbacks, names, NULL);
> +        if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) {
> +                callbacks[i] = free_page_request;
> +                names[i] = "free_page_vq";
> +        }
> +
> +        err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names,
> +                                         NULL, NULL);
>          if (err)
> -                return err;
> +                goto err_find;
>  
>          vb->inflate_vq = vqs[0];
>          vb->deflate_vq = vqs[1];
> +        i = 2;
>          if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> -                struct scatterlist sg;
> -                unsigned int num_stats;
> -                vb->stats_vq = vqs[2];
> -
> +                vb->stats_vq = vqs[i++];
>                  /*
>                   * Prime this virtqueue with one buffer so the hypervisor can
>                   * use it to signal us later (it can't be broken yet!).
>                   */
> -                num_stats = update_balloon_stats(vb);
> -
> -                sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
> +                sg_init_one(&sg, vb->stats, sizeof(vb->stats));
>                  if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
>                      < 0)
>                          BUG();
>                  virtqueue_kick(vb->stats_vq);
>          }
> +
> +        if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) {
> +                vb->free_page_vq = vqs[i];
> +                vb->report_free_page_signal = 0;
> +                sg_init_one(&sg, &vb->report_free_page_signal,
> +                            sizeof(__virtio32));
> +                if (virtqueue_add_outbuf(vb->free_page_vq, &sg, 1, vb,
> +                                         GFP_KERNEL) < 0)
> +                        dev_warn(&vb->vdev->dev, "%s: add signal buf fail\n",
> +                                 __func__);
> +                virtqueue_kick(vb->free_page_vq);
> +        }
> +
> +        kfree(names);
> +        kfree(callbacks);
> +        kfree(vqs);
>          return 0;
> +
> +err_find:
> +        kfree(names);
> +err_names:
> +        kfree(callbacks);
> +err_callback:
> +        kfree(vqs);
> +err_vq:
> +        return err;
>  }
>  
>  #ifdef CONFIG_BALLOON_COMPACTION
> @@ -590,7 +699,7 @@ static int virtballoon_migratepage(struct
> balloon_dev_info *vb_dev_info,
>          spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
>          if (use_sg) {
>                  send_one_sg(vb, vb->inflate_vq, page_address(newpage),
> -                            PAGE_SIZE);
> +                            PAGE_SIZE, 0);
>          } else {
>                  vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
>                  set_page_pfns(vb, vb->pfns, newpage);
> @@ -600,7 +709,7 @@ static int virtballoon_migratepage(struct
> balloon_dev_info *vb_dev_info,
>          balloon_page_delete(page);
>          if (use_sg) {
>                  send_one_sg(vb, vb->deflate_vq, page_address(page),
> -                            PAGE_SIZE);
> +                            PAGE_SIZE, 0);
>          } else {
>                  vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
>                  set_page_pfns(vb, vb->pfns, page);
> @@ -667,6 +776,9 @@ static int virtballoon_probe(struct virtio_device *vdev)
>          if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
>                  xb_init(&vb->page_xb);
>  
> +        if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ))
> +                INIT_WORK(&vb->report_free_page_work, report_free_page);
> +
>          vb->nb.notifier_call = virtballoon_oom_notify;
>          vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
>          err = register_oom_notifier(&vb->nb);
> @@ -731,6 +843,7 @@ static void virtballoon_remove(struct virtio_device
> *vdev)
>          spin_unlock_irq(&vb->stop_update_lock);
>          cancel_work_sync(&vb->update_balloon_size_work);
>          cancel_work_sync(&vb->update_balloon_stats_work);
> +        cancel_work_sync(&vb->report_free_page_work);
>  
>          xb_empty(&vb->page_xb);
>          remove_common(vb);
> @@ -785,6 +898,7 @@ static unsigned int features[] = {
>          VIRTIO_BALLOON_F_STATS_VQ,
>          VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
>          VIRTIO_BALLOON_F_SG,
> +        VIRTIO_BALLOON_F_FREE_PAGE_VQ,
>  };
>  
>  static struct virtio_driver virtio_balloon_driver = {
> diff --git a/include/uapi/linux/virtio_balloon.h
> b/include/uapi/linux/virtio_balloon.h
> index 37780a7..8214f84 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -35,6 +35,7 @@
>  #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 */
> +#define VIRTIO_BALLOON_F_FREE_PAGE_VQ        4 /* Virtqueue to report free pages */
>  
>  /* Size of a PFN in the balloon interface. */
>  #define VIRTIO_BALLOON_PFN_SHIFT 12
> --
> 2.7.4
> 
>
Wang, Wei W Aug. 3, 2017, 12:28 p.m. UTC | #2 
On 08/03/2017 04:13 PM, Pankaj Gupta wrote:
>>
>> +        /* Allocate space for find_vqs parameters */
>> +        vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL);
>> +        if (!vqs)
>> +                goto err_vq;
>> +        callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);
>> +        if (!callbacks)
>> +                goto err_callback;
>> +        names = kmalloc_array(nvqs, sizeof(*names), GFP_KERNEL);
>                      
>         is size here (integer) intentional?


Sorry, I didn't get it. Could you please elaborate more?


>
>> +        if (!names)
>> +                goto err_names;
>> +
>> +        callbacks[0] = balloon_ack;
>> +        names[0] = "inflate";
>> +        callbacks[1] = balloon_ack;
>> +        names[1] = "deflate";
>> +
>> +        i = 2;
>> +        if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
>> +                callbacks[i] = stats_request;
> just thinking if memory for callbacks[3] & names[3] is allocated?


Yes, the above kmalloc_array allocated them.


Best,
Wei
Michael S. Tsirkin Aug. 3, 2017, 12:33 p.m. UTC | #3 
On Thu, Aug 03, 2017 at 02:38:19PM +0800, Wei Wang wrote:
> Add a new vq to report hints of guest free pages to the host.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> ---
>  drivers/virtio/virtio_balloon.c     | 164 ++++++++++++++++++++++++++++++------
>  include/uapi/linux/virtio_balloon.h |   1 +
>  2 files changed, 140 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 29aca0c..29c4a61 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -54,11 +54,12 @@ static struct vfsmount *balloon_mnt;
>  
>  struct virtio_balloon {
>  	struct virtio_device *vdev;
> -	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
> +	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
>  
>  	/* The balloon servicing is delegated to a freezable workqueue. */
>  	struct work_struct update_balloon_stats_work;
>  	struct work_struct update_balloon_size_work;
> +	struct work_struct report_free_page_work;
>  
>  	/* Prevent updating balloon when it is being canceled. */
>  	spinlock_t stop_update_lock;
> @@ -90,6 +91,13 @@ struct virtio_balloon {
>  	/* Memory statistics */
>  	struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
>  
> +	/*
> +	 * Used by the device and driver to signal each other.
> +	 * device->driver: start the free page report.
> +	 * driver->device: end the free page report.
> +	 */
> +	__virtio32 report_free_page_signal;
> +
>  	/* To register callback in oom notifier call chain */
>  	struct notifier_block nb;
>  };
> @@ -146,7 +154,7 @@ static void set_page_pfns(struct virtio_balloon *vb,
>  }
>  
>  static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
> -			void *addr, uint32_t size)
> +			void *addr, uint32_t size, bool busywait)
>  {
>  	struct scatterlist sg;
>  	unsigned int len;
> @@ -165,7 +173,12 @@ static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
>  			cpu_relax();
>  	}
>  	virtqueue_kick(vq);
> -	wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> +	if (busywait)
> +		while (!virtqueue_get_buf(vq, &len) &&
> +		       !virtqueue_is_broken(vq))
> +			cpu_relax();
> +	else
> +		wait_event(vb->acked, virtqueue_get_buf(vq, &len));
>  }
>  
>  /*
> @@ -197,11 +210,11 @@ static void tell_host_sgs(struct virtio_balloon *vb,
>  		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);
> +			send_one_sg(vb, vq, sg_addr, sg_max_len, 0);
>  			sg_addr += sg_max_len;
>  			sg_len -= sg_max_len;
>  		}
> -		send_one_sg(vb, vq, sg_addr, sg_len);
> +		send_one_sg(vb, vq, sg_addr, sg_len, 0);
>  		xb_zero(&vb->page_xb, sg_pfn_start, sg_pfn_end);
>  		sg_pfn_start = sg_pfn_end + 1;
>  	}
> @@ -503,42 +516,138 @@ static void update_balloon_size_func(struct work_struct *work)
>  		queue_work(system_freezable_wq, work);
>  }
>  
> +static void virtio_balloon_send_free_pages(void *opaque, unsigned long pfn,
> +					   unsigned long nr_pages)
> +{
> +	struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
> +	void *addr = pfn_to_kaddr(pfn);
> +	uint32_t len = nr_pages << PAGE_SHIFT;
> +
> +	send_one_sg(vb, vb->free_page_vq, addr, len, 1);
> +}
> +
> +static void report_free_page_completion(struct virtio_balloon *vb)
> +{
> +	struct virtqueue *vq = vb->free_page_vq;
> +	struct scatterlist sg;
> +	unsigned int len;
> +
> +	sg_init_one(&sg, &vb->report_free_page_signal, sizeof(__virtio32));
> +	while (unlikely(virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL)
> +			== -ENOSPC)) {
> +		virtqueue_kick(vq);
> +		while (!virtqueue_get_buf(vq, &len) &&
> +		       !virtqueue_is_broken(vq))
> +			cpu_relax();
> +	}
> +	virtqueue_kick(vq);
> +}

This unlimited busy waiting needs to go away. A bit of polling might be
ok but even though it'd be better off as a separate driver.  You do not
want to peg CPU for unlimited periods of time for something that's an
optimization.

> +
> +static void report_free_page(struct work_struct *work)
> +{
> +	struct virtio_balloon *vb;
> +
> +	vb = container_of(work, struct virtio_balloon, report_free_page_work);
> +	walk_free_mem_block(vb, 1, &virtio_balloon_send_free_pages);
> +	report_free_page_completion(vb);
> +}
> +
> +static void free_page_request(struct virtqueue *vq)
> +{
> +	struct virtio_balloon *vb = vq->vdev->priv;
> +
> +	queue_work(system_freezable_wq, &vb->report_free_page_work);
> +}
> +
>  static int init_vqs(struct virtio_balloon *vb)
>  {
> -	struct virtqueue *vqs[3];
> -	vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
> -	static const char * const names[] = { "inflate", "deflate", "stats" };
> -	int err, nvqs;
> +	struct virtqueue **vqs;
> +	vq_callback_t **callbacks;
> +	const char **names;
> +	struct scatterlist sg;
> +	int i, nvqs, err = -ENOMEM;
> +
> +	/* Inflateq and deflateq are used unconditionally */
> +	nvqs = 2;
> +	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ))
> +		nvqs++;
> +	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ))
> +		nvqs++;
> +
> +	/* Allocate space for find_vqs parameters */
> +	vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL);
> +	if (!vqs)
> +		goto err_vq;
> +	callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);
> +	if (!callbacks)
> +		goto err_callback;
> +	names = kmalloc_array(nvqs, sizeof(*names), GFP_KERNEL);
> +	if (!names)
> +		goto err_names;
> +
> +	callbacks[0] = balloon_ack;
> +	names[0] = "inflate";
> +	callbacks[1] = balloon_ack;
> +	names[1] = "deflate";
> +
> +	i = 2;
> +	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> +		callbacks[i] = stats_request;
> +		names[i] = "stats";
> +		i++;
> +	}
>  
> -	/*
> -	 * We expect two virtqueues: inflate and deflate, and
> -	 * optionally stat.
> -	 */
> -	nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2;
> -	err = virtio_find_vqs(vb->vdev, nvqs, vqs, callbacks, names, NULL);
> +	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) {
> +		callbacks[i] = free_page_request;
> +		names[i] = "free_page_vq";
> +	}
> +
> +	err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names,
> +					 NULL, NULL);
>  	if (err)
> -		return err;
> +		goto err_find;
>  
>  	vb->inflate_vq = vqs[0];
>  	vb->deflate_vq = vqs[1];
> +	i = 2;
>  	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> -		struct scatterlist sg;
> -		unsigned int num_stats;
> -		vb->stats_vq = vqs[2];
> -
> +		vb->stats_vq = vqs[i++];
>  		/*
>  		 * Prime this virtqueue with one buffer so the hypervisor can
>  		 * use it to signal us later (it can't be broken yet!).
>  		 */
> -		num_stats = update_balloon_stats(vb);
> -
> -		sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
> +		sg_init_one(&sg, vb->stats, sizeof(vb->stats));
>  		if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
>  		    < 0)
>  			BUG();
>  		virtqueue_kick(vb->stats_vq);
>  	}
> +
> +	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) {
> +		vb->free_page_vq = vqs[i];
> +		vb->report_free_page_signal = 0;
> +		sg_init_one(&sg, &vb->report_free_page_signal,
> +			    sizeof(__virtio32));
> +		if (virtqueue_add_outbuf(vb->free_page_vq, &sg, 1, vb,
> +					 GFP_KERNEL) < 0)
> +			dev_warn(&vb->vdev->dev, "%s: add signal buf fail\n",

failed.

And we likely want to fail probe here.

> +				 __func__);
> +		virtqueue_kick(vb->free_page_vq);
> +	}
> +
> +	kfree(names);
> +	kfree(callbacks);
> +	kfree(vqs);
>  	return 0;
> +
> +err_find:
> +	kfree(names);
> +err_names:
> +	kfree(callbacks);
> +err_callback:
> +	kfree(vqs);
> +err_vq:
> +	return err;
>  }
>  
>  #ifdef CONFIG_BALLOON_COMPACTION
> @@ -590,7 +699,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
>  	spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
>  	if (use_sg) {
>  		send_one_sg(vb, vb->inflate_vq, page_address(newpage),
> -			    PAGE_SIZE);
> +			    PAGE_SIZE, 0);
>  	} else {
>  		vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
>  		set_page_pfns(vb, vb->pfns, newpage);
> @@ -600,7 +709,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
>  	balloon_page_delete(page);
>  	if (use_sg) {
>  		send_one_sg(vb, vb->deflate_vq, page_address(page),
> -			    PAGE_SIZE);
> +			    PAGE_SIZE, 0);
>  	} else {
>  		vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
>  		set_page_pfns(vb, vb->pfns, page);
> @@ -667,6 +776,9 @@ static int virtballoon_probe(struct virtio_device *vdev)
>  	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
>  		xb_init(&vb->page_xb);
>  
> +	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ))
> +		INIT_WORK(&vb->report_free_page_work, report_free_page);
> +
>  	vb->nb.notifier_call = virtballoon_oom_notify;
>  	vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
>  	err = register_oom_notifier(&vb->nb);
> @@ -731,6 +843,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
>  	spin_unlock_irq(&vb->stop_update_lock);
>  	cancel_work_sync(&vb->update_balloon_size_work);
>  	cancel_work_sync(&vb->update_balloon_stats_work);
> +	cancel_work_sync(&vb->report_free_page_work);
>  
>  	xb_empty(&vb->page_xb);
>  	remove_common(vb);
> @@ -785,6 +898,7 @@ static unsigned int features[] = {
>  	VIRTIO_BALLOON_F_STATS_VQ,
>  	VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
>  	VIRTIO_BALLOON_F_SG,
> +	VIRTIO_BALLOON_F_FREE_PAGE_VQ,
>  };
>  
>  static struct virtio_driver virtio_balloon_driver = {
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 37780a7..8214f84 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -35,6 +35,7 @@
>  #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 */
> +#define VIRTIO_BALLOON_F_FREE_PAGE_VQ	4 /* Virtqueue to report free pages */
>  
>  /* Size of a PFN in the balloon interface. */
>  #define VIRTIO_BALLOON_PFN_SHIFT 12
> -- 
> 2.7.4
Pankaj Gupta Aug. 3, 2017, 1:05 p.m. UTC | #4 
> 
> On 08/03/2017 04:13 PM, Pankaj Gupta wrote:
> >>
> >> +        /* Allocate space for find_vqs parameters */
> >> +        vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL);
> >> +        if (!vqs)
> >> +                goto err_vq;
> >> +        callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);
> >> +        if (!callbacks)
> >> +                goto err_callback;
> >> +        names = kmalloc_array(nvqs, sizeof(*names), GFP_KERNEL);
> >                      
> >         is size here (integer) intentional?
> 
> 
> Sorry, I didn't get it. Could you please elaborate more?

This is okay

> 
> 
> >
> >> +        if (!names)
> >> +                goto err_names;
> >> +
> >> +        callbacks[0] = balloon_ack;
> >> +        names[0] = "inflate";
> >> +        callbacks[1] = balloon_ack;
> >> +        names[1] = "deflate";
> >> +
> >> +        i = 2;
> >> +        if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> >> +                callbacks[i] = stats_request;
> > just thinking if memory for callbacks[3] & names[3] is allocated?
> 
> 
> Yes, the above kmalloc_array allocated them.

I mean we have created callbacks array for two entries 0,1?

callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);

But we are trying to access location '2' which is third:

         i = 2;
+        if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
+                callbacks[i] = stats_request;      <---- callbacks[2]
+                names[i] = "stats";                <----- names[2]
+                i++;
+        }

I am missing anything obvious here?

> 
> 
> Best,
> Wei
>
Wang, Wei W Aug. 3, 2017, 1:21 p.m. UTC | #5 
On 08/03/2017 09:05 PM, Pankaj Gupta wrote:
>> On 08/03/2017 04:13 PM, Pankaj Gupta wrote:
>>>> +        /* Allocate space for find_vqs parameters */
>>>> +        vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL);
>>>> +        if (!vqs)
>>>> +                goto err_vq;
>>>> +        callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);
>>>> +        if (!callbacks)
>>>> +                goto err_callback;
>>>> +        names = kmalloc_array(nvqs, sizeof(*names), GFP_KERNEL);
>>>                       
>>>          is size here (integer) intentional?
>>
>> Sorry, I didn't get it. Could you please elaborate more?
> This is okay
>
>>
>>>> +        if (!names)
>>>> +                goto err_names;
>>>> +
>>>> +        callbacks[0] = balloon_ack;
>>>> +        names[0] = "inflate";
>>>> +        callbacks[1] = balloon_ack;
>>>> +        names[1] = "deflate";
>>>> +
>>>> +        i = 2;
>>>> +        if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
>>>> +                callbacks[i] = stats_request;
>>> just thinking if memory for callbacks[3] & names[3] is allocated?
>>
>> Yes, the above kmalloc_array allocated them.
> I mean we have created callbacks array for two entries 0,1?
>
> callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);
>
> But we are trying to access location '2' which is third:
>
>           i = 2;
> +        if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> +                callbacks[i] = stats_request;      <---- callbacks[2]
> +                names[i] = "stats";                <----- names[2]
> +                i++;
> +        }
>
> I am missing anything obvious here?


Yes.
if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) is true
nvqs will be 3, that is, callbacks[2] is allocated.

Best,
Wei
kernel test robot Aug. 3, 2017, 4:11 p.m. UTC | #6 
Hi Wei,

[auto build test WARNING on v4.13-rc3]
[also build test WARNING on next-20170803]
[cannot apply to linus/master linux/master mmotm/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Wei-Wang/Virtio-balloon-Enhancement/20170803-223740
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   drivers//virtio/virtio_balloon.c: In function 'tell_host_sgs':
   drivers//virtio/virtio_balloon.c:210:3: error: implicit declaration of function 'pfn_to_kaddr' [-Werror=implicit-function-declaration]
      sg_addr = pfn_to_kaddr(sg_pfn_start);
      ^
   drivers//virtio/virtio_balloon.c:210:11: warning: assignment makes pointer from integer without a cast
      sg_addr = pfn_to_kaddr(sg_pfn_start);
              ^
   drivers//virtio/virtio_balloon.c: In function 'virtio_balloon_send_free_pages':
>> drivers//virtio/virtio_balloon.c:523:15: warning: initialization makes pointer from integer without a cast
     void *addr = pfn_to_kaddr(pfn);
                  ^
   cc1: some warnings being treated as errors

vim +523 drivers//virtio/virtio_balloon.c

   518	
   519	static void virtio_balloon_send_free_pages(void *opaque, unsigned long pfn,
   520						   unsigned long nr_pages)
   521	{
   522		struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
 > 523		void *addr = pfn_to_kaddr(pfn);
   524		uint32_t len = nr_pages << PAGE_SHIFT;
   525	
   526		send_one_sg(vb, vb->free_page_vq, addr, len, 1);
   527	}
   528	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 29aca0c..29c4a61 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -54,11 +54,12 @@  static struct vfsmount *balloon_mnt;
 
 struct virtio_balloon {
 	struct virtio_device *vdev;
-	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
+	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
 
 	/* The balloon servicing is delegated to a freezable workqueue. */
 	struct work_struct update_balloon_stats_work;
 	struct work_struct update_balloon_size_work;
+	struct work_struct report_free_page_work;
 
 	/* Prevent updating balloon when it is being canceled. */
 	spinlock_t stop_update_lock;
@@ -90,6 +91,13 @@  struct virtio_balloon {
 	/* Memory statistics */
 	struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
 
+	/*
+	 * Used by the device and driver to signal each other.
+	 * device->driver: start the free page report.
+	 * driver->device: end the free page report.
+	 */
+	__virtio32 report_free_page_signal;
+
 	/* To register callback in oom notifier call chain */
 	struct notifier_block nb;
 };
@@ -146,7 +154,7 @@  static void set_page_pfns(struct virtio_balloon *vb,
 }
 
 static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
-			void *addr, uint32_t size)
+			void *addr, uint32_t size, bool busywait)
 {
 	struct scatterlist sg;
 	unsigned int len;
@@ -165,7 +173,12 @@  static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
 			cpu_relax();
 	}
 	virtqueue_kick(vq);
-	wait_event(vb->acked, virtqueue_get_buf(vq, &len));
+	if (busywait)
+		while (!virtqueue_get_buf(vq, &len) &&
+		       !virtqueue_is_broken(vq))
+			cpu_relax();
+	else
+		wait_event(vb->acked, virtqueue_get_buf(vq, &len));
 }
 
 /*
@@ -197,11 +210,11 @@  static void tell_host_sgs(struct virtio_balloon *vb,
 		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);
+			send_one_sg(vb, vq, sg_addr, sg_max_len, 0);
 			sg_addr += sg_max_len;
 			sg_len -= sg_max_len;
 		}
-		send_one_sg(vb, vq, sg_addr, sg_len);
+		send_one_sg(vb, vq, sg_addr, sg_len, 0);
 		xb_zero(&vb->page_xb, sg_pfn_start, sg_pfn_end);
 		sg_pfn_start = sg_pfn_end + 1;
 	}
@@ -503,42 +516,138 @@  static void update_balloon_size_func(struct work_struct *work)
 		queue_work(system_freezable_wq, work);
 }
 
+static void virtio_balloon_send_free_pages(void *opaque, unsigned long pfn,
+					   unsigned long nr_pages)
+{
+	struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
+	void *addr = pfn_to_kaddr(pfn);
+	uint32_t len = nr_pages << PAGE_SHIFT;
+
+	send_one_sg(vb, vb->free_page_vq, addr, len, 1);
+}
+
+static void report_free_page_completion(struct virtio_balloon *vb)
+{
+	struct virtqueue *vq = vb->free_page_vq;
+	struct scatterlist sg;
+	unsigned int len;
+
+	sg_init_one(&sg, &vb->report_free_page_signal, sizeof(__virtio32));
+	while (unlikely(virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL)
+			== -ENOSPC)) {
+		virtqueue_kick(vq);
+		while (!virtqueue_get_buf(vq, &len) &&
+		       !virtqueue_is_broken(vq))
+			cpu_relax();
+	}
+	virtqueue_kick(vq);
+}
+
+static void report_free_page(struct work_struct *work)
+{
+	struct virtio_balloon *vb;
+
+	vb = container_of(work, struct virtio_balloon, report_free_page_work);
+	walk_free_mem_block(vb, 1, &virtio_balloon_send_free_pages);
+	report_free_page_completion(vb);
+}
+
+static void free_page_request(struct virtqueue *vq)
+{
+	struct virtio_balloon *vb = vq->vdev->priv;
+
+	queue_work(system_freezable_wq, &vb->report_free_page_work);
+}
+
 static int init_vqs(struct virtio_balloon *vb)
 {
-	struct virtqueue *vqs[3];
-	vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
-	static const char * const names[] = { "inflate", "deflate", "stats" };
-	int err, nvqs;
+	struct virtqueue **vqs;
+	vq_callback_t **callbacks;
+	const char **names;
+	struct scatterlist sg;
+	int i, nvqs, err = -ENOMEM;
+
+	/* Inflateq and deflateq are used unconditionally */
+	nvqs = 2;
+	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ))
+		nvqs++;
+	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ))
+		nvqs++;
+
+	/* Allocate space for find_vqs parameters */
+	vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL);
+	if (!vqs)
+		goto err_vq;
+	callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);
+	if (!callbacks)
+		goto err_callback;
+	names = kmalloc_array(nvqs, sizeof(*names), GFP_KERNEL);
+	if (!names)
+		goto err_names;
+
+	callbacks[0] = balloon_ack;
+	names[0] = "inflate";
+	callbacks[1] = balloon_ack;
+	names[1] = "deflate";
+
+	i = 2;
+	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
+		callbacks[i] = stats_request;
+		names[i] = "stats";
+		i++;
+	}
 
-	/*
-	 * We expect two virtqueues: inflate and deflate, and
-	 * optionally stat.
-	 */
-	nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2;
-	err = virtio_find_vqs(vb->vdev, nvqs, vqs, callbacks, names, NULL);
+	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) {
+		callbacks[i] = free_page_request;
+		names[i] = "free_page_vq";
+	}
+
+	err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names,
+					 NULL, NULL);
 	if (err)
-		return err;
+		goto err_find;
 
 	vb->inflate_vq = vqs[0];
 	vb->deflate_vq = vqs[1];
+	i = 2;
 	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
-		struct scatterlist sg;
-		unsigned int num_stats;
-		vb->stats_vq = vqs[2];
-
+		vb->stats_vq = vqs[i++];
 		/*
 		 * Prime this virtqueue with one buffer so the hypervisor can
 		 * use it to signal us later (it can't be broken yet!).
 		 */
-		num_stats = update_balloon_stats(vb);
-
-		sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
+		sg_init_one(&sg, vb->stats, sizeof(vb->stats));
 		if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
 		    < 0)
 			BUG();
 		virtqueue_kick(vb->stats_vq);
 	}
+
+	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) {
+		vb->free_page_vq = vqs[i];
+		vb->report_free_page_signal = 0;
+		sg_init_one(&sg, &vb->report_free_page_signal,
+			    sizeof(__virtio32));
+		if (virtqueue_add_outbuf(vb->free_page_vq, &sg, 1, vb,
+					 GFP_KERNEL) < 0)
+			dev_warn(&vb->vdev->dev, "%s: add signal buf fail\n",
+				 __func__);
+		virtqueue_kick(vb->free_page_vq);
+	}
+
+	kfree(names);
+	kfree(callbacks);
+	kfree(vqs);
 	return 0;
+
+err_find:
+	kfree(names);
+err_names:
+	kfree(callbacks);
+err_callback:
+	kfree(vqs);
+err_vq:
+	return err;
 }
 
 #ifdef CONFIG_BALLOON_COMPACTION
@@ -590,7 +699,7 @@  static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
 	spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
 	if (use_sg) {
 		send_one_sg(vb, vb->inflate_vq, page_address(newpage),
-			    PAGE_SIZE);
+			    PAGE_SIZE, 0);
 	} else {
 		vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
 		set_page_pfns(vb, vb->pfns, newpage);
@@ -600,7 +709,7 @@  static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
 	balloon_page_delete(page);
 	if (use_sg) {
 		send_one_sg(vb, vb->deflate_vq, page_address(page),
-			    PAGE_SIZE);
+			    PAGE_SIZE, 0);
 	} else {
 		vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
 		set_page_pfns(vb, vb->pfns, page);
@@ -667,6 +776,9 @@  static int virtballoon_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
 		xb_init(&vb->page_xb);
 
+	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ))
+		INIT_WORK(&vb->report_free_page_work, report_free_page);
+
 	vb->nb.notifier_call = virtballoon_oom_notify;
 	vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
 	err = register_oom_notifier(&vb->nb);
@@ -731,6 +843,7 @@  static void virtballoon_remove(struct virtio_device *vdev)
 	spin_unlock_irq(&vb->stop_update_lock);
 	cancel_work_sync(&vb->update_balloon_size_work);
 	cancel_work_sync(&vb->update_balloon_stats_work);
+	cancel_work_sync(&vb->report_free_page_work);
 
 	xb_empty(&vb->page_xb);
 	remove_common(vb);
@@ -785,6 +898,7 @@  static unsigned int features[] = {
 	VIRTIO_BALLOON_F_STATS_VQ,
 	VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
 	VIRTIO_BALLOON_F_SG,
+	VIRTIO_BALLOON_F_FREE_PAGE_VQ,
 };
 
 static struct virtio_driver virtio_balloon_driver = {
diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index 37780a7..8214f84 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -35,6 +35,7 @@ 
 #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 */
+#define VIRTIO_BALLOON_F_FREE_PAGE_VQ	4 /* Virtqueue to report free pages */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12