On 2017/10/20 10:13, Joonsoo Kim wrote: > On Thu, Oct 19, 2017 at 02:21:18PM +0200, Michal Hocko wrote: >> On Thu 19-10-17 10:20:41, Michal Hocko wrote: >>> On Thu 19-10-17 16:33:56, Joonsoo Kim wrote: >>>> On Thu, Oct 19, 2017 at 09:15:03AM +0200, Michal Hocko wrote: >>>>> On Thu 19-10-17 11:51:11, Joonsoo Kim wrote: >>> [...] >>>>>> Hello, >>>>>> >>>>>> This patch will break the CMA user. As you mentioned, CMA allocation >>>>>> itself isn't migrateable. So, after a single page is allocated through >>>>>> CMA allocation, has_unmovable_pages() will return true for this >>>>>> pageblock. Then, futher CMA allocation request to this pageblock will >>>>>> fail because it requires isolating the pageblock. >>>>> >>>>> Hmm, does this mean that the CMA allocation path depends on >>>>> has_unmovable_pages to return false here even though the memory is not >>>>> movable? This sounds really strange to me and kind of abuse of this >>>> >>>> Your understanding is correct. Perhaps, abuse or wrong function name. >>>> >>>>> function. Which path is that? Can we do the migrate type test theres? >>>> >>>> alloc_contig_range() -> start_isolate_page_range() -> >>>> set_migratetype_isolate() -> has_unmovable_pages() >>> >>> I see. It seems that the CMA and memory hotplug have a very different >>> view on what should happen during isolation. >>> >>>> We can add one argument, 'XXX' to set_migratetype_isolate() and change >>>> it to check migrate type rather than has_unmovable_pages() if 'XXX' is >>>> specified. >>> >>> Can we use the migratetype argument and do the special thing for >>> MIGRATE_CMA? Like the following diff? >> >> And with the full changelog. >> --- >> >From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001 >> From: Michal Hocko <mhocko@xxxxxxxx> >> Date: Thu, 19 Oct 2017 14:14:02 +0200 >> Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in >> has_unmovable_pages >> >> Joonsoo has noticed that "mm: drop migrate type checks from >> has_unmovable_pages" would break CMA allocator because it relies on >> has_unmovable_pages returning false even for CMA pageblocks which in >> fact don't have to be movable: >> alloc_contig_range >> start_isolate_page_range >> set_migratetype_isolate >> has_unmovable_pages >> >> This is a result of the code sharing between CMA and memory hotplug >> while each one has a different idea of what has_unmovable_pages should >> return. This is unfortunate but fixing it properly would require a lot >> of code duplication. >> >> Fix the issue by introducing the requested migrate type argument >> and special case MIGRATE_CMA case where CMA page blocks are handled >> properly. This will work for memory hotplug because it requires >> MIGRATE_MOVABLE. > > Unfortunately, alloc_contig_range() can be called with > MIGRATE_MOVABLE so this patch cannot perfectly fix the problem. > > I did a more thinking and found that it's strange to check if there is > unmovable page in the pageblock during the set_migratetype_isolate(). > set_migratetype_isolate() should be just for setting the migratetype > of the pageblock. Checking other things should be done by another > place, for example, before calling the start_isolate_page_range() in > __offline_pages(). > > Thanks. > Hi Joonsoo, How about add a flag to skip or not has_unmovable_pages() in set_migratetype_isolate()? Something like the skip_hwpoisoned_pages. Thanks, Xishi Qiu > > . > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
