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

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

[osalvador]

[Sorry, I forgot to cc linux-mm before]

Hi,

I wanted to bring up a topic that showed up during a discussion about
simplifying shrink code [1].
During that discussion, Michal suggested that we might be able
to get rid of the shrink code.

To put you on track, the shrink code was introduced by 815121d2b5cd
("memory_hotplug: clear zone when removing the memory") just to match
the work we did in __add_zone() and do the reverse thing there.

It is a nice thing to have as a) it keeps a zone/node boundaries strict
and b) we are consistent because we do the reverse operation than
move_pfn_range_to_zone.

But, I think that we can live without it:

     1) since c6f03e2903c9ecd8fd709a5b3fa8cf0a8ae0b3da
        ("mm, memory_hotplug: remove zone restrictions") we became more 
flexible
        and now we can have ZONE_NORMAL and ZONE_MOVABLE interleaved 
during hotplug.
        So keeping a strict zone boundary does not really make sense 
anymore.
        In the same way, we can also have interleaved nodes.

     2) From the point of view of a pfn walker, we should not care if the 
section
        removed was the first one, the last one, or some section 
in-between,
        as we should skip non-valid pfns.


When the topic arose, I was a bit worried because I was not sure if
anything out there would trust the node/zone boundaries blindly
without checking anything.
So I started to dig in to see who were the users of

- zone_start_pfn
- zone_end_pfn
- zone_intersects
- zone_spans_pfn
- node_start_pfn
- node_end_pfn

Below, there is a list with the places I found that use these
variables.
For the sake of simplicity, I left out the places where they are
only used during boot-time, as there is no danger in there.

=== ZONE related ===

[Usages of zone_start_pfn / zone_end_pfn]

  * split_huge_pages_set()
    - It uses pfn_valid()

  * alloc_gigantic_page()
    - It uses pfn_range_valid_gigantic()->pfn_valid()

  * pagetypeinfo_showblockcount_print()
    - It uses pfn_to_online_page()

  * mark_free_pages()
    - It uses pfn_valid()

  * __reset_isolation_suitable()
    - It uses pfn_to_online_page()

  * reset_cached_positions()

  * isolate_freepages_range()
  * isolate_migratepages_range()
  * isolate_migratepages()
    - They use pageblock_pfn_to_page()
      In case !zone->contiguous, we will call 
__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.

  * create_mem_extents
    - What?

  * count_highmem_pages:
    count_data_pages:
    copy_data_pages:
    - page_is_saveable()->pfn_valid()

[Usages of zone_spans_pfn]

  * move_freepages_block
  * set_pfnblock_flags_mask
  * page_outside_zone_boundaries
    - I would say this is safe, as, if anything, when removing the shrink 
code
      the system can think that we span more than we actually do, no the 
other
      way around.

[Usages of zone_intersects]

  * default_zone_for_pfn
    default_kernel_zone_for_pfn
    - It should not be a problem

=== NODE related ===

[Usages of node_start_pfn / node_end_pfn]

  * vmemmap_find_next_valid_pfn()
    - I am not really sure if this represents a problem

  * memtrace_alloc_node()
    - Should not have any problem as we currently support interleaved 
nodes.

  * kmemleak_scan()
    - It is ok, but I think we should check for the pfn to belong to the 
node here?

  * Crash core:
    - VMCOREINFO_OFFSET(pglist_data, node_start_pfn) is this a problem?

  * lookup_page_ext()
    - For !CONFIG_SPARSEMEM, node_start_pfn is used.

  * kcore_ram_list()
    - Safe, as kclist_add_private() uses pfn_valid.


So overall, besides a couple of places I am not sure it would cause 
trouble,
I would tend to say this is doable.

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.

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.

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?

[1] https://patchwork.kernel.org/patch/10700791/
[2] https://patchwork.kernel.org/patch/10700791/
[3] https://patchwork.kernel.org/cover/10700783/

Thanks
Oscar Salvador

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

[David Hildenbrand]

