Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1574922 > unrolled thread

[PATCH] mm/page_alloc: return 0 in case this node has no page within the zone

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm/page_alloc: return 0 in case this node has no page within the zone Wei Yang <richard.weiyang@gmail.com> - 2017-02-06 16:50 +0100
    Re: [PATCH] mm/page_alloc: return 0 in case this node has no page  within the zone Andrew Morton <akpm@linux-foundation.org> - 2017-02-07 00:30 +0100
      Re: [PATCH] mm/page_alloc: return 0 in case this node has no page  within the zone Wei Yang <richard.weiyang@gmail.com> - 2017-02-07 16:10 +0100
    Re: [PATCH] mm/page_alloc: return 0 in case this node has no page  within the zone Michal Hocko <mhocko@kernel.org> - 2017-02-07 10:50 +0100
      Re: [PATCH] mm/page_alloc: return 0 in case this node has no page  within the zone Wei Yang <richard.weiyang@gmail.com> - 2017-02-07 16:40 +0100
        Re: [PATCH] mm/page_alloc: return 0 in case this node has no page  within the zone Michal Hocko <mhocko@kernel.org> - 2017-02-07 16:50 +0100
          Re: [PATCH] mm/page_alloc: return 0 in case this node has no page  within the zone Wei Yang <richard.weiyang@gmail.com> - 2017-02-08 15:10 +0100
            Re: [PATCH] mm/page_alloc: return 0 in case this node has no page  within the zone Michal Hocko <mhocko@kernel.org> - 2017-02-08 16:00 +0100
          Re: [PATCH] mm/page_alloc: return 0 in case this node has no page  within the zone Wei Yang <richard.weiyang@gmail.com> - 2017-02-09 15:10 +0100

#1574922 — [PATCH] mm/page_alloc: return 0 in case this node has no page within the zone

The whole memory space is divided into several zones and nodes may have no
page in some zones. In this case, the __absent_pages_in_range() would
return 0, since the range it is searching for is an empty range.

Also this happens more often to those nodes with higher memory range when
there are more nodes, which is a trend for future architectures.

