[PATCH v2 0/2] virtio-balloon: tweak config_changed

[PATCH v2 0/2] 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.

v1->v2 ChangeLog:
    - add config_read_bitmap to indicate to the workqueue callbacks about
      the necessity of reading the related config fields.

Wei Wang (2):
  virtio-balloon: tweak config_changed implementation
  virtio-balloon: improve update_balloon_size_func

 drivers/virtio/virtio_balloon.c | 86 +++++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 29 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] 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.

Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 drivers/virtio/virtio_balloon.c | 81 +++++++++++++++++++++++++++--------------
 1 file changed, 53 insertions(+), 28 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 728ecd1..35ee762 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,6 +81,8 @@ 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;
@@ -390,37 +396,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)
@@ -609,6 +609,16 @@ static int get_free_page_and_send(struct virtio_balloon *vb)
 	return 0;
 }
 
+static void virtio_balloon_read_cmd_id_received(struct virtio_balloon *vb)
+{
+	if (!test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
+				&vb->config_read_bitmap))
+		return;
+
+	virtio_cread(vb->vdev, struct virtio_balloon_config,
+		     free_page_report_cmd_id, &vb->cmd_id_received);
+}
+
 static int send_free_pages(struct virtio_balloon *vb)
 {
 	int err;
@@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
 		 * stop the reporting.
 		 */
 		cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
+		virtio_balloon_read_cmd_id_received(vb);
 		if (cmd_id_active != vb->cmd_id_received)
 			break;
 
@@ -637,11 +648,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 +668,22 @@ 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);
+
+	virtio_balloon_read_cmd_id_received(vb);
+	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(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
-- 
2.7.4

[PATCH v2 2/2] 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 35ee762..af1e6d9 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -453,9 +453,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 v2 1/2] virtio-balloon: tweak config_changed implementation

[Cornelia Huck]

On Fri,  4 Jan 2019 15:11:52 +0800
Wei Wang <wei.w.wang@intel.com> 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.
> 
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  drivers/virtio/virtio_balloon.c | 81 +++++++++++++++++++++++++++--------------
>  1 file changed, 53 insertions(+), 28 deletions(-)
> 

(...)

> @@ -77,6 +81,8 @@ 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 */

s/are/is/

> +	unsigned long config_read_bitmap;
>  
>  	/* The list of allocated free pages, waiting to be given back to mm */
>  	struct list_head free_page_list;

(...)

Bitmap handling looks sane to me.

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

Re: [virtio-dev] [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation

[Halil Pasic]

On Fri,  4 Jan 2019 15:11:52 +0800
Wei Wang <wei.w.wang@intel.com> 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.
> 
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation

[Michael S. Tsirkin]

On Fri, Jan 04, 2019 at 03:11:52PM +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.
> 
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  drivers/virtio/virtio_balloon.c | 81 +++++++++++++++++++++++++++--------------
>  1 file changed, 53 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 728ecd1..35ee762 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,6 +81,8 @@ 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;

It seems that you never initialize this bitmap. Probably harmless here
but generally using uninitialized memory isn't good.


> @@ -390,37 +396,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)
> @@ -609,6 +609,16 @@ static int get_free_page_and_send(struct virtio_balloon *vb)
>  	return 0;
>  }
>  
> +static void virtio_balloon_read_cmd_id_received(struct virtio_balloon *vb)
> +{
> +	if (!test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> +				&vb->config_read_bitmap))
> +		return;
> +
> +	virtio_cread(vb->vdev, struct virtio_balloon_config,
> +		     free_page_report_cmd_id, &vb->cmd_id_received);
> +}
> +
>  static int send_free_pages(struct virtio_balloon *vb)
>  {
>  	int err;
> @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
>  		 * stop the reporting.
>  		 */
>  		cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
> +		virtio_balloon_read_cmd_id_received(vb);
>  		if (cmd_id_active != vb->cmd_id_received)
>  			break;
>  
> @@ -637,11 +648,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 +668,22 @@ 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);
> +
> +	virtio_balloon_read_cmd_id_received(vb);

This will not achieve what you are trying to do,
which is cancel reporting if it's in progress.

You need to re-read each time you compare to cmd_id_active.

An API similar to
	u32 virtio_balloon_cmd_id_received(vb)
