From: Minchan Kim <minchan@kernel.org>
To: Michal Hocko <mhocko@suse.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
linux-mm <linux-mm@kvack.org>,
LKML <linux-kernel@vger.kernel.org>,
joaodias@google.com, surenb@google.com, cgoldswo@codeaurora.org,
willy@infradead.org, david@redhat.com, vbabka@suse.cz,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 1/2] mm: disable LRU pagevec during the migration temporarily
Date: Fri, 5 Mar 2021 12:26:11 -0800 [thread overview]
Message-ID: <YEKT4+4nLjGaAHCx@google.com> (raw)
In-Reply-To: <YEJW+dzF9/BNIiqn@dhcp22.suse.cz>
On Fri, Mar 05, 2021 at 05:06:17PM +0100, Michal Hocko wrote:
> On Wed 03-03-21 12:23:22, Minchan Kim wrote:
> > On Wed, Mar 03, 2021 at 01:49:36PM +0100, Michal Hocko wrote:
> > > On Tue 02-03-21 13:09:48, Minchan Kim 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 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.
> > >
> > > Have you seen any improvement on the CMA allocation success rate?
> >
> > Unfortunately, the cma alloc failure rate with reasonable margin
> > of error is really hard to reproduce under real workload.
> > That's why I measured the soft metric instead of direct cma fail
> > under real workload(I don't want to make some adhoc artificial
> > benchmark and keep tunes system knobs until it could show
> > extremly exaggerated result to convice patch effect).
> >
> > Please say if you belive this work is pointless unless there is
> > stable data under reproducible scenario. I am happy to drop it.
>
> Well, I am not saying that this is pointless. In the end the resulting
> change is relatively small and it provides a useful functionality for
> other users (e.g. hotplug). That should be a sufficient justification.
Yub, that was my impression to worth upstreaming rather than keeping
downstream tree so made divergent.
>
> I was asking about CMA allocation success rate because that is a much
> more reasonable metric than how many times something has retried because
> retries can help to increase success rate and the patch doesn't really
> remove those. If you want to use number of retries as a metric then the
> average allocation latency would be more meaningful.
I believe the allocation latency would be pretty big and retrial part
would be marginal so doubt it's meaningful.
Let me send next revision with as-is descripion once I fix places
you pointed out.
Thanks.
next prev parent reply other threads:[~2021-03-05 20:26 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20210302210949.2440120-1-minchan@kernel.org>
2021-03-03 12:49 ` [PATCH 1/2] mm: disable LRU pagevec during the migration temporarily Michal Hocko
2021-03-03 20:23 ` Minchan Kim
2021-03-04 8:07 ` David Hildenbrand
2021-03-04 15:55 ` Minchan Kim
2021-03-05 16:06 ` Michal Hocko
2021-03-05 20:26 ` Minchan Kim [this message]
2021-03-03 13:38 ` kernel test robot
2021-03-03 15:11 ` kernel test robot
2021-03-03 18:12 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YEKT4+4nLjGaAHCx@google.com \
--to=minchan@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=cgoldswo@codeaurora.org \
--cc=david@redhat.com \
--cc=joaodias@google.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@suse.com \
--cc=surenb@google.com \
--cc=vbabka@suse.cz \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).