Re: [PATCH v2 2/4] mm: check for proper migrate type during isolation

On 05/22/2018 01:10 AM, Mike Kravetz wrote:

On 05/18/2018 03:32 AM, Vlastimil Babka wrote:

quoted

On 05/04/2018 01:29 AM, Mike Kravetz wrote:

quoted

The routine start_isolate_page_range and alloc_contig_range have
comments saying that migratetype must be either MIGRATE_MOVABLE or
MIGRATE_CMA.  However, this is not enforced.

Enforced, no. But if the pageblocks really were as such, it used to
shortcut has_unmovable_pages(). This was wrong and removed in
d7b236e10ced ("mm: drop migrate type checks from has_unmovable_pages")
plus 4da2ce250f98 ("mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages()").


  What is important is

quoted

that that all pageblocks in the range are of type migratetype.

                                               the same

quoted

This is because blocks will be set to migratetype on error.

Strictly speaking this is true only for the CMA case. For other cases,
the best thing actually would be to employ the same heuristics as page
allocation migratetype fallbacks, and count how many pages are free and
how many appear to be movable, see how steal_suitable_fallback() uses
the last parameter of move_freepages_block().

quoted

Add a boolean argument enforce_migratetype to the routine
start_isolate_page_range.  If set, it will check that all pageblocks
in the range have the passed migratetype.  Return -EINVAL is pageblock

                                                            if

quoted

is wrong type is found in range.

  of

quoted

A boolean is used for enforce_migratetype as there are two primary
users.  Contiguous range allocation which wants to enforce migration
type checking.  Memory offline (hotplug) which is not concerned about
type checking.

This is missing some high-level result. The end change is that CMA is
now enforcing. So we are making it more robust when it's called on
non-CMA pageblocks by mistake? (BTW I still do hope we can remove
MIGRATE_CMA soon after Joonsoo's ZONE_MOVABLE CMA conversion. Combined
with my suggestion above we could hopefully get rid of the migratetype
parameter completely instead of enforcing it?). Is this also a
preparation for introducing find_alloc_contig_pages() which will be
enforcing? (I guess, and will find out shortly, but it should be stated
here)

Thank you for looking at these patches Vlastimil.

My primary motivation for this patch was the 'error recovery' in
start_isolate_page_range.  It takes a range and attempts to set
all pageblocks to MIGRATE_ISOLATE.  If it encounters an error after
setting some blocks to isolate, it will 'clean up' by setting the
migrate type of previously modified blocks to the passed migratetype.

Right.

So, one possible side effect of an error in start_isolate_page_range
is that the migrate type of some pageblocks could be modified.  Thinking
about it more now, that may be OK.

It would be definitely OK if the migratetype was changed similarly as
steal_suitable_fallback() does it, as I've said above.

It just does not seem like the
right thing to do, especially with comments saying "migratetype must
be either MIGRATE_MOVABLE or MIGRATE_CMA".  I'm fine with leaving the
code as is and just cleaning up the comments if you think that may
be better.

That's also possible, especially when the code is restructured as I've
suggested in the other reply, which should significantly reduce the
amount of error recoveries.