On 04.12.18 10:26, osalvador@suse.de wrote:
> [Sorry, I forgot to cc linux-mm before]
> 
> Hi,
> 
> I wanted to bring up a topic that showed up during a discussion about
> simplifying shrink code [1].
> During that discussion, Michal suggested that we might be able
> to get rid of the shrink code.
> 
> To put you on track, the shrink code was introduced by 815121d2b5cd
> ("memory_hotplug: clear zone when removing the memory") just to match
> the work we did in __add_zone() and do the reverse thing there.
> 
> It is a nice thing to have as a) it keeps a zone/node boundaries strict
> and b) we are consistent because we do the reverse operation than
> move_pfn_range_to_zone.
> 
> But, I think that we can live without it:
> 
>      1) since c6f03e2903c9ecd8fd709a5b3fa8cf0a8ae0b3da
>         ("mm, memory_hotplug: remove zone restrictions") we became more 
> flexible
>         and now we can have ZONE_NORMAL and ZONE_MOVABLE interleaved 
> during hotplug.
>         So keeping a strict zone boundary does not really make sense 
> anymore.
>         In the same way, we can also have interleaved nodes.
> 
>      2) From the point of view of a pfn walker, we should not care if the 
> section
>         removed was the first one, the last one, or some section 
> in-between,
>         as we should skip non-valid pfns.
> 
> 
> When the topic arose, I was a bit worried because I was not sure if
> anything out there would trust the node/zone boundaries blindly
> without checking anything.
> So I started to dig in to see who were the users of
> 
> - zone_start_pfn
> - zone_end_pfn
> - zone_intersects
> - zone_spans_pfn
> - node_start_pfn
> - node_end_pfn
> 
> Below, there is a list with the places I found that use these
> variables.
> For the sake of simplicity, I left out the places where they are
> only used during boot-time, as there is no danger in there.
> 
> === ZONE related ===
> 
> [Usages of zone_start_pfn / zone_end_pfn]
> 
>   * split_huge_pages_set()
>     - It uses pfn_valid()
> 
>   * alloc_gigantic_page()
>     - It uses pfn_range_valid_gigantic()->pfn_valid()
> 
>   * pagetypeinfo_showblockcount_print()
>     - It uses pfn_to_online_page()
> 
>   * mark_free_pages()
>     - It uses pfn_valid()
> 
>   * __reset_isolation_suitable()
>     - It uses pfn_to_online_page()
> 
>   * reset_cached_positions()
> 
>   * isolate_freepages_range()
>   * isolate_migratepages_range()
>   * isolate_migratepages()
>     - They use pageblock_pfn_to_page()
>       In case !zone->contiguous, we will call 
> __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.
> 
>   * create_mem_extents
>     - What?
> 
>   * count_highmem_pages:
>     count_data_pages:
>     copy_data_pages:
>     - page_is_saveable()->pfn_valid()
> 
> [Usages of zone_spans_pfn]
> 
>   * move_freepages_block
>   * set_pfnblock_flags_mask
>   * page_outside_zone_boundaries
>     - I would say this is safe, as, if anything, when removing the shrink 
> code
>       the system can think that we span more than we actually do, no the 
> other
>       way around.
> 
> [Usages of zone_intersects]
> 
>   * default_zone_for_pfn
>     default_kernel_zone_for_pfn
>     - It should not be a problem
> 
> === NODE related ===
> 
> [Usages of node_start_pfn / node_end_pfn]
> 
>   * vmemmap_find_next_valid_pfn()
>     - I am not really sure if this represents a problem
> 
>   * memtrace_alloc_node()
>     - Should not have any problem as we currently support interleaved 
> nodes.
> 
>   * kmemleak_scan()
>     - It is ok, but I think we should check for the pfn to belong to the 
> node here?
> 
>   * Crash core:
>     - VMCOREINFO_OFFSET(pglist_data, node_start_pfn) is this a problem?
> 
>   * lookup_page_ext()
>     - For !CONFIG_SPARSEMEM, node_start_pfn is used.
> 
>   * kcore_ram_list()
>     - Safe, as kclist_add_private() uses pfn_valid.
> 
> 
> So overall, besides a couple of places I am not sure it would cause 
> trouble,
> I would tend to say this is doable.
> 
> 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.

