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

[David Hildenbrand (Red Hat)]

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