[PATCH v3 0/3] virtio-balloon: tweak config_changed

[PATCH v3 0/3] virtio-balloon: tweak config_changed

[Wei Wang]

Since virtio-ccw doesn't work with accessing to the config space
inside an interrupt context, this patch series avoids that issue by
moving the config register accesses to the related workqueue contexts.

v2->v3 ChangeLog:
    - rename cmd_id_received to cmd_id_received_cache, and have call sites
      read the latest value via virtio_balloon_cmd_id_received. (Still
      kept Cornelia and Halil's reviewed-by as it's a minor change)
    - remove zeroing vb->num_free_page_blocks in probe since vb is
      allocated via kzalloc.
v1->v2 ChangeLog:
    - add config_read_bitmap to indicate to the workqueue callbacks about
      the necessity of reading the related config fields.

Wei Wang (3):
  virtio-balloon: tweak config_changed implementation
  virtio-balloon: improve update_balloon_size_func
  virtio_balloon: remove the unnecessary 0-initialization

 drivers/virtio/virtio_balloon.c | 104 ++++++++++++++++++++++++++--------------
 1 file changed, 69 insertions(+), 35 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Wei Wang]

virtio-ccw has deadlock issues with reading the config space inside the
interrupt context, so we tweak the virtballoon_changed implementation
by moving the config read operations into the related workqueue contexts.
The config_read_bitmap is used as a flag to the workqueue callbacks
about the related config fields that need to be read.

The cmd_id_received is also renamed to cmd_id_received_cache, and
the value should be obtained via virtio_balloon_cmd_id_received.

Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
---
 drivers/virtio/virtio_balloon.c | 98 +++++++++++++++++++++++++++--------------
 1 file changed, 65 insertions(+), 33 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 728ecd1..fb12fe2 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -61,6 +61,10 @@ enum virtio_balloon_vq {
 	VIRTIO_BALLOON_VQ_MAX
 };
 
+enum virtio_balloon_config_read {
+	VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
+};
+
 struct virtio_balloon {
 	struct virtio_device *vdev;
 	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
@@ -77,14 +81,20 @@ struct virtio_balloon {
 	/* Prevent updating balloon when it is being canceled. */
 	spinlock_t stop_update_lock;
 	bool stop_update;
+	/* Bitmap to indicate if reading the related config fields are needed */
+	unsigned long config_read_bitmap;
 
 	/* The list of allocated free pages, waiting to be given back to mm */
 	struct list_head free_page_list;
 	spinlock_t free_page_list_lock;
 	/* The number of free page blocks on the above list */
 	unsigned long num_free_page_blocks;
-	/* The cmd id received from host */
-	u32 cmd_id_received;
+	/*
+	 * The cmd id received from host.
+	 * Read it via virtio_balloon_cmd_id_received to get the latest value
+	 * sent from host.
+	 */
+	u32 cmd_id_received_cache;
 	/* The cmd id that is actively in use */
 	__virtio32 cmd_id_active;
 	/* Buffer to store the stop sign */
@@ -390,37 +400,31 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
 	return num_returned;
 }
 
+static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
+{
+	if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
+		return;
+
+	/* No need to queue the work if the bit was already set. */
+	if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
+			     &vb->config_read_bitmap))
+		return;
+
+	queue_work(vb->balloon_wq, &vb->report_free_page_work);
+}
+
 static void virtballoon_changed(struct virtio_device *vdev)
 {
 	struct virtio_balloon *vb = vdev->priv;
 	unsigned long flags;
-	s64 diff = towards_target(vb);
-
-	if (diff) {
-		spin_lock_irqsave(&vb->stop_update_lock, flags);
-		if (!vb->stop_update)
-			queue_work(system_freezable_wq,
-				   &vb->update_balloon_size_work);
-		spin_unlock_irqrestore(&vb->stop_update_lock, flags);
-	}
 
-	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
-		virtio_cread(vdev, struct virtio_balloon_config,
-			     free_page_report_cmd_id, &vb->cmd_id_received);
-		if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
-			/* Pass ULONG_MAX to give back all the free pages */
-			return_free_pages_to_mm(vb, ULONG_MAX);
-		} else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
-			   vb->cmd_id_received !=
-			   virtio32_to_cpu(vdev, vb->cmd_id_active)) {
-			spin_lock_irqsave(&vb->stop_update_lock, flags);
-			if (!vb->stop_update) {
-				queue_work(vb->balloon_wq,
-					   &vb->report_free_page_work);
-			}
-			spin_unlock_irqrestore(&vb->stop_update_lock, flags);
-		}
+	spin_lock_irqsave(&vb->stop_update_lock, flags);
+	if (!vb->stop_update) {
+		queue_work(system_freezable_wq,
+			   &vb->update_balloon_size_work);
+		virtio_balloon_queue_free_page_work(vb);
 	}
+	spin_unlock_irqrestore(&vb->stop_update_lock, flags);
 }
 
 static void update_balloon_size(struct virtio_balloon *vb)
@@ -527,6 +531,17 @@ static int init_vqs(struct virtio_balloon *vb)
 	return 0;
 }
 
+static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb)
+{
+	if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
+			       &vb->config_read_bitmap))
+		virtio_cread(vb->vdev, struct virtio_balloon_config,
+			     free_page_report_cmd_id,
+			     &vb->cmd_id_received_cache);
+
+	return vb->cmd_id_received_cache;
+}
+
 static int send_cmd_id_start(struct virtio_balloon *vb)
 {
 	struct scatterlist sg;
@@ -537,7 +552,8 @@ static int send_cmd_id_start(struct virtio_balloon *vb)
 	while (virtqueue_get_buf(vq, &unused))
 		;
 
-	vb->cmd_id_active = cpu_to_virtio32(vb->vdev, vb->cmd_id_received);
+	vb->cmd_id_active = virtio32_to_cpu(vb->vdev,
+					virtio_balloon_cmd_id_received(vb));
 	sg_init_one(&sg, &vb->cmd_id_active, sizeof(vb->cmd_id_active));
 	err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active, GFP_KERNEL);
 	if (!err)
@@ -620,7 +636,8 @@ static int send_free_pages(struct virtio_balloon *vb)
 		 * stop the reporting.
 		 */
 		cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
-		if (cmd_id_active != vb->cmd_id_received)
+		if (unlikely(cmd_id_active !=
+			     virtio_balloon_cmd_id_received(vb)))
 			break;
 
 		/*
@@ -637,11 +654,9 @@ static int send_free_pages(struct virtio_balloon *vb)
 	return 0;
 }
 
-static void report_free_page_func(struct work_struct *work)
+static void virtio_balloon_report_free_page(struct virtio_balloon *vb)
 {
 	int err;
-	struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
-						 report_free_page_work);
 	struct device *dev = &vb->vdev->dev;
 
 	/* Start by sending the received cmd id to host with an outbuf. */
@@ -659,6 +674,23 @@ static void report_free_page_func(struct work_struct *work)
 		dev_err(dev, "Failed to send a stop id, err = %d\n", err);
 }
 