If I am not wrong, zone_contiguous is a pure mean for performance
improvement, right? So leaving zone_contiguous unset is always save. I
always disliked the whole clear/set_zone_contiguous thingy. I wonder if
we can find a different way to boost performance there (in the general
case). Or is this (zone_contiguous) even worth keeping around at all for
now? (do we have performance numbers?)

> 
> 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.
> 
> 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?

I'd say let's give it a try and find out if we are missing something. +1
to simplifying that code.

> 
> [1] https://patchwork.kernel.org/patch/10700791/
> [2] https://patchwork.kernel.org/patch/10700791/
> [3] https://patchwork.kernel.org/cover/10700783/
> 
> Thanks
> Oscar Salvador
> 


-- 

Thanks,

David / dhildenb

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

[osalvador]

On 2018-12-04 12:31, David Hildenbrand wrote:
  > If I am not wrong, zone_contiguous is a pure mean for performance
> improvement, right? So leaving zone_contiguous unset is always save. I
> always disliked the whole clear/set_zone_contiguous thingy. I wonder if
> we can find a different way to boost performance there (in the general
> case). Or is this (zone_contiguous) even worth keeping around at all 
> for
> now? (do we have performance numbers?)

It looks like it was introduced by 7cf91a98e607
("mm/compaction: speed up pageblock_pfn_to_page() when zone is 
contiguous").

The improve numbers are in the commit.
So I would say that we need to keep it around.


> I'd say let's give it a try and find out if we are missing something. 
> +1
> to simplifying that code.

I will work on a patch removing this and I will integrate it in [1].
Then I will run some tests to see if I can catch something bad.

[1] https://patchwork.kernel.org/cover/10700783/

Thanks for taking a look!

Oscar Salvador

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

[Michal Hocko]

[Cc Vlastimil]

On Tue 04-12-18 13:43:31, osalvador@suse.de wrote:
> On 2018-12-04 12:31, David Hildenbrand wrote:
>  > If I am not wrong, zone_contiguous is a pure mean for performance
> > improvement, right? So leaving zone_contiguous unset is always save. I
> > always disliked the whole clear/set_zone_contiguous thingy. I wonder if
> > we can find a different way to boost performance there (in the general
> > case). Or is this (zone_contiguous) even worth keeping around at all for
> > now? (do we have performance numbers?)
> 
> It looks like it was introduced by 7cf91a98e607
> ("mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous").
> 
> The improve numbers are in the commit.
> So I would say that we need to keep it around.

Is that still the case though?

-- 
Michal Hocko
SUSE Labs

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

[Vlastimil Babka]

On 12/5/18 8:12 PM, Michal Hocko wrote:
> [Cc Vlastimil]
> 
> On Tue 04-12-18 13:43:31, osalvador@suse.de wrote:
>> On 2018-12-04 12:31, David Hildenbrand wrote:
>>  > If I am not wrong, zone_contiguous is a pure mean for performance
>>> improvement, right? So leaving zone_contiguous unset is always save. I
>>> always disliked the whole clear/set_zone_contiguous thingy. I wonder if
>>> we can find a different way to boost performance there (in the general
>>> case). Or is this (zone_contiguous) even worth keeping around at all for
>>> now? (do we have performance numbers?)
>>
>> It looks like it was introduced by 7cf91a98e607
>> ("mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous").
>>
>> The improve numbers are in the commit.
>> So I would say that we need to keep it around.
> 
> Is that still the case though?

Well, __pageblock_pfn_to_page() has to be called for each pageblock in
compaction, when zone_contiguous is false. And that's unchanged since
the introduction of zone_contiguous, so the numbers should still hold.

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

[Michal Hocko]

On Fri 07-12-18 10:54:50, Vlastimil Babka wrote:
> On 12/5/18 8:12 PM, Michal Hocko wrote:
> > [Cc Vlastimil]
> > 
> > On Tue 04-12-18 13:43:31, osalvador@suse.de wrote:
> >> On 2018-12-04 12:31, David Hildenbrand wrote:
> >>  > If I am not wrong, zone_contiguous is a pure mean for performance
> >>> improvement, right? So leaving zone_contiguous unset is always save. I
> >>> always disliked the whole clear/set_zone_contiguous thingy. I wonder if
> >>> we can find a different way to boost performance there (in the general
> >>> case). Or is this (zone_contiguous) even worth keeping around at all for
> >>> now? (do we have performance numbers?)
> >>
> >> It looks like it was introduced by 7cf91a98e607
> >> ("mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous").
> >>
> >> The improve numbers are in the commit.
> >> So I would say that we need to keep it around.
> > 
> > Is that still the case though?
> 
> Well, __pageblock_pfn_to_page() has to be called for each pageblock in
> compaction, when zone_contiguous is false. And that's unchanged since
> the introduction of zone_contiguous, so the numbers should still hold.

OK, this means that we have to carefully re-evaluate zone_contiguous for
each offline operation.
 
Thanks!
-- 
Michal Hocko
SUSE Labs

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

[osalvador]

On 2018-12-07 11:32, Michal Hocko wrote:
> On Fri 07-12-18 10:54:50, Vlastimil Babka wrote:
>> Well, __pageblock_pfn_to_page() has to be called for each pageblock in
>> compaction, when zone_contiguous is false. And that's unchanged since
>> the introduction of zone_contiguous, so the numbers should still hold.
> 
> OK, this means that we have to carefully re-evaluate zone_contiguous 
> for
> each offline operation.

Yeah, it seems so.
I already thought about something, although I did not really think it 
through completely due to lack of time.
I expect to be able to start the patch early next week.

thanks Vlastimil for confirming ;-)

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

