#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
