[v3,1/3] virtio-balloon: tweak config_changed implementation
diff mbox series

Message ID 1546844466-38079-2-git-send-email-wei.w.wang@intel.com
State Superseded
Headers show
Series
  • virtio-balloon: tweak config_changed
Related show

Commit Message

Wei Wang Jan. 7, 2019, 7:01 a.m. UTC
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(-)

Comments

Michael S. Tsirkin Jan. 7, 2019, 1:44 p.m. UTC | #1
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
Michael S. Tsirkin Jan. 7, 2019, 1:47 p.m. UTC | #2
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
Christian Borntraeger Jan. 7, 2019, 1:49 p.m. UTC | #3
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,
>
Wei Wang Jan. 8, 2019, 5:35 a.m. UTC | #4
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
Christian Borntraeger Jan. 8, 2019, 8:46 a.m. UTC | #5
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?
Wei Wang Jan. 9, 2019, 10:35 a.m. UTC | #6
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
Christian Borntraeger Jan. 9, 2019, 12:07 p.m. UTC | #7
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.
Michael S. Tsirkin Jan. 9, 2019, 2:49 p.m. UTC | #8
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?
Michael S. Tsirkin Jan. 9, 2019, 2:52 p.m. UTC | #9
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.
Christian Borntraeger Jan. 9, 2019, 6:22 p.m. UTC | #10
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?
Michael S. Tsirkin Jan. 9, 2019, 6:31 p.m. UTC | #11
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.
Michael S. Tsirkin Jan. 15, 2019, 12:53 a.m. UTC | #12
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,
> >

Patch
diff mbox series

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,