seems to be called for, and I would rename cmd_id_received to
cmd_id_received_cache to make sure we caught all users.



> +	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(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
> -- 
> 2.7.4

RE: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation

[Wang, Wei W]

On Friday, January 4, 2019 11:45 PM, Michael S. Tsirkin wrote:
> >  struct virtio_balloon {
> >  	struct virtio_device *vdev;
> >  	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > @@ -77,6 +81,8 @@ 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;
> 
> It seems that you never initialize this bitmap. Probably harmless here but
> generally using uninitialized memory isn't good.

We've used kzalloc to allocate the vb struct, so it's already 0-initialized :)

> > +
> >  static int send_free_pages(struct virtio_balloon *vb)  {
> >  	int err;
> > @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> >  		 * stop the reporting.
> >  		 */
> >  		cmd_id_active = virtio32_to_cpu(vb->vdev, vb-
> >cmd_id_active);
> > +		virtio_balloon_read_cmd_id_received(vb);
 

[1]


> >  		if (cmd_id_active != vb->cmd_id_received)
> >  			break;
> >
> > @@ -637,11 +648,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 +668,22 @@ 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);
> > +
> > +	virtio_balloon_read_cmd_id_received(vb);
> 
> This will not achieve what you are trying to do, which is cancel reporting if it's
> in progress.
> 
> You need to re-read each time you compare to cmd_id_active.

Yes, already did, please see [1] above


> An API similar to
> 	u32 virtio_balloon_cmd_id_received(vb)
> seems to be called for, and I would rename cmd_id_received to
> cmd_id_received_cache to make sure we caught all users.
> 

I'm not sure about adding "cache" here, cmd_id_received refers to the cmd id
that the driver just received from the device. There is one called "cmd_id_active" which
is the one that the driver is actively using. 
So cmd_id_received is already a "cache" to " cmd_id_active " in some sense.

Best,
Wei

Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation

[Michael S. Tsirkin]

On Sat, Jan 05, 2019 at 03:32:44AM +0000, Wang, Wei W wrote:
> On Friday, January 4, 2019 11:45 PM, Michael S. Tsirkin wrote:
> > >  struct virtio_balloon {
> > >  	struct virtio_device *vdev;
> > >  	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > > @@ -77,6 +81,8 @@ 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;
> > 
> > It seems that you never initialize this bitmap. Probably harmless here but
> > generally using uninitialized memory isn't good.
> 
> We've used kzalloc to allocate the vb struct, so it's already 0-initialized :)

Ah ok. We do explicitly init other fields like stop_update so
it seems cleaner to init this one too though.

> > > +
> > >  static int send_free_pages(struct virtio_balloon *vb)  {
> > >  	int err;
> > > @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> > >  		 * stop the reporting.
> > >  		 */
> > >  		cmd_id_active = virtio32_to_cpu(vb->vdev, vb-
> > >cmd_id_active);
> > > +		virtio_balloon_read_cmd_id_received(vb);
>  
> 
> [1]
> 
> 
> > >  		if (cmd_id_active != vb->cmd_id_received)
> > >  			break;
> > >
> > > @@ -637,11 +648,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 +668,22 @@ 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);
> > > +
> > > +	virtio_balloon_read_cmd_id_received(vb);
> > 
> > This will not achieve what you are trying to do, which is cancel reporting if it's
> > in progress.
> > 
> > You need to re-read each time you compare to cmd_id_active.
> 
> Yes, already did, please see [1] above

Oh right. Sorry.

> 
> > An API similar to
> > 	u32 virtio_balloon_cmd_id_received(vb)
> > seems to be called for, and I would rename cmd_id_received to
> > cmd_id_received_cache to make sure we caught all users.
> > 
> 
> I'm not sure about adding "cache" here, cmd_id_received refers to the cmd id
> that the driver just received from the device. There is one called "cmd_id_active" which
> is the one that the driver is actively using. 
> So cmd_id_received is already a "cache" to " cmd_id_active " in some sense.
> 
> Best,
> Wei

The point is that any access to cmd_id_received should first call
virtio_balloon_read_cmd_id_received. So a better API is
to have virtio_balloon_read_cmd_id_received return the value
instead of poking at cmd_id_received afterwards.
Renaming cmd_id_received will help make sure all no
call sites were missed and help stress it's never used directly.