Re: [PATCH v1 02/29] mm/balloon_compaction: convert balloon_page_delete() to balloon_page_finalize()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Mon, Jun 30, 2025 at 02:59:43PM +0200, David Hildenbrand wrote:
> Let's move the removal of the page from the balloon list into the single
> caller, to remove the dependency on the PG_isolated flag and clarify
> locking requirements.
>
> We'll shuffle the operations a bit such that they logically make more sense
> (e.g., remove from the list before clearing flags).
>
> In balloon migration functions we can now move the balloon_page_finalize()
> out of the balloon lock and perform the finalization just before dropping
> the balloon reference.
>
> Document that the page lock is currently required when modifying the
> movability aspects of a page; hopefully we can soon decouple this from the
> page lock.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/powerpc/platforms/pseries/cmm.c |  2 +-
>  drivers/misc/vmw_balloon.c           |  3 +-
>  drivers/virtio/virtio_balloon.c      |  4 +--
>  include/linux/balloon_compaction.h   | 43 +++++++++++-----------------
>  mm/balloon_compaction.c              |  3 +-
>  5 files changed, 21 insertions(+), 34 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/cmm.c b/arch/powerpc/platforms/pseries/cmm.c
> index 5f4037c1d7fe8..5e0a718d1be7b 100644
> --- a/arch/powerpc/platforms/pseries/cmm.c
> +++ b/arch/powerpc/platforms/pseries/cmm.c
> @@ -532,7 +532,6 @@ static int cmm_migratepage(struct balloon_dev_info *b_dev_info,
>
>  	spin_lock_irqsave(&b_dev_info->pages_lock, flags);
>  	balloon_page_insert(b_dev_info, newpage);
> -	balloon_page_delete(page);

We seem to just be removing this and not replacing with finalize, is this right?

>  	b_dev_info->isolated_pages--;
>  	spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
>
> @@ -542,6 +541,7 @@ static int cmm_migratepage(struct balloon_dev_info *b_dev_info,
>  	 */
>  	plpar_page_set_active(page);
>
> +	balloon_page_finalize(page);
>  	/* balloon page list reference */
>  	put_page(page);
>
> diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c
> index c817d8c216413..6653fc53c951c 100644
> --- a/drivers/misc/vmw_balloon.c
> +++ b/drivers/misc/vmw_balloon.c
> @@ -1778,8 +1778,7 @@ static int vmballoon_migratepage(struct balloon_dev_info *b_dev_info,
>  	 * @pages_lock . We keep holding @comm_lock since we will need it in a
>  	 * second.
>  	 */
> -	balloon_page_delete(page);
> -
> +	balloon_page_finalize(page);
>  	put_page(page);
>
>  	/* Inflate */
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 89da052f4f687..e299e18346a30 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -866,15 +866,13 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
>  	tell_host(vb, vb->inflate_vq);
>
>  	/* balloon's page migration 2nd step -- deflate "page" */
> -	spin_lock_irqsave(&vb_dev_info->pages_lock, flags);
> -	balloon_page_delete(page);
> -	spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
>  	vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
>  	set_page_pfns(vb, vb->pfns, page);
>  	tell_host(vb, vb->deflate_vq);
>
>  	mutex_unlock(&vb->balloon_lock);
>
> +	balloon_page_finalize(page);
>  	put_page(page); /* balloon reference */
>
>  	return MIGRATEPAGE_SUCCESS;
> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> index 5ca2d56996201..b9f19da37b089 100644
> --- a/include/linux/balloon_compaction.h
> +++ b/include/linux/balloon_compaction.h
> @@ -97,27 +97,6 @@ static inline void balloon_page_insert(struct balloon_dev_info *balloon,
>  	list_add(&page->lru, &balloon->pages);
>  }
>
> -/*
> - * balloon_page_delete - delete a page from balloon's page list and clear
> - *			 the page->private assignement accordingly.
> - * @page    : page to be released from balloon's page list
> - *
> - * Caller must ensure the page is locked and the spin_lock protecting balloon
> - * pages list is held before deleting a page from the balloon device.
> - */
> -static inline void balloon_page_delete(struct page *page)
> -{
> -	__ClearPageOffline(page);
> -	__ClearPageMovable(page);
> -	set_page_private(page, 0);
> -	/*
> -	 * No touch page.lru field once @page has been isolated
> -	 * because VM is using the field.
> -	 */
> -	if (!PageIsolated(page))
> -		list_del(&page->lru);

I don't see this check elsewhere, is it because, as per the 1/xx of this series,
because by the time we do the finalize

> -}
> -
>  /*
>   * balloon_page_device - get the b_dev_info descriptor for the balloon device
>   *			 that enqueues the given page.
> @@ -141,12 +120,6 @@ static inline void balloon_page_insert(struct balloon_dev_info *balloon,
>  	list_add(&page->lru, &balloon->pages);
>  }
>
> -static inline void balloon_page_delete(struct page *page)
> -{
> -	__ClearPageOffline(page);
> -	list_del(&page->lru);
> -}
> -
>  static inline gfp_t balloon_mapping_gfp_mask(void)
>  {
>  	return GFP_HIGHUSER;
> @@ -154,6 +127,22 @@ static inline gfp_t balloon_mapping_gfp_mask(void)
>
>  #endif /* CONFIG_BALLOON_COMPACTION */
>
> +/*
> + * balloon_page_finalize - prepare a balloon page that was removed from the
> + *			   balloon list for release to the page allocator
> + * @page: page to be released to the page allocator
> + *
> + * Caller must ensure that the page is locked.

Can we assert this? Maybe mention that the balloon lock should not be held?

> + */
> +static inline void balloon_page_finalize(struct page *page)
> +{
> +	if (IS_ENABLED(CONFIG_BALLOON_COMPACTION)) {
> +		__ClearPageMovable(page);
> +		set_page_private(page, 0);
> +	}

Why do we check this? Is this function called from anywhere where that config won't be set?

> +	__ClearPageOffline(page);
> +}
> +
>  /*
>   * balloon_page_push - insert a page into a page list.
>   * @head : pointer to list
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index fcb60233aa35d..ec176bdb8a78b 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -94,7 +94,8 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info,
>  		if (!trylock_page(page))
>  			continue;
>
> -		balloon_page_delete(page);
> +		list_del(&page->lru);
> +		balloon_page_finalize(page);
>  		__count_vm_event(BALLOON_DEFLATE);
>  		list_add(&page->lru, pages);
>  		unlock_page(page);
> --
> 2.49.0
>