Re: [PATCH] mm,page_alloc,cma: conditionally prefer cma pageblocks for movable allocations

From: Roman Gushchin <guro@fb.com>
To: Joonsoo Kim <js1304@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Vlastimil Babka <vbabka@suse.cz>, Rik van Riel <riel@surriel.com>,
	Linux Memory Management List <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>, <kernel-team@fb.com>,
	Qian Cai <cai@lca.pw>, Mel Gorman <mgorman@techsingularity.net>,
	Anshuman Khandual <anshuman.khandual@arm.com>,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>
Subject: Re: [PATCH] mm,page_alloc,cma: conditionally prefer cma pageblocks for movable allocations
Date: Fri, 3 Apr 2020 10:50:08 -0700	[thread overview]
Message-ID: <20200403174559.GC220160@carbon.lan> (raw)
In-Reply-To: <CAAmzW4M7cGmvssfmT5kmeZNOiQfbZP6N71TRHBq0BDtWqi78=g@mail.gmail.com>

On Fri, Apr 03, 2020 at 01:34:45PM +0900, Joonsoo Kim wrote:
> 2020년 4월 3일 (금) 오전 4:42, Roman Gushchin <guro@fb.com>님이 작성:
> > > In fact, I've tested this patch and your fixes for migration problem
> > > and found that there is
> > > still migration problem and failure rate is increased by this patch.
> >
> > Do you mind sharing any details? What kind of pages are those?
> 
> I don't investigate more since I had not enough time to do. If I
> remember correctly,
> it's the page used by journaling. I attach my test script below to
> help you reproduce it.
> My test setup is:
> - virtual machine, 8 cpus and 1024 MB mem (256 MB cma mem)
> - ubuntu 16.04 with custom kernel
> - filesystem is ext4
> 
> > I'm using the following patch to dump failed pages:

Thank you! I'll take a look.

> >
> > @@ -1455,6 +1455,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> >                                                 private, page, pass > 2, mode,
> >                                                 reason);
> >
> > +                       if (rc && reason == MR_CONTIG_RANGE)
> > +                               dump_page(page, "unmap_and_move");
> > +
> >                         switch(rc) {
> >                         case -ENOMEM:
> >                                 /*
> >
> >
> > > However, given that
> > > there is no progress on this area for a long time, I think that
> > > applying the change aggressively
> > > is required to break the current situation.
> >
> > I totally agree!
> >
> > Btw, I've found that cma_release() grabs the cma->lock mutex,
> > so it can't be called from the atomic context (I've got a lockdep warning).
> >
> > Of course, I can change the calling side, but I think it's better to change
> > the cma code to make cma_release() more accepting. What do you think
> > about the following patch?
> 
> For 2GB CMA area, we need to check 8192(?) bytes in worst case scenario and
> I don't think it's small enough for spinlock. Even, there is no limit
> on the size of
> the cma area. If cma area is bigger, it takes more. So, I think that
> spinlock() isn't
> good here.

Ok, I'll try to implement the other approach.

Thanks!

Roman