On Tue 04-12-18 10:26:00, osalvador@xxxxxxx wrote: [...] > __pageblock_pfn_to_page()->pfn_to_online_page() > In case zone->contiguous, we just return with pfn_to_page(). > So we just need to make sure that zone->contiguous has the right value. Or we can consider removing this optimization altogether. THe only consumer is compaction and I do not see this to be a hot path. > * create_mem_extents > - What? I have crossed that code as well and I cannot say I follow. Anyway I suspect we should be safe after saveable_*_page uses pfn_to_online_page is used and David has laready sent a patch for that. > * vmemmap_find_next_valid_pfn() > - I am not really sure if this represents a problem git grep doesn't see any user of this function. > * kmemleak_scan() > - It is ok, but I think we should check for the pfn to belong to the node > here? This wants to use pfn_to_online_page and chech the node id. Checking the node_id would be an optimization because interleaving nodes would check the same pfn multiple times. > * Crash core: > - VMCOREINFO_OFFSET(pglist_data, node_start_pfn) is this a problem? My understanding in this area is very minimal. But I do not see this to be a problem. Crash, as the only consumer should have to handle offline holes somehow anyway. > > * lookup_page_ext() > - For !CONFIG_SPARSEMEM, node_start_pfn is used. HOTPLUG is SPARSEMEM only > So overall, besides a couple of places I am not sure it would cause trouble, > I would tend to say this is doable. Cool! Thanks for crawling all that code. This must have been really tedious. > Another thing that needs remark is that Patchset [3] aims for not > touching pages during hot-remove path, so we will have to find another > way to trigger clear/set_zone_contiguous, but that is another topic. I still didn't get around to look into that one, sorry. > While it is true that the current shrink code can be simplified as > showed in [2], I think that getting rid of it would be a nice thing to > do unless we need to keep the code around. Absolutely agreed. > I would like to hear other opinions though. > Is it too risky? Is there anything I overlooked that might cause trouble? > Did I miss anything? No, go ahead. This is a relict from the nasty zone shifting times. The more code we can get rid of the better. > [1] https://patchwork.kernel.org/patch/10700791/ > [2] https://patchwork.kernel.org/patch/10700791/ > [3] https://patchwork.kernel.org/cover/10700783/ Thanks a lot! -- Michal Hocko SUSE Labs