[osalvador]

On 2018-12-07 11:35, osalvador@suse.de wrote:
> On 2018-12-07 11:32, Michal Hocko wrote:
>> On Fri 07-12-18 10:54:50, Vlastimil Babka wrote:
>>> Well, __pageblock_pfn_to_page() has to be called for each pageblock 
>>> in
>>> compaction, when zone_contiguous is false. And that's unchanged since
>>> the introduction of zone_contiguous, so the numbers should still 
>>> hold.
>> 
>> OK, this means that we have to carefully re-evaluate zone_contiguous 
>> for
>> each offline operation.

I started to think about this but some questions arose.

Actually, no matter what we do, if we re-evaluate zone_contiguous
in the offline operation, zone_contiguous will be left unset.

set_zone_contiguous() calls __pageblock_pfn_to_page() in
pageblock_nr_pages chunks to determine if the zone is contiguous,
and checks the following:

* first/end pfn of the pageblock are valid sections
* first pfn of the pageblock is online
* we do not intersect different zones
* first/end pfn belong to the same zone

Now, when dropping the shrink code and re-evaluating zone_contiguous in 
offline
operation, set_zone_contiguous() will return false, leaving us with 
zone_contiguous
unset.

I wonder if we want that, or we want to stick with the optimization that
zone_contiguous brings us.

If we do not care, dropping everything and just calling 
clear_zone_contiguous and
set_zone_contiguous is the right thing to go.
But, if we want to keep the zone_contiguous optimization, I am afraid 
that we need
to re-adjust zone boundaries in the offline operation whenever we remove 
first/end
section.
In that case, set_zone_contiguous will still keep zone_contiguous as 
set.

So, it seems that the only headache we still have is this 
zone_contiguous thing.
Ideas? Suggestions?

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

[David Hildenbrand]

