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) <david@kernel.org>
---
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)
{
- 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. */
- 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. */
+ 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. */
+ __count_vm_event(BALLOON_DEFLATE);
+ break;
+ default:
+ return rc;
+ }
+
+ 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);
+
+ return 0;
}
const struct movable_operations balloon_mops = {
--
2.52.0On 1/15/26 10:19, 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) <david@kernel.org>
> ---
Andrew, the following on top:
From 4c8b4f0aba5859a4ec71c7449a98b10e0547237f Mon Sep 17 00:00:00 2001
From: "David Hildenbrand (Red Hat)" <david@kernel.org>
Date: Mon, 19 Jan 2026 23:20:41 +0100
Subject: [PATCH] fixup: mm/balloon_compaction: centralize basic page migration
handling
Remove newline, talk about "page" instead of "old page" and avoid the
switch.
Signed-off-by: David Hildenbrand (Red Hat) <david@kernel.org>
---
mm/balloon_compaction.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
index 5444c61bb9e76..b859411811d0b 100644
--- a/mm/balloon_compaction.c
+++ b/mm/balloon_compaction.c
@@ -247,29 +247,21 @@ static int balloon_page_migrate(struct page *newpage, struct page *page,
return -EAGAIN;
rc = b_dev_info->migratepage(b_dev_info, newpage, page, mode);
- switch (rc) {
- case 0:
- spin_lock_irqsave(&b_dev_info->pages_lock, flags);
+ if (rc < 0 && rc != -ENOENT)
+ return rc;
+ spin_lock_irqsave(&b_dev_info->pages_lock, flags);
+ if (!rc) {
/* Insert the new page into the balloon list. */
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. */
- __count_vm_event(BALLOON_DEFLATE);
- break;
- default:
- return rc;
}
-
b_dev_info->isolated_pages--;
spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
+ /* If -ENOENT, page was deflated but new page not inflated. */
+ __count_vm_event(rc ? BALLOON_DEFLATE : BALLOON_MIGRATE);
+
/* Free the now-deflated page we isolated in balloon_page_isolate(). */
balloon_page_finalize(page);
put_page(page);
--
2.52.0
--
Cheers
David
On 1/19/26 23:22, David Hildenbrand (Red Hat) wrote: > On 1/15/26 10:19, 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) <david@kernel.org> >> --- > > Andrew, the following on top: Ah no, I'll rather resend the whole thing, as it creates some conflicts in the other patches. -- Cheers David
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) <david@kernel.org>
> ---
> 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
>> #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?
Yeah, likely it should be made consistent. But not as part of this patch
series :)
In particular, as we should be making all other things, like
balloon_dev_info's migratepage and the ones implementing it use the same
terminology in the same go.
On the TODO list.
>
>> {
>> - 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?
The page is isolated, as documented for "struct movable_operations". And
as the comment states, isolated pages cannot deflate.
So consequently, if we reach this point, we still have a balloon device,
because the balloon device could not have deflated the page.
I don't really want to change the comment as part of this change here,
it logically does not belong into this patch.
Maybe something for a cleanup patch:
"When we isolated the page, the page was inflated in a balloon device.
As isolated balloon pages cannot get deflated, we still have a balloon
device here."
>
> 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.
When a page is in the list we have to grab a reference. No strong
opinion about dropping the newline.
>
>> + 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?
I can strip the "Old" for now, but dst vs. src will be handled separately.
>
>> + __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?
A non-0 return value from balloon_page_migrate() means that migration
failed and that the (src) page stays isolated.
For example, migration core can later retry migration without re-isolation.
So the migration-core takes care of this.
>
> Ah I guess baloon_page_putback() would be invoked :) Fun!
Right, the isolated page has to be putback later.
>
>> + }
>
> 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 :)
You summarized my thoughts well ;)
I had exactly the thing you write below before I converted to switch. I
didn't particularly like the filtering for return codes. Let me think
about whether I want to go back.
As you note, it's highly subjective.
[...]
>
>> +
>> + 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?
Right, the (old)/page source was deflated, so we prepare for handing it
back to the buddy.
In the future, once these pages are frozen, migration-core will likely
take care of doing the freeing, instead of us doing the put_page() here.
One goal of this patch set was to move the getting/putting of pages out
as far as possible, such that the return values from
isolate/migrate/putback later on indicate who now "owns" the reference
to the frozen page.
Thanks for the review!
--
Cheers
David
© 2016 - 2026 Red Hat, Inc.