+static void report_free_page_func(struct work_struct *work)
+{
+	struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
+						 report_free_page_work);
+	u32 cmd_id_received;
+
+	cmd_id_received = virtio_balloon_cmd_id_received(vb);
+	if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
+		/* Pass ULONG_MAX to give back all the free pages */
+		return_free_pages_to_mm(vb, ULONG_MAX);
+	} else if (cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
+		   cmd_id_received !=
+		   virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) {
+		virtio_balloon_report_free_page(vb);
+	}
+}
+
 #ifdef CONFIG_BALLOON_COMPACTION
 /*
  * virtballoon_migratepage - perform the balloon page migration on behalf of
@@ -885,7 +917,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
 			goto out_del_vqs;
 		}
 		INIT_WORK(&vb->report_free_page_work, report_free_page_func);
-		vb->cmd_id_received = VIRTIO_BALLOON_CMD_ID_STOP;
+		vb->cmd_id_received_cache = VIRTIO_BALLOON_CMD_ID_STOP;
 		vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
 						  VIRTIO_BALLOON_CMD_ID_STOP);
 		vb->cmd_id_stop = cpu_to_virtio32(vb->vdev,
-- 
2.7.4

[PATCH v3 2/3] virtio-balloon: improve update_balloon_size_func

[Wei Wang]

There is no need to update the balloon actual register when there is no
ballooning request. This patch avoids update_balloon_size when diff is 0.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
---
 drivers/virtio/virtio_balloon.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index fb12fe2..e33dc8e 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -457,9 +457,12 @@ static void update_balloon_size_func(struct work_struct *work)
 			  update_balloon_size_work);
 	diff = towards_target(vb);
 
+	if (!diff)
+		return;
+
 	if (diff > 0)
 		diff -= fill_balloon(vb, diff);
-	else if (diff < 0)
+	else
 		diff += leak_balloon(vb, -diff);
 	update_balloon_size(vb);
 
-- 
2.7.4

[PATCH v3 3/3] virtio_balloon: remove the unnecessary 0-initialization

[Wei Wang]

We've changed to kzalloc the vb struct, so no need to 0-initialize
this field one more time.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 drivers/virtio/virtio_balloon.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index e33dc8e..f19061b 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -925,7 +925,6 @@ static int virtballoon_probe(struct virtio_device *vdev)
 						  VIRTIO_BALLOON_CMD_ID_STOP);
 		vb->cmd_id_stop = cpu_to_virtio32(vb->vdev,
 						  VIRTIO_BALLOON_CMD_ID_STOP);
-		vb->num_free_page_blocks = 0;
 		spin_lock_init(&vb->free_page_list_lock);
 		INIT_LIST_HEAD(&vb->free_page_list);
 		if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
-- 
2.7.4

Re: [PATCH v3 0/3] virtio-balloon: tweak config_changed

[Christian Borntraeger]

Can you please cc stable? Right now 4.20 does not work under KVM.



On 07.01.2019 08:01, Wei Wang wrote:
> Since virtio-ccw doesn't work with accessing to the config space
> inside an interrupt context, this patch series avoids that issue by
> moving the config register accesses to the related workqueue contexts.
> 
> v2->v3 ChangeLog:
>     - rename cmd_id_received to cmd_id_received_cache, and have call sites
>       read the latest value via virtio_balloon_cmd_id_received. (Still
>       kept Cornelia and Halil's reviewed-by as it's a minor change)
>     - remove zeroing vb->num_free_page_blocks in probe since vb is
>       allocated via kzalloc.
> v1->v2 ChangeLog:
>     - add config_read_bitmap to indicate to the workqueue callbacks about
>       the necessity of reading the related config fields.
> 
> Wei Wang (3):
>   virtio-balloon: tweak config_changed implementation
>   virtio-balloon: improve update_balloon_size_func
>   virtio_balloon: remove the unnecessary 0-initialization
> 
>  drivers/virtio/virtio_balloon.c | 104 ++++++++++++++++++++++++++--------------
>  1 file changed, 69 insertions(+), 35 deletions(-)
> 

Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Michael S. Tsirkin]

On Mon, Jan 07, 2019 at 03:01:04PM +0800, Wei Wang wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
> 
> The cmd_id_received is also renamed to cmd_id_received_cache, and
> the value should be obtained via virtio_balloon_cmd_id_received.
> 
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> ---
>  drivers/virtio/virtio_balloon.c | 98 +++++++++++++++++++++++++++--------------
>  1 file changed, 65 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 728ecd1..fb12fe2 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -61,6 +61,10 @@ enum virtio_balloon_vq {
>  	VIRTIO_BALLOON_VQ_MAX
>  };
>  
> +enum virtio_balloon_config_read {
> +	VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
> +};
> +
>  struct virtio_balloon {
>  	struct virtio_device *vdev;
>  	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> @@ -77,14 +81,20 @@ struct virtio_balloon {
>  	/* Prevent updating balloon when it is being canceled. */
>  	spinlock_t stop_update_lock;
>  	bool stop_update;
> +	/* Bitmap to indicate if reading the related config fields are needed */
> +	unsigned long config_read_bitmap;
>  
>  	/* The list of allocated free pages, waiting to be given back to mm */
>  	struct list_head free_page_list;
>  	spinlock_t free_page_list_lock;
>  	/* The number of free page blocks on the above list */
>  	unsigned long num_free_page_blocks;
> -	/* The cmd id received from host */
> -	u32 cmd_id_received;
> +	/*
> +	 * The cmd id received from host.
> +	 * Read it via virtio_balloon_cmd_id_received to get the latest value
> +	 * sent from host.
> +	 */
> +	u32 cmd_id_received_cache;
>  	/* The cmd id that is actively in use */
>  	__virtio32 cmd_id_active;
>  	/* Buffer to store the stop sign */
> @@ -390,37 +400,31 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
>  	return num_returned;
>  }
>  
> +static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
> +{
> +	if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> +		return;
> +
> +	/* No need to queue the work if the bit was already set. */
> +	if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> +			     &vb->config_read_bitmap))
> +		return;
> +
> +	queue_work(vb->balloon_wq, &vb->report_free_page_work);
> +}
> +
>  static void virtballoon_changed(struct virtio_device *vdev)
>  {
>  	struct virtio_balloon *vb = vdev->priv;
>  	unsigned long flags;
> -	s64 diff = towards_target(vb);
> -
> -	if (diff) {
> -		spin_lock_irqsave(&vb->stop_update_lock, flags);
> -		if (!vb->stop_update)
> -			queue_work(system_freezable_wq,
> -				   &vb->update_balloon_size_work);
> -		spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> -	}
>  
> -	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> -		virtio_cread(vdev, struct virtio_balloon_config,
> -			     free_page_report_cmd_id, &vb->cmd_id_received);
> -		if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> -			/* Pass ULONG_MAX to give back all the free pages */
> -			return_free_pages_to_mm(vb, ULONG_MAX);
> -		} else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> -			   vb->cmd_id_received !=
> -			   virtio32_to_cpu(vdev, vb->cmd_id_active)) {
> -			spin_lock_irqsave(&vb->stop_update_lock, flags);
> -			if (!vb->stop_update) {
> -				queue_work(vb->balloon_wq,
> -					   &vb->report_free_page_work);
> -			}
> -			spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> -		}
> +	spin_lock_irqsave(&vb->stop_update_lock, flags);
> +	if (!vb->stop_update) {
> +		queue_work(system_freezable_wq,
> +			   &vb->update_balloon_size_work);
> +		virtio_balloon_queue_free_page_work(vb);
>  	}
> +	spin_unlock_irqrestore(&vb->stop_update_lock, flags);
>  }
>  
>  static void update_balloon_size(struct virtio_balloon *vb)
> @@ -527,6 +531,17 @@ static int init_vqs(struct virtio_balloon *vb)
>  	return 0;
>  }
>  
> +static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb)
> +{
> +	if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> +			       &vb->config_read_bitmap))
> +		virtio_cread(vb->vdev, struct virtio_balloon_config,
> +			     free_page_report_cmd_id,
> +			     &vb->cmd_id_received_cache);
> +
> +	return vb->cmd_id_received_cache;
> +}
> +
>  static int send_cmd_id_start(struct virtio_balloon *vb)
>  {
>  	struct scatterlist sg;
> @@ -537,7 +552,8 @@ static int send_cmd_id_start(struct virtio_balloon *vb)
>  	while (virtqueue_get_buf(vq, &unused))
>  		;
>  
> -	vb->cmd_id_active = cpu_to_virtio32(vb->vdev, vb->cmd_id_received);
> +	vb->cmd_id_active = virtio32_to_cpu(vb->vdev,
> +					virtio_balloon_cmd_id_received(vb));


You switches cpu_to_virtio32 to over to virtio32_to_cpu.
That will produce a sparse warning I think.
Pls always run the static checker when you submit patches.


>  	sg_init_one(&sg, &vb->cmd_id_active, sizeof(vb->cmd_id_active));
>  	err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active, GFP_KERNEL);
>  	if (!err)
> @@ -620,7 +636,8 @@ static int send_free_pages(struct virtio_balloon *vb)
>  		 * stop the reporting.
>  		 */
>  		cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
> -		if (cmd_id_active != vb->cmd_id_received)
> +		if (unlikely(cmd_id_active !=
> +			     virtio_balloon_cmd_id_received(vb)))
>  			break;
>  
>  		/*
> @@ -637,11 +654,9 @@ static int send_free_pages(struct virtio_balloon *vb)
>  	return 0;
>  }
>  
> -static void report_free_page_func(struct work_struct *work)
> +static void virtio_balloon_report_free_page(struct virtio_balloon *vb)
>  {
>  	int err;
> -	struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> -						 report_free_page_work);
>  	struct device *dev = &vb->vdev->dev;
>  
>  	/* Start by sending the received cmd id to host with an outbuf. */
> @@ -659,6 +674,23 @@ static void report_free_page_func(struct work_struct *work)
>  		dev_err(dev, "Failed to send a stop id, err = %d\n", err);
>  }
>  
> +static void report_free_page_func(struct work_struct *work)
> +{
> +	struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> +						 report_free_page_work);
> +	u32 cmd_id_received;
> +
> +	cmd_id_received = virtio_balloon_cmd_id_received(vb);
> +	if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> +		/* Pass ULONG_MAX to give back all the free pages */
> +		return_free_pages_to_mm(vb, ULONG_MAX);
> +	} else if (cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> +		   cmd_id_received !=
> +		   virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) {
> +		virtio_balloon_report_free_page(vb);
> +	}
> +}
> +
>  #ifdef CONFIG_BALLOON_COMPACTION
>  /*
>   * virtballoon_migratepage - perform the balloon page migration on behalf of
> @@ -885,7 +917,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
>  			goto out_del_vqs;
>  		}
>  		INIT_WORK(&vb->report_free_page_work, report_free_page_func);
> -		vb->cmd_id_received = VIRTIO_BALLOON_CMD_ID_STOP;
> +		vb->cmd_id_received_cache = VIRTIO_BALLOON_CMD_ID_STOP;
>  		vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
>  						  VIRTIO_BALLOON_CMD_ID_STOP);
>  		vb->cmd_id_stop = cpu_to_virtio32(vb->vdev,
> -- 
> 2.7.4

Re: [PATCH v3 0/3] virtio-balloon: tweak config_changed

[Michael S. Tsirkin]

On Mon, Jan 07, 2019 at 03:01:03PM +0800, Wei Wang wrote:
> Since virtio-ccw doesn't work with accessing to the config space
> inside an interrupt context, this patch series avoids that issue by
> moving the config register accesses to the related workqueue contexts.

So is this enough to get ccw going again or do we also need
the patches that deal with NULL VQ names?

> v2->v3 ChangeLog:
>     - rename cmd_id_received to cmd_id_received_cache, and have call sites
>       read the latest value via virtio_balloon_cmd_id_received. (Still
>       kept Cornelia and Halil's reviewed-by as it's a minor change)
>     - remove zeroing vb->num_free_page_blocks in probe since vb is
>       allocated via kzalloc.
> v1->v2 ChangeLog:
>     - add config_read_bitmap to indicate to the workqueue callbacks about
>       the necessity of reading the related config fields.
> 
> Wei Wang (3):
>   virtio-balloon: tweak config_changed implementation
>   virtio-balloon: improve update_balloon_size_func
>   virtio_balloon: remove the unnecessary 0-initialization
> 
>  drivers/virtio/virtio_balloon.c | 104 ++++++++++++++++++++++++++--------------
>  1 file changed, 69 insertions(+), 35 deletions(-)
> 
> -- 
> 2.7.4

Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Michael S. Tsirkin]

On Mon, Jan 07, 2019 at 03:01:04PM +0800, Wei Wang wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
> 
> The cmd_id_received is also renamed to cmd_id_received_cache, and
> the value should be obtained via virtio_balloon_cmd_id_received.
> 
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

Please include a Fixes tag to list the problematic commit.
See Documentation/process/submitting-patches.rst for the appropriate format.

> ---
>  drivers/virtio/virtio_balloon.c | 98 +++++++++++++++++++++++++++--------------
>  1 file changed, 65 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 728ecd1..fb12fe2 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -61,6 +61,10 @@ enum virtio_balloon_vq {
>  	VIRTIO_BALLOON_VQ_MAX
>  };
>  
> +enum virtio_balloon_config_read {
> +	VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
> +};
> +
>  struct virtio_balloon {
>  	struct virtio_device *vdev;
>  	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> @@ -77,14 +81,20 @@ struct virtio_balloon {
>  	/* Prevent updating balloon when it is being canceled. */
>  	spinlock_t stop_update_lock;
>  	bool stop_update;
> +	/* Bitmap to indicate if reading the related config fields are needed */
> +	unsigned long config_read_bitmap;
>  
>  	/* The list of allocated free pages, waiting to be given back to mm */
>  	struct list_head free_page_list;
>  	spinlock_t free_page_list_lock;
>  	/* The number of free page blocks on the above list */
>  	unsigned long num_free_page_blocks;
> -	/* The cmd id received from host */
> -	u32 cmd_id_received;
> +	/*
> +	 * The cmd id received from host.
> +	 * Read it via virtio_balloon_cmd_id_received to get the latest value
> +	 * sent from host.
> +	 */
> +	u32 cmd_id_received_cache;
>  	/* The cmd id that is actively in use */
>  	__virtio32 cmd_id_active;
>  	/* Buffer to store the stop sign */
> @@ -390,37 +400,31 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
>  	return num_returned;
>  }
>  
> +static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
> +{
> +	if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> +		return;
> +
> +	/* No need to queue the work if the bit was already set. */
> +	if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> +			     &vb->config_read_bitmap))
> +		return;
> +
> +	queue_work(vb->balloon_wq, &vb->report_free_page_work);
> +}
> +
>  static void virtballoon_changed(struct virtio_device *vdev)
>  {
>  	struct virtio_balloon *vb = vdev->priv;
>  	unsigned long flags;
> -	s64 diff = towards_target(vb);
> -
> -	if (diff) {
> -		spin_lock_irqsave(&vb->stop_update_lock, flags);
> -		if (!vb->stop_update)
> -			queue_work(system_freezable_wq,
> -				   &vb->update_balloon_size_work);
> -		spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> -	}
>  
> -	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> -		virtio_cread(vdev, struct virtio_balloon_config,
> -			     free_page_report_cmd_id, &vb->cmd_id_received);
> -		if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> -			/* Pass ULONG_MAX to give back all the free pages */
> -			return_free_pages_to_mm(vb, ULONG_MAX);
> -		} else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> -			   vb->cmd_id_received !=
> -			   virtio32_to_cpu(vdev, vb->cmd_id_active)) {
> -			spin_lock_irqsave(&vb->stop_update_lock, flags);
> -			if (!vb->stop_update) {
> -				queue_work(vb->balloon_wq,
> -					   &vb->report_free_page_work);
> -			}
> -			spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> -		}
> +	spin_lock_irqsave(&vb->stop_update_lock, flags);
> +	if (!vb->stop_update) {
> +		queue_work(system_freezable_wq,
> +			   &vb->update_balloon_size_work);
> +		virtio_balloon_queue_free_page_work(vb);
>  	}
> +	spin_unlock_irqrestore(&vb->stop_update_lock, flags);
>  }
>  
>  static void update_balloon_size(struct virtio_balloon *vb)
> @@ -527,6 +531,17 @@ static int init_vqs(struct virtio_balloon *vb)
>  	return 0;
>  }
>  
> +static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb)
> +{
> +	if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> +			       &vb->config_read_bitmap))
> +		virtio_cread(vb->vdev, struct virtio_balloon_config,
> +			     free_page_report_cmd_id,
> +			     &vb->cmd_id_received_cache);
> +
> +	return vb->cmd_id_received_cache;
> +}
> +
>  static int send_cmd_id_start(struct virtio_balloon *vb)
>  {
>  	struct scatterlist sg;
> @@ -537,7 +552,8 @@ static int send_cmd_id_start(struct virtio_balloon *vb)
>  	while (virtqueue_get_buf(vq, &unused))
>  		;
>  
> -	vb->cmd_id_active = cpu_to_virtio32(vb->vdev, vb->cmd_id_received);
> +	vb->cmd_id_active = virtio32_to_cpu(vb->vdev,
> +					virtio_balloon_cmd_id_received(vb));
>  	sg_init_one(&sg, &vb->cmd_id_active, sizeof(vb->cmd_id_active));
>  	err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active, GFP_KERNEL);
>  	if (!err)
> @@ -620,7 +636,8 @@ static int send_free_pages(struct virtio_balloon *vb)
>  		 * stop the reporting.
>  		 */
>  		cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
> -		if (cmd_id_active != vb->cmd_id_received)
> +		if (unlikely(cmd_id_active !=
> +			     virtio_balloon_cmd_id_received(vb)))
>  			break;
>  
>  		/*
> @@ -637,11 +654,9 @@ static int send_free_pages(struct virtio_balloon *vb)
>  	return 0;
>  }
>  
> -static void report_free_page_func(struct work_struct *work)
> +static void virtio_balloon_report_free_page(struct virtio_balloon *vb)
>  {
>  	int err;
> -	struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> -						 report_free_page_work);
>  	struct device *dev = &vb->vdev->dev;
>  
>  	/* Start by sending the received cmd id to host with an outbuf. */
> @@ -659,6 +674,23 @@ static void report_free_page_func(struct work_struct *work)
>  		dev_err(dev, "Failed to send a stop id, err = %d\n", err);
>  }
>  
> +static void report_free_page_func(struct work_struct *work)
> +{
> +	struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> +						 report_free_page_work);
> +	u32 cmd_id_received;
> +
> +	cmd_id_received = virtio_balloon_cmd_id_received(vb);
> +	if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> +		/* Pass ULONG_MAX to give back all the free pages */
> +		return_free_pages_to_mm(vb, ULONG_MAX);
> +	} else if (cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> +		   cmd_id_received !=
> +		   virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) {
> +		virtio_balloon_report_free_page(vb);
> +	}
> +}
> +
>  #ifdef CONFIG_BALLOON_COMPACTION
>  /*
>   * virtballoon_migratepage - perform the balloon page migration on behalf of
> @@ -885,7 +917,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
>  			goto out_del_vqs;
>  		}
>  		INIT_WORK(&vb->report_free_page_work, report_free_page_func);
> -		vb->cmd_id_received = VIRTIO_BALLOON_CMD_ID_STOP;
> +		vb->cmd_id_received_cache = VIRTIO_BALLOON_CMD_ID_STOP;
>  		vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
>  						  VIRTIO_BALLOON_CMD_ID_STOP);
>  		vb->cmd_id_stop = cpu_to_virtio32(vb->vdev,
> -- 
> 2.7.4

Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Christian Borntraeger]

On 07.01.2019 08:01, Wei Wang wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
> 
> The cmd_id_received is also renamed to cmd_id_received_cache, and
> the value should be obtained via virtio_balloon_cmd_id_received.
> 
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

Together with 
  virtio_pci: use queue idx instead of array idx to set up the vq
  virtio: don't allocate vqs when names[i] = NULL

Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>

as already said, would be good to add cc stable (and a Fixes: tag)


> ---
>  drivers/virtio/virtio_balloon.c | 98 +++++++++++++++++++++++++++--------------
>  1 file changed, 65 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 728ecd1..fb12fe2 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -61,6 +61,10 @@ enum virtio_balloon_vq {
>  	VIRTIO_BALLOON_VQ_MAX
>  };
>  
> +enum virtio_balloon_config_read {
> +	VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
> +};
> +
>  struct virtio_balloon {
>  	struct virtio_device *vdev;
>  	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> @@ -77,14 +81,20 @@ struct virtio_balloon {
>  	/* Prevent updating balloon when it is being canceled. */
>  	spinlock_t stop_update_lock;
>  	bool stop_update;
> +	/* Bitmap to indicate if reading the related config fields are needed */
> +	unsigned long config_read_bitmap;
>  
>  	/* The list of allocated free pages, waiting to be given back to mm */
>  	struct list_head free_page_list;
>  	spinlock_t free_page_list_lock;
>  	/* The number of free page blocks on the above list */
>  	unsigned long num_free_page_blocks;
> -	/* The cmd id received from host */
> -	u32 cmd_id_received;
> +	/*
> +	 * The cmd id received from host.
> +	 * Read it via virtio_balloon_cmd_id_received to get the latest value
> +	 * sent from host.
> +	 */
> +	u32 cmd_id_received_cache;
>  	/* The cmd id that is actively in use */
>  	__virtio32 cmd_id_active;
>  	/* Buffer to store the stop sign */
> @@ -390,37 +400,31 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
>  	return num_returned;
>  }
>  
> +static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
> +{
> +	if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> +		return;
> +
> +	/* No need to queue the work if the bit was already set. */
> +	if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> +			     &vb->config_read_bitmap))
> +		return;
> +
> +	queue_work(vb->balloon_wq, &vb->report_free_page_work);
> +}
> +
>  static void virtballoon_changed(struct virtio_device *vdev)
>  {
>  	struct virtio_balloon *vb = vdev->priv;
>  	unsigned long flags;
> -	s64 diff = towards_target(vb);
> -
> -	if (diff) {
> -		spin_lock_irqsave(&vb->stop_update_lock, flags);
> -		if (!vb->stop_update)
> -			queue_work(system_freezable_wq,
> -				   &vb->update_balloon_size_work);
> -		spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> -	}
>  
> -	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> -		virtio_cread(vdev, struct virtio_balloon_config,
> -			     free_page_report_cmd_id, &vb->cmd_id_received);
> -		if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> -			/* Pass ULONG_MAX to give back all the free pages */
> -			return_free_pages_to_mm(vb, ULONG_MAX);
> -		} else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> -			   vb->cmd_id_received !=
> -			   virtio32_to_cpu(vdev, vb->cmd_id_active)) {
> -			spin_lock_irqsave(&vb->stop_update_lock, flags);
> -			if (!vb->stop_update) {
> -				queue_work(vb->balloon_wq,
> -					   &vb->report_free_page_work);
> -			}
> -			spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> -		}
> +	spin_lock_irqsave(&vb->stop_update_lock, flags);
> +	if (!vb->stop_update) {
> +		queue_work(system_freezable_wq,
> +			   &vb->update_balloon_size_work);
> +		virtio_balloon_queue_free_page_work(vb);
>  	}
> +	spin_unlock_irqrestore(&vb->stop_update_lock, flags);
>  }
>  
>  static void update_balloon_size(struct virtio_balloon *vb)
> @@ -527,6 +531,17 @@ static int init_vqs(struct virtio_balloon *vb)
>  	return 0;
>  }
>  
> +static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb)
> +{
> +	if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> +			       &vb->config_read_bitmap))
> +		virtio_cread(vb->vdev, struct virtio_balloon_config,
> +			     free_page_report_cmd_id,
> +			     &vb->cmd_id_received_cache);
> +
> +	return vb->cmd_id_received_cache;
> +}
> +
>  static int send_cmd_id_start(struct virtio_balloon *vb)
>  {
>  	struct scatterlist sg;
> @@ -537,7 +552,8 @@ static int send_cmd_id_start(struct virtio_balloon *vb)
>  	while (virtqueue_get_buf(vq, &unused))
>  		;
>  
> -	vb->cmd_id_active = cpu_to_virtio32(vb->vdev, vb->cmd_id_received);
> +	vb->cmd_id_active = virtio32_to_cpu(vb->vdev,
> +					virtio_balloon_cmd_id_received(vb));
>  	sg_init_one(&sg, &vb->cmd_id_active, sizeof(vb->cmd_id_active));
>  	err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active, GFP_KERNEL);
>  	if (!err)
> @@ -620,7 +636,8 @@ static int send_free_pages(struct virtio_balloon *vb)
>  		 * stop the reporting.
>  		 */
>  		cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
> -		if (cmd_id_active != vb->cmd_id_received)
> +		if (unlikely(cmd_id_active !=
> +			     virtio_balloon_cmd_id_received(vb)))
>  			break;
>  
>  		/*
> @@ -637,11 +654,9 @@ static int send_free_pages(struct virtio_balloon *vb)
>  	return 0;
>  }
>  
> -static void report_free_page_func(struct work_struct *work)
> +static void virtio_balloon_report_free_page(struct virtio_balloon *vb)
>  {
>  	int err;
> -	struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> -						 report_free_page_work);
>  	struct device *dev = &vb->vdev->dev;
>  
>  	/* Start by sending the received cmd id to host with an outbuf. */
> @@ -659,6 +674,23 @@ static void report_free_page_func(struct work_struct *work)
>  		dev_err(dev, "Failed to send a stop id, err = %d\n", err);
>  }
>  
> +static void report_free_page_func(struct work_struct *work)
> +{
> +	struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> +						 report_free_page_work);
> +	u32 cmd_id_received;
> +
> +	cmd_id_received = virtio_balloon_cmd_id_received(vb);
> +	if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> +		/* Pass ULONG_MAX to give back all the free pages */
> +		return_free_pages_to_mm(vb, ULONG_MAX);
> +	} else if (cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> +		   cmd_id_received !=
> +		   virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) {
> +		virtio_balloon_report_free_page(vb);
> +	}
> +}
> +
>  #ifdef CONFIG_BALLOON_COMPACTION
>  /*
>   * virtballoon_migratepage - perform the balloon page migration on behalf of
> @@ -885,7 +917,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
>  			goto out_del_vqs;
>  		}
>  		INIT_WORK(&vb->report_free_page_work, report_free_page_func);
> -		vb->cmd_id_received = VIRTIO_BALLOON_CMD_ID_STOP;
> +		vb->cmd_id_received_cache = VIRTIO_BALLOON_CMD_ID_STOP;
>  		vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
>  						  VIRTIO_BALLOON_CMD_ID_STOP);
>  		vb->cmd_id_stop = cpu_to_virtio32(vb->vdev,
> 

Re: [PATCH v3 0/3] virtio-balloon: tweak config_changed

[Christian Borntraeger]

On 07.01.2019 14:45, Michael S. Tsirkin wrote:
> On Mon, Jan 07, 2019 at 03:01:03PM +0800, Wei Wang wrote:
>> Since virtio-ccw doesn't work with accessing to the config space
>> inside an interrupt context, this patch series avoids that issue by
>> moving the config register accesses to the related workqueue contexts.
> 
> So is this enough to get ccw going again or do we also need
> the patches that deal with NULL VQ names?

This alone does not fix 4.20. You need the other ones as well.(The other patches
fix the memory corruption during boot, this patch fixes the deadlock when actually
changing the balloon size)


> 
>> v2->v3 ChangeLog:
>>     - rename cmd_id_received to cmd_id_received_cache, and have call sites
>>       read the latest value via virtio_balloon_cmd_id_received. (Still
>>       kept Cornelia and Halil's reviewed-by as it's a minor change)
>>     - remove zeroing vb->num_free_page_blocks in probe since vb is
>>       allocated via kzalloc.
>> v1->v2 ChangeLog:
>>     - add config_read_bitmap to indicate to the workqueue callbacks about
>>       the necessity of reading the related config fields.
>>
>> Wei Wang (3):
>>   virtio-balloon: tweak config_changed implementation
>>   virtio-balloon: improve update_balloon_size_func
>>   virtio_balloon: remove the unnecessary 0-initialization
>>
>>  drivers/virtio/virtio_balloon.c | 104 ++++++++++++++++++++++++++--------------
>>  1 file changed, 69 insertions(+), 35 deletions(-)
>>
>> -- 
>> 2.7.4
> 

Re: [PATCH v3 3/3] virtio_balloon: remove the unnecessary 0-initialization

[Cornelia Huck]

On Mon,  7 Jan 2019 15:01:06 +0800
Wei Wang <wei.w.wang@intel.com> wrote:

> We've changed to kzalloc the vb struct, so no need to 0-initialize
> this field one more time.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  drivers/virtio/virtio_balloon.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index e33dc8e..f19061b 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -925,7 +925,6 @@ static int virtballoon_probe(struct virtio_device *vdev)
>  						  VIRTIO_BALLOON_CMD_ID_STOP);
>  		vb->cmd_id_stop = cpu_to_virtio32(vb->vdev,
>  						  VIRTIO_BALLOON_CMD_ID_STOP);
> -		vb->num_free_page_blocks = 0;
>  		spin_lock_init(&vb->free_page_list_lock);
>  		INIT_LIST_HEAD(&vb->free_page_list);
>  		if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Wei Wang]

On 01/07/2019 09:49 PM, Christian Borntraeger wrote:
>
> On 07.01.2019 08:01, Wei Wang wrote:
>> virtio-ccw has deadlock issues with reading the config space inside the
>> interrupt context, so we tweak the virtballoon_changed implementation
>> by moving the config read operations into the related workqueue contexts.
>> The config_read_bitmap is used as a flag to the workqueue callbacks
>> about the related config fields that need to be read.
>>
>> The cmd_id_received is also renamed to cmd_id_received_cache, and
>> the value should be obtained via virtio_balloon_cmd_id_received.
>>
>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> Together with
>    virtio_pci: use queue idx instead of array idx to set up the vq
>    virtio: don't allocate vqs when names[i] = NULL
>
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>

OK. I don't plan to send a new version of the above patches as no 
changes needed so far.

Michael, if the above two patches look good to you, please help add the 
related tested-by
and reviewed-by tags. Thanks.

Best,
Wei

Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Christian Borntraeger]

On 08.01.2019 06:35, Wei Wang wrote:
> On 01/07/2019 09:49 PM, Christian Borntraeger wrote:
>>
>> On 07.01.2019 08:01, Wei Wang wrote:
>>> virtio-ccw has deadlock issues with reading the config space inside the
>>> interrupt context, so we tweak the virtballoon_changed implementation
>>> by moving the config read operations into the related workqueue contexts.
>>> The config_read_bitmap is used as a flag to the workqueue callbacks
>>> about the related config fields that need to be read.
>>>
>>> The cmd_id_received is also renamed to cmd_id_received_cache, and
>>> the value should be obtained via virtio_balloon_cmd_id_received.
>>>
>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
>> Together with
>>    virtio_pci: use queue idx instead of array idx to set up the vq
>>    virtio: don't allocate vqs when names[i] = NULL
>>
>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> OK. I don't plan to send a new version of the above patches as no changes needed so far.
> 
> Michael, if the above two patches look good to you, please help add the related tested-by
> and reviewed-by tags. Thanks.

Can we also make sure that 

virtio_pci: use queue idx instead of array idx to set up the vq
virtio: don't allocate vqs when names[i] = NULL

also land in stable?

Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Wei Wang]

On 01/08/2019 04:46 PM, Christian Borntraeger wrote:
>
> On 08.01.2019 06:35, Wei Wang wrote:
>> On 01/07/2019 09:49 PM, Christian Borntraeger wrote:
>>> On 07.01.2019 08:01, Wei Wang wrote:
>>>> virtio-ccw has deadlock issues with reading the config space inside the
>>>> interrupt context, so we tweak the virtballoon_changed implementation
>>>> by moving the config read operations into the related workqueue contexts.
>>>> The config_read_bitmap is used as a flag to the workqueue callbacks
>>>> about the related config fields that need to be read.
>>>>
>>>> The cmd_id_received is also renamed to cmd_id_received_cache, and
>>>> the value should be obtained via virtio_balloon_cmd_id_received.
>>>>
>>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
>>> Together with
>>>     virtio_pci: use queue idx instead of array idx to set up the vq
>>>     virtio: don't allocate vqs when names[i] = NULL
>>>
>>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> OK. I don't plan to send a new version of the above patches as no changes needed so far.
>>
>> Michael, if the above two patches look good to you, please help add the related tested-by
>> and reviewed-by tags. Thanks.
> Can we also make sure that
>
> virtio_pci: use queue idx instead of array idx to set up the vq
> virtio: don't allocate vqs when names[i] = NULL
>
> also land in stable?
>

You could also send the request to stable after it gets merged to Linus' 
tree.
The stable review committee will decide whether to take it.

Please see Option 2:

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html


Best,
Wei

Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Christian Borntraeger]

On 09.01.2019 11:35, Wei Wang wrote:
> On 01/08/2019 04:46 PM, Christian Borntraeger wrote:
>>
>> On 08.01.2019 06:35, Wei Wang wrote:
>>> On 01/07/2019 09:49 PM, Christian Borntraeger wrote:
>>>> On 07.01.2019 08:01, Wei Wang wrote:
>>>>> virtio-ccw has deadlock issues with reading the config space inside the
>>>>> interrupt context, so we tweak the virtballoon_changed implementation
>>>>> by moving the config read operations into the related workqueue contexts.
>>>>> The config_read_bitmap is used as a flag to the workqueue callbacks
>>>>> about the related config fields that need to be read.
>>>>>
>>>>> The cmd_id_received is also renamed to cmd_id_received_cache, and
>>>>> the value should be obtained via virtio_balloon_cmd_id_received.
>>>>>
>>>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
>>>> Together with
>>>>     virtio_pci: use queue idx instead of array idx to set up the vq
>>>>     virtio: don't allocate vqs when names[i] = NULL
>>>>
>>>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> OK. I don't plan to send a new version of the above patches as no changes needed so far.
>>>
>>> Michael, if the above two patches look good to you, please help add the related tested-by
>>> and reviewed-by tags. Thanks.
>> Can we also make sure that
>>
>> virtio_pci: use queue idx instead of array idx to set up the vq
>> virtio: don't allocate vqs when names[i] = NULL
>>
>> also land in stable?
>>
> 
> You could also send the request to stable after it gets merged to Linus' tree.
> The stable review committee will decide whether to take it.
> 
> Please see Option 2:
> 
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> 

Those patches are not upstream yet, Correct?

Michael,

can you add the stable tag before submitting? If not, can you give me a heads up when doing the
pull request so that I can ping the stable folks.

Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Michael S. Tsirkin]

On Wed, Jan 09, 2019 at 06:35:01PM +0800, Wei Wang wrote:
> On 01/08/2019 04:46 PM, Christian Borntraeger wrote:
> > 
> > On 08.01.2019 06:35, Wei Wang wrote:
> > > On 01/07/2019 09:49 PM, Christian Borntraeger wrote:
> > > > On 07.01.2019 08:01, Wei Wang wrote:
> > > > > virtio-ccw has deadlock issues with reading the config space inside the
> > > > > interrupt context, so we tweak the virtballoon_changed implementation
> > > > > by moving the config read operations into the related workqueue contexts.
> > > > > The config_read_bitmap is used as a flag to the workqueue callbacks
> > > > > about the related config fields that need to be read.
> > > > > 
> > > > > The cmd_id_received is also renamed to cmd_id_received_cache, and
> > > > > the value should be obtained via virtio_balloon_cmd_id_received.
> > > > > 
> > > > > Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > > > > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > > > > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > > > > Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> > > > Together with
> > > >     virtio_pci: use queue idx instead of array idx to set up the vq
> > > >     virtio: don't allocate vqs when names[i] = NULL
> > > > 
> > > > Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > > OK. I don't plan to send a new version of the above patches as no changes needed so far.
> > > 
> > > Michael, if the above two patches look good to you, please help add the related tested-by
> > > and reviewed-by tags. Thanks.
> > Can we also make sure that
> > 
> > virtio_pci: use queue idx instead of array idx to set up the vq
> > virtio: don't allocate vqs when names[i] = NULL
> > 
> > also land in stable?
> > 
> 
> You could also send the request to stable after it gets merged to Linus'
> tree.
> The stable review committee will decide whether to take it.
> 
> Please see Option 2:
> 
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> 
> 
> Best,
> Wei

That's mostly for 3rd party reporters.
Why not Cc stable directly in the patches?

-- 
MST

Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Michael S. Tsirkin]

On Wed, Jan 09, 2019 at 01:07:16PM +0100, Christian Borntraeger wrote:
> On 09.01.2019 11:35, Wei Wang wrote:
> > On 01/08/2019 04:46 PM, Christian Borntraeger wrote:
> >>
> >> On 08.01.2019 06:35, Wei Wang wrote:
> >>> On 01/07/2019 09:49 PM, Christian Borntraeger wrote:
> >>>> On 07.01.2019 08:01, Wei Wang wrote:
> >>>>> virtio-ccw has deadlock issues with reading the config space inside the
> >>>>> interrupt context, so we tweak the virtballoon_changed implementation
> >>>>> by moving the config read operations into the related workqueue contexts.
> >>>>> The config_read_bitmap is used as a flag to the workqueue callbacks
> >>>>> about the related config fields that need to be read.
> >>>>>
> >>>>> The cmd_id_received is also renamed to cmd_id_received_cache, and
> >>>>> the value should be obtained via virtio_balloon_cmd_id_received.
> >>>>>
> >>>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> >>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >>>>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> >>>> Together with
> >>>>     virtio_pci: use queue idx instead of array idx to set up the vq
> >>>>     virtio: don't allocate vqs when names[i] = NULL
> >>>>
> >>>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>> OK. I don't plan to send a new version of the above patches as no changes needed so far.
> >>>
> >>> Michael, if the above two patches look good to you, please help add the related tested-by
> >>> and reviewed-by tags. Thanks.
> >> Can we also make sure that
> >>
> >> virtio_pci: use queue idx instead of array idx to set up the vq
> >> virtio: don't allocate vqs when names[i] = NULL
> >>
> >> also land in stable?
> >>
> > 
> > You could also send the request to stable after it gets merged to Linus' tree.
> > The stable review committee will decide whether to take it.
> > 
> > Please see Option 2:
> > 
> > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > 
> 
> Those patches are not upstream yet, Correct?
> 
> Michael,
> 
> can you add the stable tag before submitting? If not, can you give me a heads up when doing the
> pull request so that I can ping the stable folks.

Can you reply to patches that you feel are needed on stable with just

Cc: stable@vger.kernel.org

in the message body?

Then it's all automatically handled by ack attaching scripts.

-- 
MST

Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Christian Borntraeger]

On 09.01.2019 15:52, Michael S. Tsirkin wrote:
> On Wed, Jan 09, 2019 at 01:07:16PM +0100, Christian Borntraeger wrote:
>> On 09.01.2019 11:35, Wei Wang wrote:
>>> On 01/08/2019 04:46 PM, Christian Borntraeger wrote:
>>>>
>>>> On 08.01.2019 06:35, Wei Wang wrote:
>>>>> On 01/07/2019 09:49 PM, Christian Borntraeger wrote:
>>>>>> On 07.01.2019 08:01, Wei Wang wrote:
>>>>>>> virtio-ccw has deadlock issues with reading the config space inside the
>>>>>>> interrupt context, so we tweak the virtballoon_changed implementation
>>>>>>> by moving the config read operations into the related workqueue contexts.
>>>>>>> The config_read_bitmap is used as a flag to the workqueue callbacks
>>>>>>> about the related config fields that need to be read.
>>>>>>>
>>>>>>> The cmd_id_received is also renamed to cmd_id_received_cache, and
>>>>>>> the value should be obtained via virtio_balloon_cmd_id_received.
>>>>>>>
>>>>>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>>>>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
>>>>>> Together with
>>>>>>     virtio_pci: use queue idx instead of array idx to set up the vq
>>>>>>     virtio: don't allocate vqs when names[i] = NULL
>>>>>>
>>>>>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>> OK. I don't plan to send a new version of the above patches as no changes needed so far.
>>>>>
>>>>> Michael, if the above two patches look good to you, please help add the related tested-by
>>>>> and reviewed-by tags. Thanks.
>>>> Can we also make sure that
>>>>
>>>> virtio_pci: use queue idx instead of array idx to set up the vq
>>>> virtio: don't allocate vqs when names[i] = NULL
>>>>
>>>> also land in stable?
>>>>
>>>
>>> You could also send the request to stable after it gets merged to Linus' tree.
>>> The stable review committee will decide whether to take it.
>>>
>>> Please see Option 2:
>>>
>>> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>>>
>>
>> Those patches are not upstream yet, Correct?
>>
>> Michael,
>>
>> can you add the stable tag before submitting? If not, can you give me a heads up when doing the
>> pull request so that I can ping the stable folks.
> 
> Can you reply to patches that you feel are needed on stable with just
> 
> Cc: stable@vger.kernel.org
> 
> in the message body?
> 
> Then it's all automatically handled by ack attaching scripts.

Done. But this only works if those patches are not already part of a tree. I guess they have to go via
your tree, correct?

Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Michael S. Tsirkin]

On Wed, Jan 09, 2019 at 07:22:50PM +0100, Christian Borntraeger wrote:
> 
> On 09.01.2019 15:52, Michael S. Tsirkin wrote:
> > On Wed, Jan 09, 2019 at 01:07:16PM +0100, Christian Borntraeger wrote:
> >> On 09.01.2019 11:35, Wei Wang wrote:
> >>> On 01/08/2019 04:46 PM, Christian Borntraeger wrote:
> >>>>
> >>>> On 08.01.2019 06:35, Wei Wang wrote:
> >>>>> On 01/07/2019 09:49 PM, Christian Borntraeger wrote:
> >>>>>> On 07.01.2019 08:01, Wei Wang wrote:
> >>>>>>> virtio-ccw has deadlock issues with reading the config space inside the
> >>>>>>> interrupt context, so we tweak the virtballoon_changed implementation
> >>>>>>> by moving the config read operations into the related workqueue contexts.
> >>>>>>> The config_read_bitmap is used as a flag to the workqueue callbacks
> >>>>>>> about the related config fields that need to be read.
> >>>>>>>
> >>>>>>> The cmd_id_received is also renamed to cmd_id_received_cache, and
> >>>>>>> the value should be obtained via virtio_balloon_cmd_id_received.
> >>>>>>>
> >>>>>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>>>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> >>>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >>>>>>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> >>>>>> Together with
> >>>>>>     virtio_pci: use queue idx instead of array idx to set up the vq
> >>>>>>     virtio: don't allocate vqs when names[i] = NULL
> >>>>>>
> >>>>>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>>>> OK. I don't plan to send a new version of the above patches as no changes needed so far.
> >>>>>
> >>>>> Michael, if the above two patches look good to you, please help add the related tested-by
> >>>>> and reviewed-by tags. Thanks.
> >>>> Can we also make sure that
> >>>>
> >>>> virtio_pci: use queue idx instead of array idx to set up the vq
> >>>> virtio: don't allocate vqs when names[i] = NULL
> >>>>
> >>>> also land in stable?
> >>>>
> >>>
> >>> You could also send the request to stable after it gets merged to Linus' tree.
> >>> The stable review committee will decide whether to take it.
> >>>
> >>> Please see Option 2:
> >>>
> >>> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> >>>
> >>
> >> Those patches are not upstream yet, Correct?
> >>
> >> Michael,
> >>
> >> can you add the stable tag before submitting? If not, can you give me a heads up when doing the
> >> pull request so that I can ping the stable folks.
> > 
> > Can you reply to patches that you feel are needed on stable with just
> > 
> > Cc: stable@vger.kernel.org
> > 
> > in the message body?
> > 
> > Then it's all automatically handled by ack attaching scripts.
> 
> Done. But this only works if those patches are not already part of a tree. I guess they have to go via
> your tree, correct?

Yes. It works because I rebase my tree.

-- 
MST

Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation

[Michael S. Tsirkin]

On Mon, Jan 07, 2019 at 02:49:03PM +0100, Christian Borntraeger wrote:
> 
> 
> On 07.01.2019 08:01, Wei Wang wrote:
> > virtio-ccw has deadlock issues with reading the config space inside the
> > interrupt context, so we tweak the virtballoon_changed implementation
> > by moving the config read operations into the related workqueue contexts.
> > The config_read_bitmap is used as a flag to the workqueue callbacks
> > about the related config fields that need to be read.
> > 
> > The cmd_id_received is also renamed to cmd_id_received_cache, and
> > the value should be obtained via virtio_balloon_cmd_id_received.
> > 
> > Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> 
> Together with 
>   virtio_pci: use queue idx instead of array idx to set up the vq
>   virtio: don't allocate vqs when names[i] = NULL
> 
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> as already said, would be good to add cc stable (and a Fixes: tag)


I don't think you did that.
Did it manually as the pull needed some manual handling
but I won't next time and you will have to send it to
Greg yourself.


> 
> > ---
> >  drivers/virtio/virtio_balloon.c | 98 +++++++++++++++++++++++++++--------------
> >  1 file changed, 65 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index 728ecd1..fb12fe2 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -61,6 +61,10 @@ enum virtio_balloon_vq {
> >  	VIRTIO_BALLOON_VQ_MAX
> >  };
> >  
> > +enum virtio_balloon_config_read {
> > +	VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
> > +};
> > +
> >  struct virtio_balloon {
> >  	struct virtio_device *vdev;
> >  	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > @@ -77,14 +81,20 @@ struct virtio_balloon {
> >  	/* Prevent updating balloon when it is being canceled. */
> >  	spinlock_t stop_update_lock;
> >  	bool stop_update;
> > +	/* Bitmap to indicate if reading the related config fields are needed */
> > +	unsigned long config_read_bitmap;
> >  
> >  	/* The list of allocated free pages, waiting to be given back to mm */
> >  	struct list_head free_page_list;
> >  	spinlock_t free_page_list_lock;
> >  	/* The number of free page blocks on the above list */
> >  	unsigned long num_free_page_blocks;
> > -	/* The cmd id received from host */
> > -	u32 cmd_id_received;
> > +	/*
> > +	 * The cmd id received from host.
> > +	 * Read it via virtio_balloon_cmd_id_received to get the latest value
> > +	 * sent from host.
> > +	 */
> > +	u32 cmd_id_received_cache;
> >  	/* The cmd id that is actively in use */
> >  	__virtio32 cmd_id_active;
> >  	/* Buffer to store the stop sign */
> > @@ -390,37 +400,31 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
> >  	return num_returned;
> >  }
> >  
> > +static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
> > +{
> > +	if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> > +		return;
> > +
> > +	/* No need to queue the work if the bit was already set. */
> > +	if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> > +			     &vb->config_read_bitmap))
> > +		return;
> > +
> > +	queue_work(vb->balloon_wq, &vb->report_free_page_work);
> > +}
> > +
> >  static void virtballoon_changed(struct virtio_device *vdev)
> >  {
> >  	struct virtio_balloon *vb = vdev->priv;
> >  	unsigned long flags;
> > -	s64 diff = towards_target(vb);
> > -
> > -	if (diff) {
> > -		spin_lock_irqsave(&vb->stop_update_lock, flags);
> > -		if (!vb->stop_update)
> > -			queue_work(system_freezable_wq,
> > -				   &vb->update_balloon_size_work);
> > -		spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> > -	}
> >  
> > -	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> > -		virtio_cread(vdev, struct virtio_balloon_config,
> > -			     free_page_report_cmd_id, &vb->cmd_id_received);
> > -		if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> > -			/* Pass ULONG_MAX to give back all the free pages */
> > -			return_free_pages_to_mm(vb, ULONG_MAX);
> > -		} else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> > -			   vb->cmd_id_received !=
> > -			   virtio32_to_cpu(vdev, vb->cmd_id_active)) {
> > -			spin_lock_irqsave(&vb->stop_update_lock, flags);
> > -			if (!vb->stop_update) {
> > -				queue_work(vb->balloon_wq,
> > -					   &vb->report_free_page_work);
> > -			}
> > -			spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> > -		}
> > +	spin_lock_irqsave(&vb->stop_update_lock, flags);
> > +	if (!vb->stop_update) {
> > +		queue_work(system_freezable_wq,
> > +			   &vb->update_balloon_size_work);
> > +		virtio_balloon_queue_free_page_work(vb);
> >  	}
> > +	spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> >  }
> >  
> >  static void update_balloon_size(struct virtio_balloon *vb)
> > @@ -527,6 +531,17 @@ static int init_vqs(struct virtio_balloon *vb)
> >  	return 0;
> >  }
> >  
> > +static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb)
> > +{
> > +	if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> > +			       &vb->config_read_bitmap))
> > +		virtio_cread(vb->vdev, struct virtio_balloon_config,
> > +			     free_page_report_cmd_id,
> > +			     &vb->cmd_id_received_cache);
> > +
> > +	return vb->cmd_id_received_cache;
> > +}
> > +
> >  static int send_cmd_id_start(struct virtio_balloon *vb)
> >  {
> >  	struct scatterlist sg;
> > @@ -537,7 +552,8 @@ static int send_cmd_id_start(struct virtio_balloon *vb)
> >  	while (virtqueue_get_buf(vq, &unused))
> >  		;
> >  
> > -	vb->cmd_id_active = cpu_to_virtio32(vb->vdev, vb->cmd_id_received);
> > +	vb->cmd_id_active = virtio32_to_cpu(vb->vdev,
> > +					virtio_balloon_cmd_id_received(vb));
> >  	sg_init_one(&sg, &vb->cmd_id_active, sizeof(vb->cmd_id_active));
> >  	err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active, GFP_KERNEL);
> >  	if (!err)
> > @@ -620,7 +636,8 @@ static int send_free_pages(struct virtio_balloon *vb)
> >  		 * stop the reporting.
> >  		 */
> >  		cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
> > -		if (cmd_id_active != vb->cmd_id_received)
> > +		if (unlikely(cmd_id_active !=
> > +			     virtio_balloon_cmd_id_received(vb)))
> >  			break;
> >  
> >  		/*
> > @@ -637,11 +654,9 @@ static int send_free_pages(struct virtio_balloon *vb)
> >  	return 0;
> >  }
> >  
> > -static void report_free_page_func(struct work_struct *work)
> > +static void virtio_balloon_report_free_page(struct virtio_balloon *vb)
> >  {
> >  	int err;
> > -	struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> > -						 report_free_page_work);
> >  	struct device *dev = &vb->vdev->dev;
> >  
> >  	/* Start by sending the received cmd id to host with an outbuf. */
> > @@ -659,6 +674,23 @@ static void report_free_page_func(struct work_struct *work)
> >  		dev_err(dev, "Failed to send a stop id, err = %d\n", err);
> >  }
> >  
> > +static void report_free_page_func(struct work_struct *work)
> > +{
> > +	struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> > +						 report_free_page_work);
> > +	u32 cmd_id_received;
> > +
> > +	cmd_id_received = virtio_balloon_cmd_id_received(vb);
> > +	if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> > +		/* Pass ULONG_MAX to give back all the free pages */
> > +		return_free_pages_to_mm(vb, ULONG_MAX);
> > +	} else if (cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> > +		   cmd_id_received !=
> > +		   virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) {
> > +		virtio_balloon_report_free_page(vb);
> > +	}
> > +}
> > +
> >  #ifdef CONFIG_BALLOON_COMPACTION
> >  /*
> >   * virtballoon_migratepage - perform the balloon page migration on behalf of
> > @@ -885,7 +917,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> >  			goto out_del_vqs;
> >  		}
> >  		INIT_WORK(&vb->report_free_page_work, report_free_page_func);
> > -		vb->cmd_id_received = VIRTIO_BALLOON_CMD_ID_STOP;
> > +		vb->cmd_id_received_cache = VIRTIO_BALLOON_CMD_ID_STOP;
> >  		vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
> >  						  VIRTIO_BALLOON_CMD_ID_STOP);
> >  		vb->cmd_id_stop = cpu_to_virtio32(vb->vdev,
> > 

Re: [PATCH v3 2/3] virtio-balloon: improve update_balloon_size_func

[Michael S. Tsirkin]

On Mon, Jan 07, 2019 at 03:01:05PM +0800, Wei Wang wrote:
> There is no need to update the balloon actual register when there is no
> ballooning request. This patch avoids update_balloon_size when diff is 0.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

It sounds reasonable but it does have a side effect of
not writing into actual anymore.

I am not sure actual never can get out of sync as a result.

So better deferred, pls send this in the next merge window.




> ---
>  drivers/virtio/virtio_balloon.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index fb12fe2..e33dc8e 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -457,9 +457,12 @@ static void update_balloon_size_func(struct work_struct *work)
>  			  update_balloon_size_work);
>  	diff = towards_target(vb);
>  
> +	if (!diff)
> +		return;
> +
>  	if (diff > 0)
>  		diff -= fill_balloon(vb, diff);
> -	else if (diff < 0)
> +	else
>  		diff += leak_balloon(vb, -diff);
>  	update_balloon_size(vb);
>  
> -- 
> 2.7.4

Re: [PATCH v3 2/3] virtio-balloon: improve update_balloon_size_func

[Wei Wang]

On 01/15/2019 09:00 AM, Michael S. Tsirkin wrote:
> On Mon, Jan 07, 2019 at 03:01:05PM +0800, Wei Wang wrote:
>> There is no need to update the balloon actual register when there is no
>> ballooning request. This patch avoids update_balloon_size when diff is 0.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> It sounds reasonable but it does have a side effect of
> not writing into actual anymore.
>
> I am not sure actual never can get out of sync as a result.
>
> So better deferred, pls send this in the next merge window.
>

No problem, it's not urgent to me.

Best,
Wei