On Wed 02-05-18 14:13:32, Mike Kravetz wrote:

On 04/21/2018 09:16 AM, Vlastimil Babka wrote:

quoted

On 04/17/2018 04:09 AM, Mike Kravetz wrote:

quoted

find_alloc_contig_pages() is a new interface that attempts to locate
and allocate a contiguous range of pages.  It is provided as a more
convenient interface than alloc_contig_range() which is currently
used by CMA and gigantic huge pages.

When attempting to allocate a range of pages, migration is employed
if possible.  There is no guarantee that the routine will succeed.
So, the user must be prepared for failure and have a fall back plan.

Signed-off-by: Mike Kravetz <redacted>

Hi, just two quick observations, maybe discussion pointers for the
LSF/MM session:
- it's weird that find_alloc_contig_pages() takes an order, and
free_contig_pages() takes a nr_pages. I suspect the interface would be
more future-proof with both using nr_pages? Perhaps also minimum
alignment for the allocation side? Order is fine for hugetlb, but what
about other potential users?

Agreed, and I am changing this to nr_pages and adding alignment.

quoted

- contig_alloc_migratetype_ok() says that MIGRATE_CMA blocks are OK to
allocate from. This silently assumes that everything allocated by this
will be migratable itself, or it might eat CMA reserves. Is it the case?
Also you then call alloc_contig_range() with MIGRATE_MOVABLE, so it will
skip/fail on MIGRATE_CMA anyway IIRC.

When looking closer at the code, alloc_contig_range currently has comments
saying migratetype must be MIGRATE_MOVABLE or MIGRATE_CMA.  However, this
is not checked/enforced anywhere in the code (that I can see).  The
migratetype passed to alloc_contig_range() will be used to set the migrate
type of all pageblocks in the range.  If there is an error, one side effect
is that some pageblocks may have their migrate type changed to migratetype.
Depending on how far we got before hitting the error, the number of pageblocks
changed is unknown.  This actually can happen at the lower level routine
start_isolate_page_range().

My first thought was to make start_isolate_page_range/set_migratetype_isolate
check that the migrate type of a pageblock was migratetype before isolating.
This would work for CMA, and I could make it work for the new allocator.
However, offline_pages also calls start_isolate_page_range and I believe we
do not want to enforce such a rule (all pageblocks must be of the same migrate
type) for memory hotplug/offline?

Should we be concerned at all about this potential changing of migrate type
on error?  The only way I can think to avoid this is to save the original
migrate type before isolation.

This is more a question to Vlastimil, Joonsoo. But my understanding is
that it doesn't matter. MIGRATE_MOVABLE will not block other
allocations. So we seem to need it only for MIGRATE_CMA. The later
should die sooner or later hopefully so this awful kludge should just
die with it.
-- 
Michal Hocko
SUSE Labs