This patch checks the zone range after clamp and adjustment, return 0 if
the range is an empty range.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 mm/page_alloc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6de9440e3ae2..51c60c0eadcb 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5521,6 +5521,11 @@ static unsigned long __meminit zone_absent_pages_in_node(int nid,
 	adjust_zone_range_for_zone_movable(nid, zone_type,
 			node_start_pfn, node_end_pfn,
 			&zone_start_pfn, &zone_end_pfn);
+
+	/* If this node has no page within this zone, return 0. */
+	if (zone_start_pfn == zone_end_pfn)
+		return 0;
+
 	nr_absent = __absent_pages_in_range(nid, zone_start_pfn, zone_end_pfn);
 
 	/*
-- 
2.11.0

[toc] | [next] | [standalone]

#1575245 — Re: [PATCH] mm/page_alloc: return 0 in case this node has no page within the zone

On Mon,  6 Feb 2017 23:43:14 +0800 Wei Yang <richard.weiyang@gmail.com> wrote:

> The whole memory space is divided into several zones and nodes may have no
> page in some zones. In this case, the __absent_pages_in_range() would
> return 0, since the range it is searching for is an empty range.
> 
> Also this happens more often to those nodes with higher memory range when
> there are more nodes, which is a trend for future architectures.
> 
> This patch checks the zone range after clamp and adjustment, return 0 if
> the range is an empty range.

What are the user-visible runtime effects of this change?

[toc] | [prev] | [next] | [standalone]

#1575791 — Re: [PATCH] mm/page_alloc: return 0 in case this node has no page within the zone

[Multipart message — attachments visible in raw view] — view raw

On Mon, Feb 06, 2017 at 03:29:32PM -0800, Andrew Morton wrote:
>On Mon,  6 Feb 2017 23:43:14 +0800 Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> The whole memory space is divided into several zones and nodes may have no
>> page in some zones. In this case, the __absent_pages_in_range() would
>> return 0, since the range it is searching for is an empty range.
>> 
>> Also this happens more often to those nodes with higher memory range when
>> there are more nodes, which is a trend for future architectures.
>> 
>> This patch checks the zone range after clamp and adjustment, return 0 if
>> the range is an empty range.
>
>What are the user-visible runtime effects of this change?

Hmm, for users they may not "see" the effect, while it will save some time in
case there is no overlap between the zone and node.

-- 
Wei Yang
Help you, Help me

[toc] | [prev] | [next] | [standalone]

#1575528 — Re: [PATCH] mm/page_alloc: return 0 in case this node has no page within the zone

On Mon 06-02-17 23:43:14, Wei Yang wrote:
> The whole memory space is divided into several zones and nodes may have no
> page in some zones. In this case, the __absent_pages_in_range() would
> return 0, since the range it is searching for is an empty range.
> 
> Also this happens more often to those nodes with higher memory range when
> there are more nodes, which is a trend for future architectures.

I do not understand this part. Why would we see more zones with zero pfn
range in higher memory ranges.

> This patch checks the zone range after clamp and adjustment, return 0 if
> the range is an empty range.

I assume the whole point of this patch is to save
__absent_pages_in_range which iterates over all memblock regions, right?
Is there any reason why for_each_mem_pfn_range cannot be changed to
honor the given start/end pfns instead? I can imagine that a small zone
would see a similar pointless iterations...

> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  mm/page_alloc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6de9440e3ae2..51c60c0eadcb 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5521,6 +5521,11 @@ static unsigned long __meminit zone_absent_pages_in_node(int nid,
>  	adjust_zone_range_for_zone_movable(nid, zone_type,
>  			node_start_pfn, node_end_pfn,
>  			&zone_start_pfn, &zone_end_pfn);
> +
> +	/* If this node has no page within this zone, return 0. */
> +	if (zone_start_pfn == zone_end_pfn)
> +		return 0;
> +
>  	nr_absent = __absent_pages_in_range(nid, zone_start_pfn, zone_end_pfn);
>  
>  	/*
> -- 
> 2.11.0
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575808 — Re: [PATCH] mm/page_alloc: return 0 in case this node has no page within the zone

[Multipart message — attachments visible in raw view] — view raw

On Tue, Feb 07, 2017 at 10:45:57AM +0100, Michal Hocko wrote:
>On Mon 06-02-17 23:43:14, Wei Yang wrote:
>> The whole memory space is divided into several zones and nodes may have no
>> page in some zones. In this case, the __absent_pages_in_range() would
>> return 0, since the range it is searching for is an empty range.
>> 
>> Also this happens more often to those nodes with higher memory range when
>> there are more nodes, which is a trend for future architectures.
>
>I do not understand this part. Why would we see more zones with zero pfn
>range in higher memory ranges.
>

Based on my understanding, zone boundary is fixed address. For example, on
x84_64, ZONE_DMA is < 16M, ZONE_DMA32 is < 4G. And similar rules apply to
sparc, ia64, s390 as shown in the comment of ZONE definition.

For example, currently we see a server with 8 NUMA nodes and with 4T memory.
Those zone boundaries may all sits in the first node range, so that the nodes
with higher memory range may all sits in the last zone, which is ZONE_NORMAL I
think. During the memory initialization, for each node we still iterate on
each zone and calculate the memory range in each zone. By doing so, those
nodes with higher memory range will see several empty zones.

>> This patch checks the zone range after clamp and adjustment, return 0 if
>> the range is an empty range.
>
>I assume the whole point of this patch is to save
>__absent_pages_in_range which iterates over all memblock regions, right?

Yes, you are right. Since we know there is no overlap, it is not necessary to
do the iteration on memblock.

>Is there any reason why for_each_mem_pfn_range cannot be changed to
>honor the given start/end pfns instead? I can imagine that a small zone
>would see a similar pointless iterations...
>

Hmm... No special reason, just not thought about this implementation. And
actually I just do the similar thing as in zone_spanned_pages_in_node(), in
which also return 0 when there is no overlap.

BTW, I don't get your point. You wish to put the check in
for_each_mem_pfn_range() definition?

>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> ---
>>  mm/page_alloc.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 6de9440e3ae2..51c60c0eadcb 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -5521,6 +5521,11 @@ static unsigned long __meminit zone_absent_pages_in_node(int nid,
>>  	adjust_zone_range_for_zone_movable(nid, zone_type,
>>  			node_start_pfn, node_end_pfn,
>>  			&zone_start_pfn, &zone_end_pfn);
>> +
>> +	/* If this node has no page within this zone, return 0. */
>> +	if (zone_start_pfn == zone_end_pfn)
>> +		return 0;
>> +
>>  	nr_absent = __absent_pages_in_range(nid, zone_start_pfn, zone_end_pfn);
>>  
>>  	/*
>> -- 
>> 2.11.0
>> 
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

[toc] | [prev] | [next] | [standalone]

#1575811 — Re: [PATCH] mm/page_alloc: return 0 in case this node has no page within the zone

On Tue 07-02-17 23:32:47, Wei Yang wrote:
> On Tue, Feb 07, 2017 at 10:45:57AM +0100, Michal Hocko wrote:
[...]
> >Is there any reason why for_each_mem_pfn_range cannot be changed to
> >honor the given start/end pfns instead? I can imagine that a small zone
> >would see a similar pointless iterations...
> >
> 
> Hmm... No special reason, just not thought about this implementation. And
> actually I just do the similar thing as in zone_spanned_pages_in_node(), in
> which also return 0 when there is no overlap.
> 
> BTW, I don't get your point. You wish to put the check in
> for_each_mem_pfn_range() definition?

My point was that you are handling one special case (an empty zone) but
the underlying problem is that __absent_pages_in_range might be wasting
cycles iterating over memblocks that are way outside of the given pfn
range. At least this is my understanding. If you fix that you do not
need the special case, right?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1576605 — Re: [PATCH] mm/page_alloc: return 0 in case this node has no page within the zone

[Multipart message — attachments visible in raw view] — view raw

On Tue, Feb 07, 2017 at 04:41:21PM +0100, Michal Hocko wrote:
>On Tue 07-02-17 23:32:47, Wei Yang wrote:
>> On Tue, Feb 07, 2017 at 10:45:57AM +0100, Michal Hocko wrote:
>[...]
>> >Is there any reason why for_each_mem_pfn_range cannot be changed to
>> >honor the given start/end pfns instead? I can imagine that a small zone
>> >would see a similar pointless iterations...
>> >
>> 
>> Hmm... No special reason, just not thought about this implementation. And
>> actually I just do the similar thing as in zone_spanned_pages_in_node(), in
>> which also return 0 when there is no overlap.
>> 
>> BTW, I don't get your point. You wish to put the check in
>> for_each_mem_pfn_range() definition?
>
>My point was that you are handling one special case (an empty zone) but
>the underlying problem is that __absent_pages_in_range might be wasting
>cycles iterating over memblocks that are way outside of the given pfn
>range. At least this is my understanding. If you fix that you do not
>need the special case, right?

Yep, I think this is a good suggestion. By doing do, this could save iterating
cycles in __absent_pages_in_range(). 

Hmm, the case is a little bit different in zone_absent_pages_in_node() in case
there is movable zone in this node. Even __absent_pages_in_range() returns 0,
it is not a proof that this node has no page in this zone. Which means, we
still need to go through the ZONE_MOVABLE handling part, which is a memblock
iteration too.

Let's take a look whether guard __absent_pages_in_range() internally is
necessary now.

The function itself is invoked at three places:

* numa_meminfo_cover_memory()
* zone_absent_pages_in_node()
* absent_pages_in_range()

The first one is invoked on numa_meminfo which is sanitized by
numa_cleanup_meminfo().
The second one is analysed here.

The third one is invoked at two places:
* numa_meminfo_cover_memory()
* mem_hole_size()

At the first place, it is passed with (0, max_pfn) as parameter, which I think
is not common to have max_pfn to be 0.
At the second place, the start_pfn and end_pfn is already guarded.

With all those status, currently I choose to put the check in
zone_absent_pages_in_node().

BTW, the ZONE_MOVABLE handling looks strange to me and the comment "Treat
pages to be ZONE_MOVABLE in ZONE_NORMAL as absent pages and vice versa" is
hard to understand. From the code point of view, if zone_type is ZONE_NORMAL,
each memblock region between zone_start_pfn and zone_end_pfn would be treated
as absent pages if it is not mirrored. Do you have some hint on this?

>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

[toc] | [prev] | [next] | [standalone]

#1576633 — Re: [PATCH] mm/page_alloc: return 0 in case this node has no page within the zone

On Wed 08-02-17 22:05:18, Wei Yang wrote:
[...]
> BTW, the ZONE_MOVABLE handling looks strange to me and the comment "Treat
> pages to be ZONE_MOVABLE in ZONE_NORMAL as absent pages and vice versa" is
> hard to understand. From the code point of view, if zone_type is ZONE_NORMAL,
> each memblock region between zone_start_pfn and zone_end_pfn would be treated
> as absent pages if it is not mirrored. Do you have some hint on this?

Not really, sorry, this area is full of awkward and subtle code when new
changes build on top of previous awkwardness/surprises. Any cleanup
would be really appreciated. That is the reason I didn't like the
initial check all that much.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1577624 — Re: [PATCH] mm/page_alloc: return 0 in case this node has no page within the zone

[Multipart message — attachments visible in raw view] — view raw

On Tue, Feb 07, 2017 at 04:41:21PM +0100, Michal Hocko wrote:
>On Tue 07-02-17 23:32:47, Wei Yang wrote:
>> On Tue, Feb 07, 2017 at 10:45:57AM +0100, Michal Hocko wrote:
>[...]
>> >Is there any reason why for_each_mem_pfn_range cannot be changed to
>> >honor the given start/end pfns instead? I can imagine that a small zone
>> >would see a similar pointless iterations...
>> >
>> 
>> Hmm... No special reason, just not thought about this implementation. And
>> actually I just do the similar thing as in zone_spanned_pages_in_node(), in
>> which also return 0 when there is no overlap.
>> 
>> BTW, I don't get your point. You wish to put the check in
>> for_each_mem_pfn_range() definition?
>
>My point was that you are handling one special case (an empty zone) but
>the underlying problem is that __absent_pages_in_range might be wasting
>cycles iterating over memblocks that are way outside of the given pfn
>range. At least this is my understanding. If you fix that you do not
>need the special case, right?
>-- 
>Michal Hocko
>SUSE Labs

> Not really, sorry, this area is full of awkward and subtle code when new
> changes build on top of previous awkwardness/surprises. Any cleanup
> would be really appreciated. That is the reason I didn't like the
> initial check all that much.

Looks my fetchmail failed to get your last reply. So I copied it here.

Yes, the change here looks not that nice, while currently this is what I can't
come up with.

Thanks for your review :-)

-- 
Wei Yang
Help you, Help me

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web