Re: [PATCH 6/12] mm: page migration use the put_new_page whenever necessary

[Hugh Dickins <hughd@google.com>]

On Thu, 5 Nov 2015, Vlastimil Babka wrote:
> On 10/19/2015 06:57 AM, Hugh Dickins wrote:
> > I don't know of any problem from the way it's used in our current tree,
> > but there is one defect in page migration's custom put_new_page feature.
> > 
> > An unused newpage is expected to be released with the put_new_page(),
> > but there was one MIGRATEPAGE_SUCCESS (0) path which released it with
> > putback_lru_page(): which can be very wrong for a custom pool.
> 
> I'm a bit confused. So there's no immediate bug to be fixed but there was one in
> the mainline in the past? Or elsewhere?

"elsewhere": I came across it (and several of the other issues addressed
in this patchset) when using migrate_pages() in my huge tmpfs work.

I admit that, until I came to reply to you, I had thought this oversight
resulted in a (minor) unintended inefficiency in compaction - still the
sole user of the put_new_page feature in current mainline.  I thought it
was permitting a waste of effort of the kind that put_new_page was added
to stop.

But that's not so: because it's limited to (the page_count 1 case of)
MIGRATEPAGE_SUCCESS, migrate_pages() will not retry on the old page, so
it does not matter that its migration target is diverted to the public
pool, instead of back to the private pool.

At least, I think it barely matters; but using putback_lru_page does
miss out on the "pfn > high_pfn" check in release_freepages().

> 
> > Fixed more easily by resetting put_new_page once it won't be needed,
> > than by adding a further flag to modify the rc test.
> 
> What is "fixed" if there is no bug? :) Maybe "Further bugs would be
> prevented..." or something?

I never claimed a "critical bug" there, nor asked for this to go to
stable.  I think it's fair to describe something as a "bug", where a
design is not quite working as it had intended; though "defect" is the
word I actually used.  It's reasonable to "fix" a "defect", isn't it?

> 
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> 
> I agree it's less error-prone after you patch, so:
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

Thanks, and for your other scrutiny and Acks too.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>