On 10/24/2017 06:44 AM, Joonsoo Kim wrote: >>> I'm not sure what is the confusing semantic you mentioned. I think >>> that set_migratetype_isolate() has confusing semantic and should be >>> fixed since making the pageblock isolated doesn't need to check if >>> there is unmovable page or not. Do you think that >>> set_migratetype_isolate() need to check it? If so, why? >> >> My intuitive understanding of set_migratetype_isolate is that it either >> suceeds and that means that the given pfn range can be isolated for the >> given type of allocation (be it movable or cma). No new pages will be >> allocated from this range to allow converging into a free range in a >> finit amount of time. At least this is how the hotplug code would like >> to use it and I suppose that the alloc_contig_range would like to >> guarantee the same to not rely on a fixed amount of migration attempts. > > Yes, alloc_contig_range() also want to guarantee the similar thing. > Major difference between them is 'given pfn range'. memory hotplug > works by pageblock unit but alloc_contig_range() doesn't. > alloc_contig_range() works by the page unit. However, there is no easy > way to isolate individual page so it uses pageblock isolation > regardless of 'given pfn range'. In this case, checking movability of > all pages on the pageblock would cause the problem as I mentioned > before.
I couldn't look too closely yet, but do I understand correctly that the *potential* problem (because as you say there are no such alloc_contig_range callers) you are describing is not newly introduced by Michal's series? Then his patch fixing the introduced regression should be enough for now, and further improvements could be posted on top, and not vice versa? Please don't take it wrong, I agree the current state is a bit of a mess and improvements are welcome. Also it seems to me that Michal is right, and there's nothing preventing alloc_contig_range() to allocate from CMA pageblocks for non-CMA purposes (likely not movable), and that should be also fixed? Vlastimil
