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

["Michael S. Tsirkin" <mst@redhat.com>]

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,
> >