Re: [RFC Get rid of shrink code - memory-hotplug]

[Michal Hocko <mhocko@suse.com>]

On Tue 04-12-18 10:26:00, osalvador@suse.de 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