On Thu, Jan 15, 2026 at 10:19:54AM +0100, David Hildenbrand (Red Hat) wrote:
> Let's update the balloon page references, the balloon page list, the
> BALLOON_MIGRATE counter and the isolated-pages counter in
> balloon_page_migrate(), after letting the balloon->migratepage()
> callback deal with the actual inflation+deflation.
>
> Note that we now perform the balloon list modifications outside of any
> implementation-specific locks: which is fine, there is nothing special
> about these page actions that the lock would be protecting.
>
> The old page is already no longer in the list (isolated) and the new page
> is not yet in the list.
>
> Let's use -ENOENT to communicate the special "inflation of new page
> failed after already deflating the old page" to balloon_page_migrate() so
> it can handle it accordingly.
>
> While at it, rename balloon->b_dev_info to make it match the other
> functions. Also, drop the comment above balloon_page_migrate(), which
> seems unnecessary.
>
> Signed-off-by: David Hildenbrand (Red Hat) <[email protected]>
> ---
> arch/powerpc/platforms/pseries/cmm.c | 16 ---------
> drivers/misc/vmw_balloon.c | 49 +++++-----------------------
> drivers/virtio/virtio_balloon.c | 12 -------
> mm/balloon_compaction.c | 37 ++++++++++++++++++---
> 4 files changed, 41 insertions(+), 73 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/cmm.c
> b/arch/powerpc/platforms/pseries/cmm.c
> index 9a6efbc80d2ad..15f873f733a41 100644
> --- a/arch/powerpc/platforms/pseries/cmm.c
> +++ b/arch/powerpc/platforms/pseries/cmm.c
> @@ -501,8 +501,6 @@ static int cmm_migratepage(struct balloon_dev_info
> *b_dev_info,
> struct page *newpage, struct page *page,
> enum migrate_mode mode)
> {
> - unsigned long flags;
> -
> /*
> * loan/"inflate" the newpage first.
> *
> @@ -517,9 +515,6 @@ static int cmm_migratepage(struct balloon_dev_info
> *b_dev_info,
> return -EBUSY;
> }
>
> - /* balloon page list reference */
> - get_page(newpage);
> -
> /*
> * When we migrate a page to a different zone, we have to fixup the
> * count of both involved zones as we adjusted the managed page count
> @@ -530,22 +525,11 @@ static int cmm_migratepage(struct balloon_dev_info
> *b_dev_info,
> adjust_managed_page_count(newpage, -1);
> }
>
> - spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> - balloon_page_insert(b_dev_info, newpage);
> - __count_vm_event(BALLOON_MIGRATE);
> - b_dev_info->isolated_pages--;
> - spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> -
> /*
> * activate/"deflate" the old page. We ignore any errors just like the
> * other callers.
> */
> plpar_page_set_active(page);
> -
> - balloon_page_finalize(page);
> - /* balloon page list reference */
> - put_page(page);
> -
> return 0;
> }
> #else /* CONFIG_BALLOON_COMPACTION */
> diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c
> index 07e60a4b846aa..52b8c0f1eead7 100644
> --- a/drivers/misc/vmw_balloon.c
> +++ b/drivers/misc/vmw_balloon.c
> @@ -1724,18 +1724,17 @@ static inline void vmballoon_debugfs_exit(struct
> vmballoon *b)
> * @page: a ballooned page that should be migrated.
> * @mode: migration mode, ignored.
> *
> - * This function is really open-coded, but that is according to the interface
> - * that balloon_compaction provides.
> - *
> * Return: zero on success, -EAGAIN when migration cannot be performed
> - * momentarily, and -EBUSY if migration failed and should be retried
> - * with that specific page.
> + * momentarily, -EBUSY if migration failed and should be retried
> + * with that specific page, and -ENOENT when deflating @page
> + * succeeded but inflating @newpage failed, effectively deflating
> + * the balloon.
> */
> static int vmballoon_migratepage(struct balloon_dev_info *b_dev_info,
> struct page *newpage, struct page *page,
> enum migrate_mode mode)
> {
> - unsigned long status, flags;
> + unsigned long status;
> struct vmballoon *b;
> int ret = 0;
>
> @@ -1773,14 +1772,6 @@ static int vmballoon_migratepage(struct
> balloon_dev_info *b_dev_info,
> goto out_unlock;
> }
>
> - /*
> - * The page is isolated, so it is safe to delete it without holding
> - * @pages_lock . We keep holding @comm_lock since we will need it in a
> - * second.
> - */
> - balloon_page_finalize(page);
> - put_page(page);
> -
> /* Inflate */
> vmballoon_add_page(b, 0, newpage);
> status = vmballoon_lock_op(b, 1, VMW_BALLOON_4K_PAGE,
> @@ -1799,36 +1790,12 @@ static int vmballoon_migratepage(struct
> balloon_dev_info *b_dev_info,
> * change.
> */
> atomic64_dec(&b->size);
> - } else {
> /*
> - * Success. Take a reference for the page, and we will add it to
> - * the list after acquiring the lock.
> + * Tell the core that we're deflating the old page and don't
> + * need the new page.
> */
> - get_page(newpage);
> - }
> -
> - /* Update the balloon list under the @pages_lock */
> - spin_lock_irqsave(&b->b_dev_info.pages_lock, flags);
> -
> - /*
> - * On inflation success, we already took a reference for the @newpage.
> - * If we succeed just insert it to the list and update the statistics
> - * under the lock.
> - */
> - if (status == VMW_BALLOON_SUCCESS) {
> - balloon_page_insert(&b->b_dev_info, newpage);
> - __count_vm_event(BALLOON_MIGRATE);
> - } else {
> - __count_vm_event(BALLOON_DEFLATE);
> + ret = -ENOENT;
> }
> -
> - /*
> - * We deflated successfully, so regardless to the inflation success, we
> - * need to reduce the number of isolated_pages.
> - */
> - b->b_dev_info.isolated_pages--;
> - spin_unlock_irqrestore(&b->b_dev_info.pages_lock, flags);
> -
> out_unlock:
> up_read(&b->conf_sem);
> return ret;
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 74fe59f5a78c6..df2756c071dae 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -827,7 +827,6 @@ static int virtballoon_migratepage(struct
> balloon_dev_info *vb_dev_info,
> {
> struct virtio_balloon *vb = container_of(vb_dev_info,
> struct virtio_balloon, vb_dev_info);
> - unsigned long flags;
>
> /*
> * In order to avoid lock contention while migrating pages concurrently
> @@ -840,8 +839,6 @@ static int virtballoon_migratepage(struct
> balloon_dev_info *vb_dev_info,
> if (!mutex_trylock(&vb->balloon_lock))
> return -EAGAIN;
>
> - get_page(newpage); /* balloon reference */
> -
> /*
> * When we migrate a page to a different zone and adjusted the
> * managed page count when inflating, we have to fixup the count of
> @@ -854,11 +851,6 @@ static int virtballoon_migratepage(struct
> balloon_dev_info *vb_dev_info,
> }
>
> /* balloon's page migration 1st step -- inflate "newpage" */
> - spin_lock_irqsave(&vb_dev_info->pages_lock, flags);
> - balloon_page_insert(vb_dev_info, newpage);
> - vb_dev_info->isolated_pages--;
> - __count_vm_event(BALLOON_MIGRATE);
> - spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
> vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> set_page_pfns(vb, vb->pfns, newpage);
> tell_host(vb, vb->inflate_vq);
> @@ -869,10 +861,6 @@ static int virtballoon_migratepage(struct
> balloon_dev_info *vb_dev_info,
> tell_host(vb, vb->deflate_vq);
>
> mutex_unlock(&vb->balloon_lock);
> -
> - balloon_page_finalize(page);
> - put_page(page); /* balloon reference */
> -
> return 0;
> }
> #endif /* CONFIG_BALLOON_COMPACTION */
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index 03c5dbabb1565..5444c61bb9e76 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -232,20 +232,49 @@ static void balloon_page_putback(struct page *page)
> spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> }
>
> -/* move_to_new_page() counterpart for a ballooned page */
> static int balloon_page_migrate(struct page *newpage, struct page *page,
> enum migrate_mode mode)
I honestly wonder if page should be 'oldpage', or rather we should just match
args to the struct movable_operations e.g. dst, src?
> {
> - struct balloon_dev_info *balloon = balloon_page_device(page);
> + struct balloon_dev_info *b_dev_info = balloon_page_device(page);
> + unsigned long flags;
> + int rc;
>
> VM_BUG_ON_PAGE(!PageLocked(page), page);
> VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
>
> /* Isolated balloon pages cannot get deflated. */
Hmm, I'm a bit confused by this comment, isn't 'page' isolated?
This comment reads like !b_dev_info implies page isolated and thus a
WARN_ON_ONCE() issue, but later you say 'Free the now-deflated page we isolated
in balloon_page_isolate().' in reference to page?
So both can't be true.
> - if (WARN_ON_ONCE(!balloon))
> + if (WARN_ON_ONCE(!b_dev_info))
> return -EAGAIN;
>
> - return balloon->migratepage(balloon, newpage, page, mode);
> + rc = b_dev_info->migratepage(b_dev_info, newpage, page, mode);
> + switch (rc) {
> + case 0:
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> +
> + /* Insert the new page into the balloon list. */
Slightly weird to put this comment next to the pageref update then a newline
hten the actual insertion bit.
> + get_page(newpage);
> +
> + balloon_page_insert(b_dev_info, newpage);
> + __count_vm_event(BALLOON_MIGRATE);
> + break;
> + case -ENOENT:
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> +
> + /* Old page was deflated but new page not inflated. */
Weird reference to old page and new page when old page is 'page', with dst, src
we could just say destination/source?
> + __count_vm_event(BALLOON_DEFLATE);
> + break;
> + default:
> + return rc;
Don't we need to change the isolate stats etc. if we simply fail here? Or does
the movable ops logic correctly handle this for us?
Ah I guess baloon_page_putback() would be invoked :) Fun!
> + }
It's subjective and pedantic but I don't love this use of the switch here, it
really makes it seem like 'just another case' to do the _key_ action here of
migrating a balloon page. Also could compress things a bit, that's even more
subjective :)
Also it's kind of horrible to have the spin lock line duplicated like that,
that's more important and not clear on quick glance to see whether matching
lock/unlock.
So maybe change to something like:
rc = b_dev_info->migratepage(b_dev_info, newpage, page, mode);
if (rc < 0 && rc != -ENOENT)
return rc;
spin_lock_irqsave(&b_dev_info->pages_lock, flags);
if (rc == -ENOENT) {
/* Old page was deflated but new page not inflated. */
__count_vm_event(BALLOON_DEFLATE);
} else {
get_page(newpage);
/* Insert the new page into the balloon list. */
balloon_page_insert(b_dev_info, newpage);
__count_vm_event(BALLOON_MIGRATE);
}
Or even could be:
rc = b_dev_info->migratepage(b_dev_info, newpage, page, mode);
if (rc < 0 && rc != -ENOENT)
return rc;
spin_lock_irqsave(&b_dev_info->pages_lock, flags);
b_dev_info->isolated_pages--;
if (!rc) {
get_page(newpage);
/* Insert the new page into the balloon list. */
balloon_page_insert(b_dev_info, newpage);
__count_vm_event(BALLOON_MIGRATE);
}
spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
/* If -ENOENT, old page was deflated but new page not inflated. */
__count_vm_event(rc ? BALLOON_DEFLATE : BALLOON_MIGRATE);
To only lock over the operations that actually need it and to really highlight
the 'success' path?
> +
> + b_dev_info->isolated_pages--;
> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> +
> + /* Free the now-deflated page we isolated in balloon_page_isolate(). */
> + balloon_page_finalize(page);
> + put_page(page);
OK so we get on migrate, but put the source page which would have got gotten
previously I guess?
> +
> + return 0;
> }
>
> const struct movable_operations balloon_mops = {
> --
> 2.52.0
>
Thanks, Lorenzo
