Re: [PATCH v2 04/23] mm/balloon_compaction: centralize basic page migration handling

David Hildenbrand (Red Hat)
2026-01-15T12:58:01Z

>> #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