Re: [PATCH v2 2/2] virtio_balloon: avoid shrinker execution during PM suspend

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

On Fri, Jul 17, 2026 at 12:22:21AM +0000, Link Lin wrote:
> During PM freeze (e.g. S4 hibernation), virtballoon_freeze() calls
> remove_common() which resets the virtio device and deletes all virtqueues.
> However, the balloon shrinker remains registered with core MM.
> 
> If memory pressure occurs during S4 hibernation image creation/saving, MM
> invokes virtio_balloon_shrinker_scan(), which attempts to reclaim free
> pages. Although return_free_pages_to_mm() only frees pages back to MM,
> reclaiming free pages under memory pressure can trigger page reporting
> which might access the deleted reporting virtqueue if it is not yet
> frozen, or interact with other parts of the driver in a teardown state.


i feel this is the thing to fix then? not the freeing itself.

> 
> Avoid this by adding a `suspended` flag to `struct virtio_balloon`. Set
> this flag to true in virtballoon_freeze() and false in
> virtballoon_restore(). Check this flag in both shrinker callbacks (scan
> and count) and return 0 if the device is suspended, preventing any
> shrinker execution while virtqueues are deleted. Wrap the lockless reads
> in READ_ONCE() and writes in WRITE_ONCE() to prevent compiler
> optimization issues and KCSAN data race warnings.

so you prevented the warnings but did you fix the races? CB

> 
> Fixes: 71019de8219b ("virtio_balloon: Add free page hinting support")
> Cc: stable@vger.kernel.org
> Acked-by: David Rientjes <rientjes@google.com>
> Signed-off-by: Link Lin <linkl@google.com>
> ---
>  drivers/virtio/virtio_balloon.c | 49 +++++++++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 088b3a0e6c..38e1166a64 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -68,6 +68,8 @@ struct virtio_balloon {
>  	/* Prevent updating balloon when it is being canceled. */
>  	spinlock_t stop_update_lock;
>  	bool stop_update;
> +	/* Prevent shrinker from running while device is suspended. */
> +	bool suspended;
>  	/* Bitmap to indicate if reading the related config fields are needed */
>  	unsigned long config_read_bitmap;
>  
> @@ -471,9 +473,9 @@ static inline s64 towards_target(struct virtio_balloon *vb)
>  	return target - vb->num_pages;
>  }
>  
> -/* Gives back @num_to_return blocks of free pages to mm. */
> -static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
> -					     unsigned long num_to_return)
> +/* Helper: must be called with free_page_list_lock held */
> +static unsigned long __return_free_pages_to_mm(struct virtio_balloon *vb,
> +					       unsigned long num_to_return)
>  {
>  	unsigned long num_returned = 0;
>  	struct page *page, *next;
> @@ -481,8 +483,6 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
>  	if (unlikely(!num_to_return))
>  		return 0;
>  
> -	spin_lock_irq(&vb->free_page_list_lock);
> -
>  	list_for_each_entry_safe(page, next, &vb->free_page_list, lru) {
>  		list_del(&page->lru);
>  		__free_pages(page, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
> @@ -490,11 +490,27 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
>  			break;
>  	}
>  	vb->num_free_page_blocks -= num_returned;
> -	spin_unlock_irq(&vb->free_page_list_lock);
>  
>  	return num_returned;
>  }
>  
> +/* Gives back @num_to_return blocks of free pages to mm. */
> +static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
> +					     unsigned long num_to_return)
> +{
> +	unsigned long ret;
> +
> +	spin_lock_irq(&vb->free_page_list_lock);
> +	if (vb->suspended) {
> +		spin_unlock_irq(&vb->free_page_list_lock);
> +		return 0;
> +	}
> +	ret = __return_free_pages_to_mm(vb, num_to_return);
> +	spin_unlock_irq(&vb->free_page_list_lock);
> +
> +	return ret;
> +}
> +
>  static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
>  {
>  	if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> @@ -871,6 +887,9 @@ static unsigned long virtio_balloon_shrinker_scan(struct shrinker *shrinker,
>  {
>  	struct virtio_balloon *vb = shrinker->private_data;
>  
> +	if (READ_ONCE(vb->suspended))
> +		return 0;
> +

what if vb->suspended is set right here?

>  	return shrink_free_pages(vb, sc->nr_to_scan);
>  }
>  
> @@ -879,6 +898,9 @@ static unsigned long virtio_balloon_shrinker_count(struct shrinker *shrinker,
>  {
>  	struct virtio_balloon *vb = shrinker->private_data;
>  
> +	if (READ_ONCE(vb->suspended))
> +		return 0;
> +

or here?

>  	return vb->num_free_page_blocks * VIRTIO_BALLOON_HINT_BLOCK_PAGES;
>  }
>  
> @@ -1089,8 +1111,11 @@ static void remove_common(struct virtio_balloon *vb)
>  	update_balloon_size(vb);
>  
>  	/* There might be free pages that are being reported: release them. */
> -	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> -		return_free_pages_to_mm(vb, ULONG_MAX);
> +	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> +		spin_lock_irq(&vb->free_page_list_lock);
> +		__return_free_pages_to_mm(vb, ULONG_MAX);
> +		spin_unlock_irq(&vb->free_page_list_lock);

this is remove, taking locks seems futile - if something will acquire
the lock here, we will be in trouble.

> +	}

>  
>  	/* Now we reset the device so we can clean up the queues. */
>  	virtio_reset_device(vb->vdev);
> @@ -1133,6 +1158,10 @@ static int virtballoon_freeze(struct virtio_device *vdev)
>  	 * The workqueue is already frozen by the PM core before this
>  	 * function is called.
>  	 */
> +	spin_lock_irq(&vb->free_page_list_lock);
> +	WRITE_ONCE(vb->suspended, true);
> +	spin_unlock_irq(&vb->free_page_list_lock);
> +
>  	remove_common(vb);
>  	return 0;
>  }
> @@ -1148,6 +1177,10 @@ static int virtballoon_restore(struct virtio_device *vdev)
>  
>  	virtio_device_ready(vdev);
>  
> +	spin_lock_irq(&vb->free_page_list_lock);
> +	WRITE_ONCE(vb->suspended, false);
> +	spin_unlock_irq(&vb->free_page_list_lock);
> +
>  	if (towards_target(vb))
>  		virtballoon_changed(vdev);
>  	update_balloon_size(vb);
> -- 
> 2.55.0.229.g6434b31f56-goog