quoted

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

quoted

  {
-	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.

quoted

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

quoted

+		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.

quoted

+		__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.

quoted

+	}

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.
[...]

quoted

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