On 10.12.18 14:53, osalvador@suse.de wrote:
> On 2018-12-07 11:35, osalvador@suse.de wrote:
>> On 2018-12-07 11:32, Michal Hocko wrote:
>>> On Fri 07-12-18 10:54:50, Vlastimil Babka wrote:
>>>> Well, __pageblock_pfn_to_page() has to be called for each pageblock 
>>>> in
>>>> compaction, when zone_contiguous is false. And that's unchanged since
>>>> the introduction of zone_contiguous, so the numbers should still 
>>>> hold.
>>>
>>> OK, this means that we have to carefully re-evaluate zone_contiguous 
>>> for
>>> each offline operation.
> 
> I started to think about this but some questions arose.
> 
> Actually, no matter what we do, if we re-evaluate zone_contiguous
> in the offline operation, zone_contiguous will be left unset.
> 
> set_zone_contiguous() calls __pageblock_pfn_to_page() in
> pageblock_nr_pages chunks to determine if the zone is contiguous,
> and checks the following:
> 
> * first/end pfn of the pageblock are valid sections
> * first pfn of the pageblock is online
> * we do not intersect different zones
> * first/end pfn belong to the same zone
> 
> Now, when dropping the shrink code and re-evaluating zone_contiguous in 
> offline
> operation, set_zone_contiguous() will return false, leaving us with 
> zone_contiguous
> unset.
> 
> I wonder if we want that, or we want to stick with the optimization that
> zone_contiguous brings us.
> 
> If we do not care, dropping everything and just calling 
> clear_zone_contiguous and
> set_zone_contiguous is the right thing to go.
> But, if we want to keep the zone_contiguous optimization, I am afraid 
> that we need
> to re-adjust zone boundaries in the offline operation whenever we remove 
> first/end
> section.
> In that case, set_zone_contiguous will still keep zone_contiguous as 
> set.
> 
> So, it seems that the only headache we still have is this 
> zone_contiguous thing.
> Ideas? Suggestions?

At least for memory hot(un)plug we always online/offline complete
sections. And they all go to the same zone. However we check on
pageblock granularity. I would assume that we only have mixed zones on
one section for some corner cases sections in our system?

I wonder if we could optimize somehow differently. E.g. mark sections as
belonging completely to a zone. If not, check on pageblock granularity.

E.g. SECTION_SINGLE_ZONE

-- 

Thanks,

David / dhildenb

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

[Michal Hocko]

On Mon 10-12-18 14:53:22, osalvador@suse.de wrote:
> On 2018-12-07 11:35, osalvador@suse.de wrote:
> > On 2018-12-07 11:32, Michal Hocko wrote:
> > > On Fri 07-12-18 10:54:50, Vlastimil Babka wrote:
> > > > Well, __pageblock_pfn_to_page() has to be called for each
> > > > pageblock in
> > > > compaction, when zone_contiguous is false. And that's unchanged since
> > > > the introduction of zone_contiguous, so the numbers should still
> > > > hold.
> > > 
> > > OK, this means that we have to carefully re-evaluate zone_contiguous
> > > for
> > > each offline operation.
> 
> I started to think about this but some questions arose.
> 
> Actually, no matter what we do, if we re-evaluate zone_contiguous
> in the offline operation, zone_contiguous will be left unset.
> 
> set_zone_contiguous() calls __pageblock_pfn_to_page() in
> pageblock_nr_pages chunks to determine if the zone is contiguous,
> and checks the following:
> 
> * first/end pfn of the pageblock are valid sections
> * first pfn of the pageblock is online
> * we do not intersect different zones
> * first/end pfn belong to the same zone
> 
> Now, when dropping the shrink code and re-evaluating zone_contiguous in
> offline
> operation, set_zone_contiguous() will return false, leaving us with
> zone_contiguous
> unset.

Yeah. But does anything prevent us to alter the logic that the zone is
not contiguous iff there are offline holes or zones intermixed.
-- 
Michal Hocko
SUSE Labs

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

[osalvador]

>> Now, when dropping the shrink code and re-evaluating zone_contiguous 
>> in
>> offline
>> operation, set_zone_contiguous() will return false, leaving us with
>> zone_contiguous
>> unset.
> 
> Yeah. But does anything prevent us to alter the logic that the zone is
> not contiguous iff there are offline holes or zones intermixed.

Yes, thanks for the hint.
I think this is the best way to go.

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

[Michal Hocko]

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