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

["David Hildenbrand (Red Hat)" <david@kernel.org>]

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