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

[David Hildenbrand]

> > 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?

Yes, fatal errors.

> >  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?

I think this is now the second fatal error we can have (-EINTR, 
-ENOMEM), thus the current interface (return "NULL" on fatal errros) no 
longer works properly.

No strong opinion about fixing this up on top - could be it's cleaner to 
just do it right away.

--
Thanks,

David / dhildenb