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
