Re: [PATCH v3 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Oscar Salvador]

On Mon, Mar 01, 2021 at 03:09:06PM +0100, David Hildenbrand wrote:
> On 22.02.21 14:51, Oscar Salvador wrote:
> > @@ -905,6 +905,18 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> >   			valid_page = page;
> >   		}
> > +		if (PageHuge(page) && cc->alloc_contig) {
> > +			if (!isolate_or_dissolve_huge_page(page))
> > +				goto isolate_fail;

Bleh, sorry for the lateness David, I was farly busy.

> So, the callchain is:
> 
> alloc_contig_range()->__alloc_contig_migrate_range()->isolate_migratepages_range()->isolate_migratepages_block()
> 
> The case I am thinking about is if we run out of memory and would return
> -ENOMEM from alloc_and_dissolve_huge_page(). We silently drop the real error
> (e.g., -ENOMEM vs. -EBUSY vs. e.g., -EAGAIN) we had in
> isolate_or_dissolve_huge_page().

Yes, that is true.

> I think we should not swallo such return values in
> isolate_or_dissolve_huge_page() and instead properly report esp. -ENOMEM
> properly up this callchain now. Otherwise we'll end up retrying / reporting
> -EBUSY, which is misleading.

I am not sure I follow you here.
So, atm, alloc_and_dissolve_huge_page can either generate -ENOMEM or
-EBUSY wrt. error codes.
-ENOMEM when we cannot allocate a page, and -EBUSY when we raced with
someone.
You mean to only report ENOMEM down the chain?

> From isolate_migratepages_range()/isolate_migratepages_block() we'll keep
> reporting "pfn > 0".
> 
> a) In isolate_migratepages_range() we'll keep iterating over pageblocks
> although we should just fail with -ENOMEM right away.
> 
> b) In __alloc_contig_migrate_range() we'll keep retrying up to 5 times
> although we should just fail with -ENOMEM. We end up returning "-EBUSY"
> after retrying.
> 
> c) In alloc_contig_range() we'll continue trying to isolate although we
> should just return -ENOMEM.

Yes, "fatal" errors get masked, and hence we treat everything as "things
are busy, let us try again", and this is rather unforunate.

> I think we have should start returning proper errors from
> isolate_migratepages_range()/isolate_migratepages_block() on critical issues
> (-EINTR, -ENOMEM) instead of going via "!pfn vs. pfn" and retrying on "pfn".
> 
> So we should then fail with -ENOMEM during isolate_migratepages_range()
> cleanly, just as we would do when we get -ENOMEM during migrate_pages().

I guess we could rework the interface and make isolate_migratepages_range and
isolate_migratepages_block to report the right thing.
I yet have to check that this does not mess up a lot with the compaction
interface.

But overall I agree with your point here, and I am willing to to tackle
this.

The question is whether we want to do this as part of this series, or
after this series gets picked up.
IMHO, we could do it after, as a follow-up, unless you feel strong about
it.

What do you think?


-- 
Oscar Salvador
SUSE L3