Re: [PATCH v3 2/3] mm: disable LRU pagevec during the migration temporarily

[Michal Hocko]

On Wed 17-03-21 17:13:16, Andrew Morton wrote:
> On Wed, 10 Mar 2021 08:14:28 -0800 Minchan Kim <minchan@xxxxxxxxxx> wrote:
> 
> > LRU pagevec holds refcount of pages until the pagevec are drained.
> > It could prevent migration since the refcount of the page is greater
> > than the expection in migration logic. To mitigate the issue,
> > callers of migrate_pages drains LRU pagevec via migrate_prep or
> > lru_add_drain_all before migrate_pages call.
> > 
> > However, it's not enough because pages coming into pagevec after the
> > draining call still could stay at the pagevec so it could keep
> > preventing page migration. Since some callers of migrate_pages have
> > retrial logic with LRU draining, the page would migrate at next trail
> > but it is still fragile in that it doesn't close the fundamental race
> > between upcoming LRU pages into pagvec and migration so the migration
> > failure could cause contiguous memory allocation failure in the end.
> > 
> > To close the race, this patch disables lru caches(i.e, pagevec)
> > during ongoing migration until migrate is done.
> > 
> > Since it's really hard to reproduce, I measured how many times
> > migrate_pages retried with force mode(it is about a fallback to a
> > sync migration) with below debug code.
> > 
> > int migrate_pages(struct list_head *from, new_page_t get_new_page,
> > 			..
> > 			..
> > 
> > if (rc && reason == MR_CONTIG_RANGE && pass > 2) {
> >        printk(KERN_ERR, "pfn 0x%lx reason %d\n", page_to_pfn(page), rc);
> >        dump_page(page, "fail to migrate");
> > }
> > 
> > The test was repeating android apps launching with cma allocation
> > in background every five seconds. Total cma allocation count was
> > about 500 during the testing. With this patch, the dump_page count
> > was reduced from 400 to 30.
> > 
> > The new interface is also useful for memory hotplug which currently
> > drains lru pcp caches after each migration failure. This is rather
> > suboptimal as it has to disrupt others running during the operation.
> > With the new interface the operation happens only once. This is also in
> > line with pcp allocator cache which are disabled for the offlining as
> > well.
> > 
> 
> This is really a rather ugly thing, particularly from a maintainability
> point of view.  Are you sure you found all the sites which need the
> enable/disable?  How do we prevent new ones from creeping in which need
> the same treatment?  Is there some way of adding a runtime check which
> will trip if a conversion was missed?

I am not sure I am following. What is your concern here? This is a
lock-like interface to disable a certain optimization because it stands
in the way. Not using the interface is not a correctness problem.

If you refer to disable/enable interface and one potentially missing
enable for some reason then again this will not become a correctness
problem. It will result in a suboptimal behavior. So in the end this is
much less of a probel than leaving a lock behind.

The functionality is not exported to modules and I would agree that this
is not something for out of core/MM code to be used. We can hide it into
an internal mm header of you want?
-- 
Michal Hocko
SUSE Labs