[PATCH] mm/page_alloc: break on the first hit of mem range

[PATCH] mm/page_alloc: break on the first hit of mem range

[Wei Yang]

find_min_pfn_for_node() iterate on pfn range to find the minimum pfn for a
node. The memblock_region in memblock_type are already ordered, which means
the first hit in iteration is the minimum pfn.

This patch returns the fist hit instead of iterating the whole regions.

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

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 635d7dd29d7f..a65de1ec4b91 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -6365,14 +6365,14 @@ unsigned long __init node_map_pfn_alignment(void)
 /* Find the lowest pfn for a node */
 static unsigned long __init find_min_pfn_for_node(int nid)
 {
-	unsigned long min_pfn = ULONG_MAX;
-	unsigned long start_pfn;
+	unsigned long min_pfn;
 	int i;
 
-	for_each_mem_pfn_range(i, nid, &start_pfn, NULL, NULL)
-		min_pfn = min(min_pfn, start_pfn);
+	for_each_mem_pfn_range(i, nid, &min_pfn, NULL, NULL) {
+		break;
+	}
 
-	if (min_pfn == ULONG_MAX) {
+	if (i == -1) {
 		pr_warn("Could not find start_pfn for node %d\n", nid);
 		return 0;
 	}
-- 
2.15.1

Re: [PATCH] mm/page_alloc: break on the first hit of mem range

[Michal Hocko]

On Tue 27-03-18 11:57:07, Wei Yang wrote:
> find_min_pfn_for_node() iterate on pfn range to find the minimum pfn for a
> node. The memblock_region in memblock_type are already ordered, which means
> the first hit in iteration is the minimum pfn.

I haven't looked at the code yet but the changelog should contain the
motivation why it exists. It seems like this is an optimization. If so,
what is the impact?

> This patch returns the fist hit instead of iterating the whole regions.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  mm/page_alloc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 635d7dd29d7f..a65de1ec4b91 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6365,14 +6365,14 @@ unsigned long __init node_map_pfn_alignment(void)
>  /* Find the lowest pfn for a node */
>  static unsigned long __init find_min_pfn_for_node(int nid)
>  {
> -	unsigned long min_pfn = ULONG_MAX;
> -	unsigned long start_pfn;
> +	unsigned long min_pfn;
>  	int i;
>  
> -	for_each_mem_pfn_range(i, nid, &start_pfn, NULL, NULL)
> -		min_pfn = min(min_pfn, start_pfn);
> +	for_each_mem_pfn_range(i, nid, &min_pfn, NULL, NULL) {
> +		break;
> +	}
>  
> -	if (min_pfn == ULONG_MAX) {
> +	if (i == -1) {
>  		pr_warn("Could not find start_pfn for node %d\n", nid);
>  		return 0;
>  	}
> -- 
> 2.15.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/page_alloc: break on the first hit of mem range

[Wei Yang]

On Tue, Mar 27, 2018 at 12:58:21PM +0200, Michal Hocko wrote:
>On Tue 27-03-18 11:57:07, Wei Yang wrote:
>> find_min_pfn_for_node() iterate on pfn range to find the minimum pfn for a
>> node. The memblock_region in memblock_type are already ordered, which means
>> the first hit in iteration is the minimum pfn.
>
>I haven't looked at the code yet but the changelog should contain the
>motivation why it exists. It seems like this is an optimization. If so,
>what is the impact?
>

Yep, this is a trivial optimization on searching the minimal pfn on a special
node. It would be better for audience to understand if I put some words in
change log.

The impact of this patch is it would accelerate the searching process when
there are many memory ranges in memblock.

For example, in the case https://lkml.org/lkml/2018/3/25/291, there are around
30 memory ranges on node 0. The original code need to iterate all those ranges
to find the minimal pfn, while after optimization it just need once.

The more memory ranges there are, the more impact this patch has.

>> This patch returns the fist hit instead of iterating the whole regions.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> ---
>>  mm/page_alloc.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 635d7dd29d7f..a65de1ec4b91 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -6365,14 +6365,14 @@ unsigned long __init node_map_pfn_alignment(void)
>>  /* Find the lowest pfn for a node */
>>  static unsigned long __init find_min_pfn_for_node(int nid)
>>  {
>> -	unsigned long min_pfn = ULONG_MAX;
>> -	unsigned long start_pfn;
>> +	unsigned long min_pfn;
>>  	int i;
>>  
>> -	for_each_mem_pfn_range(i, nid, &start_pfn, NULL, NULL)
>> -		min_pfn = min(min_pfn, start_pfn);
>> +	for_each_mem_pfn_range(i, nid, &min_pfn, NULL, NULL) {
>> +		break;
>> +	}
>>  
>> -	if (min_pfn == ULONG_MAX) {
>> +	if (i == -1) {
>>  		pr_warn("Could not find start_pfn for node %d\n", nid);
>>  		return 0;
>>  	}
>> -- 
>> 2.15.1
>> 
>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm/page_alloc: break on the first hit of mem range

[Michal Hocko]

On Wed 28-03-18 08:39:36, Wei Yang wrote:
> On Tue, Mar 27, 2018 at 12:58:21PM +0200, Michal Hocko wrote:
> >On Tue 27-03-18 11:57:07, Wei Yang wrote:
> >> find_min_pfn_for_node() iterate on pfn range to find the minimum pfn for a
> >> node. The memblock_region in memblock_type are already ordered, which means
> >> the first hit in iteration is the minimum pfn.
> >
> >I haven't looked at the code yet but the changelog should contain the
> >motivation why it exists. It seems like this is an optimization. If so,
> >what is the impact?
> >
> 
> Yep, this is a trivial optimization on searching the minimal pfn on a special
> node. It would be better for audience to understand if I put some words in
> change log.
> 
> The impact of this patch is it would accelerate the searching process when
> there are many memory ranges in memblock.
> 
> For example, in the case https://lkml.org/lkml/2018/3/25/291, there are around
> 30 memory ranges on node 0. The original code need to iterate all those ranges
> to find the minimal pfn, while after optimization it just need once.

Then show us some numbers to justify the change.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/page_alloc: break on the first hit of mem range

[Wei Yang]

On Wed, Mar 28, 2018 at 09:02:00AM +0200, Michal Hocko wrote:
>On Wed 28-03-18 08:39:36, Wei Yang wrote:
>> On Tue, Mar 27, 2018 at 12:58:21PM +0200, Michal Hocko wrote:
>> >On Tue 27-03-18 11:57:07, Wei Yang wrote:
>> >> find_min_pfn_for_node() iterate on pfn range to find the minimum pfn for a
>> >> node. The memblock_region in memblock_type are already ordered, which means
>> >> the first hit in iteration is the minimum pfn.
>> >
>> >I haven't looked at the code yet but the changelog should contain the
>> >motivation why it exists. It seems like this is an optimization. If so,
>> >what is the impact?
>> >
>> 
>> Yep, this is a trivial optimization on searching the minimal pfn on a special
>> node. It would be better for audience to understand if I put some words in
>> change log.
>> 
>> The impact of this patch is it would accelerate the searching process when
>> there are many memory ranges in memblock.
>> 
>> For example, in the case https://lkml.org/lkml/2018/3/25/291, there are around
>> 30 memory ranges on node 0. The original code need to iterate all those ranges
>> to find the minimal pfn, while after optimization it just need once.
>
>Then show us some numbers to justify the change.

Oops, I don't have any data to prove this.

My test machine just has 7 memory regions and only one node. So it reduce
iteration from 7 to 1, which I don't think will have some visible effect.

While we can do some calculation to estimate the effect.

Assume there are N memory regions and M nodes and each node has equal number
of memory regions.

So before the change, there are

	N * M    iterations

After this optimization, there are

        (N / 2) * M   iterations

So the expected improvement of this change is half the iterations for finding
the minimal pfn.

Last but not the least, as I know, usually there are less than 100 memory
regions on a machine. This improvement is really limited on current systems.
The more memory regions and node a system has, the more improvement it will
has.

>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm/page_alloc: break on the first hit of mem range

[Andrew Morton]

On Tue, 27 Mar 2018 11:57:07 +0800 Wei Yang <richard.weiyang@gmail.com> wrote:

> find_min_pfn_for_node() iterate on pfn range to find the minimum pfn for a
> node. The memblock_region in memblock_type are already ordered, which means
> the first hit in iteration is the minimum pfn.
> 
> This patch returns the fist hit instead of iterating the whole regions.
> 
> ...
>
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6365,14 +6365,14 @@ unsigned long __init node_map_pfn_alignment(void)
>  /* Find the lowest pfn for a node */
>  static unsigned long __init find_min_pfn_for_node(int nid)
>  {
> -	unsigned long min_pfn = ULONG_MAX;
> -	unsigned long start_pfn;
> +	unsigned long min_pfn;
>  	int i;
>  
> -	for_each_mem_pfn_range(i, nid, &start_pfn, NULL, NULL)
> -		min_pfn = min(min_pfn, start_pfn);
> +	for_each_mem_pfn_range(i, nid, &min_pfn, NULL, NULL) {
> +		break;
> +	}

That would be the weirdest-looking code snippet in mm/!

Can't we just use a single and simple call to __next_mem_pfn_range(),
or something like that?

>
> ...
>

Re: [PATCH] mm/page_alloc: break on the first hit of mem range

[Wei Yang]

On Tue, Mar 27, 2018 at 03:47:40PM -0700, Andrew Morton wrote:
>On Tue, 27 Mar 2018 11:57:07 +0800 Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> find_min_pfn_for_node() iterate on pfn range to find the minimum pfn for a
>> node. The memblock_region in memblock_type are already ordered, which means
>> the first hit in iteration is the minimum pfn.
>> 
>> This patch returns the fist hit instead of iterating the whole regions.
>> 
>> ...
>>
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -6365,14 +6365,14 @@ unsigned long __init node_map_pfn_alignment(void)
>>  /* Find the lowest pfn for a node */
>>  static unsigned long __init find_min_pfn_for_node(int nid)
>>  {
>> -	unsigned long min_pfn = ULONG_MAX;
>> -	unsigned long start_pfn;
>> +	unsigned long min_pfn;
>>  	int i;
>>  
>> -	for_each_mem_pfn_range(i, nid, &start_pfn, NULL, NULL)
>> -		min_pfn = min(min_pfn, start_pfn);
>> +	for_each_mem_pfn_range(i, nid, &min_pfn, NULL, NULL) {
>> +		break;
>> +	}
>
>That would be the weirdest-looking code snippet in mm/!
>

You mean the only break in a for_each loop? Hmm..., this is really not that
nice. Haven't noticed could get a "best" in this way :-)

>Can't we just use a single and simple call to __next_mem_pfn_range(),
>or something like that?
>

Sounds a better choice, if you like this version, I would rearrange the patch
and send v2.

Have a nice day~

>>
>> ...
>>

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm/page_alloc: break on the first hit of mem range

[Andrew Morton]

On Wed, 28 Mar 2018 08:51:42 +0800 Wei Yang <richard.weiyang@gmail.com> wrote:

> On Tue, Mar 27, 2018 at 03:47:40PM -0700, Andrew Morton wrote:
> >On Tue, 27 Mar 2018 11:57:07 +0800 Wei Yang <richard.weiyang@gmail.com> wrote:
> >
> >> find_min_pfn_for_node() iterate on pfn range to find the minimum pfn for a
> >> node. The memblock_region in memblock_type are already ordered, which means
> >> the first hit in iteration is the minimum pfn.
> >> 
> >> This patch returns the fist hit instead of iterating the whole regions.
> >> 
> >> ...
> >>
> >> --- a/mm/page_alloc.c
> >> +++ b/mm/page_alloc.c
> >> @@ -6365,14 +6365,14 @@ unsigned long __init node_map_pfn_alignment(void)
> >>  /* Find the lowest pfn for a node */
> >>  static unsigned long __init find_min_pfn_for_node(int nid)
> >>  {
> >> -	unsigned long min_pfn = ULONG_MAX;
> >> -	unsigned long start_pfn;
> >> +	unsigned long min_pfn;
> >>  	int i;
> >>  
> >> -	for_each_mem_pfn_range(i, nid, &start_pfn, NULL, NULL)
> >> -		min_pfn = min(min_pfn, start_pfn);
> >> +	for_each_mem_pfn_range(i, nid, &min_pfn, NULL, NULL) {
> >> +		break;
> >> +	}
> >
> >That would be the weirdest-looking code snippet in mm/!
> >
> 
> You mean the only break in a for_each loop? Hmm..., this is really not that
> nice. Haven't noticed could get a "best" in this way :-)

I guess we can make it nicer by adding a comment along the lines of

	/*
	 * Use for_each_mem_pfn_range() to locate the lowest valid pfn in the
	 * range.  We only need to iterate a single time, as the pfn's are
	 * sorted in ascending order.
	 */

Because adding a call to the obviously-internal __next_mem_pfn_range()
isn't very nice either.

Anyway, please have a think, see what we can come up with.

Re: [PATCH] mm/page_alloc: break on the first hit of mem range

[Wei Yang]

On Tue, Mar 27, 2018 at 06:37:57PM -0700, Andrew Morton wrote:
>On Wed, 28 Mar 2018 08:51:42 +0800 Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> On Tue, Mar 27, 2018 at 03:47:40PM -0700, Andrew Morton wrote:
>> >On Tue, 27 Mar 2018 11:57:07 +0800 Wei Yang <richard.weiyang@gmail.com> wrote:
>> >
>> >> find_min_pfn_for_node() iterate on pfn range to find the minimum pfn for a
>> >> node. The memblock_region in memblock_type are already ordered, which means
>> >> the first hit in iteration is the minimum pfn.
>> >> 
>> >> This patch returns the fist hit instead of iterating the whole regions.
>> >> 
>> >> ...
>> >>
>> >> --- a/mm/page_alloc.c
>> >> +++ b/mm/page_alloc.c
>> >> @@ -6365,14 +6365,14 @@ unsigned long __init node_map_pfn_alignment(void)
>> >>  /* Find the lowest pfn for a node */
>> >>  static unsigned long __init find_min_pfn_for_node(int nid)
>> >>  {
>> >> -	unsigned long min_pfn = ULONG_MAX;
>> >> -	unsigned long start_pfn;
>> >> +	unsigned long min_pfn;
>> >>  	int i;
>> >>  
>> >> -	for_each_mem_pfn_range(i, nid, &start_pfn, NULL, NULL)
>> >> -		min_pfn = min(min_pfn, start_pfn);
>> >> +	for_each_mem_pfn_range(i, nid, &min_pfn, NULL, NULL) {
>> >> +		break;
>> >> +	}
>> >
>> >That would be the weirdest-looking code snippet in mm/!
>> >
>> 
>> You mean the only break in a for_each loop? Hmm..., this is really not that
>> nice. Haven't noticed could get a "best" in this way :-)
>
>I guess we can make it nicer by adding a comment along the lines of
>
>	/*
>	 * Use for_each_mem_pfn_range() to locate the lowest valid pfn in the
>	 * range.  We only need to iterate a single time, as the pfn's are
>	 * sorted in ascending order.
>	 */
>
>Because adding a call to the obviously-internal __next_mem_pfn_range()
>isn't very nice either.

Yep, you are right.

>
>Anyway, please have a think, see what we can come up with.

My approach is to add a macro fist_mem_pfn() as a self-explain wrapper of
__next_mem_pfn_range().

Hope you would like this :-)

-- 
Wei Yang
Help you, Help me

[PATCH] mm/page_alloc: optimize find_min_pfn_for_node() by geting the minimal pfn directly

[Wei Yang]

find_min_pfn_for_node() iterates on pfn range to find the minimum pfn for a
node, while this process could be optimized. The memblock_region in
memblock_type are already in ascending order, so the first one is the
minimal one.

For example, if there are 30 memory regions, the original version would
iterate all 30 regions, while the new version just need iterate a single
time.

This patch does a trivial optimization by adding first_mem_pfn() and use
this to get the minimal pfn directly.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

---
v2:
    * add first_mem_pfn() and use it to replace for_each_mem_pfn_ragne()
    * some more meaningful words in change log
---
 include/linux/memblock.h |  9 +++++++++
 mm/page_alloc.c          | 14 ++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 8be5077efb5f..22932f45538f 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -189,6 +189,15 @@ void __next_mem_pfn_range(int *idx, int nid, unsigned long *out_start_pfn,
 			  unsigned long *out_end_pfn, int *out_nid);
 unsigned long memblock_next_valid_pfn(unsigned long pfn, unsigned long max_pfn);
 
+/**
+ * first_mem_pfn - get the first memory pfn
+ * @i: an integer used as an indicator
+ * @nid: node selector, %MAX_NUMNODES for all nodes
+ * @p_first: ptr to ulong for first pfn of the range, can be %NULL
+ */
+#define first_mem_pfn(i, nid, p_first)				\
+	__next_mem_pfn_range(&i, nid, p_first, NULL, NULL)
+
 /**
  * for_each_mem_pfn_range - early memory pfn range iterator
  * @i: an integer used as loop variable
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 635d7dd29d7f..8c964dcc3a9e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -6365,14 +6365,16 @@ unsigned long __init node_map_pfn_alignment(void)
 /* Find the lowest pfn for a node */
 static unsigned long __init find_min_pfn_for_node(int nid)
 {
-	unsigned long min_pfn = ULONG_MAX;
-	unsigned long start_pfn;
-	int i;
+	unsigned long min_pfn;
+	int i = -1;
 
-	for_each_mem_pfn_range(i, nid, &start_pfn, NULL, NULL)
-		min_pfn = min(min_pfn, start_pfn);
+	/*
+	 * The first pfn on nid node is the minimal one, as the pfn's are
+	 * stored in ascending order.
+	 */
+	first_mem_pfn(i, nid, &min_pfn);
 
-	if (min_pfn == ULONG_MAX) {
+	if (i == -1) {
 		pr_warn("Could not find start_pfn for node %d\n", nid);
 		return 0;
 	}
-- 
2.15.1

Re: [PATCH] mm/page_alloc: optimize find_min_pfn_for_node() by geting the minimal pfn directly

[Michal Hocko]

On Wed 28-03-18 11:47:52, Wei Yang wrote:
[...]
> +/**
> + * first_mem_pfn - get the first memory pfn
> + * @i: an integer used as an indicator
> + * @nid: node selector, %MAX_NUMNODES for all nodes
> + * @p_first: ptr to ulong for first pfn of the range, can be %NULL
> + */
> +#define first_mem_pfn(i, nid, p_first)				\
> +	__next_mem_pfn_range(&i, nid, p_first, NULL, NULL)
> +

Is this really something that we want to export to all users? And if
that is the case is the documenation really telling user how to use it?

>  /**
>   * for_each_mem_pfn_range - early memory pfn range iterator
>   * @i: an integer used as loop variable
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 635d7dd29d7f..8c964dcc3a9e 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6365,14 +6365,16 @@ unsigned long __init node_map_pfn_alignment(void)
>  /* Find the lowest pfn for a node */
>  static unsigned long __init find_min_pfn_for_node(int nid)
>  {
> -	unsigned long min_pfn = ULONG_MAX;
> -	unsigned long start_pfn;
> -	int i;
> +	unsigned long min_pfn;
> +	int i = -1;
>  
> -	for_each_mem_pfn_range(i, nid, &start_pfn, NULL, NULL)
> -		min_pfn = min(min_pfn, start_pfn);
> +	/*
> +	 * The first pfn on nid node is the minimal one, as the pfn's are
> +	 * stored in ascending order.
> +	 */
> +	first_mem_pfn(i, nid, &min_pfn);
>  
> -	if (min_pfn == ULONG_MAX) {
> +	if (i == -1) {
>  		pr_warn("Could not find start_pfn for node %d\n", nid);
>  		return 0;
>  	}

I would just open code it. Other than that I strongly suspect this will
not have any measurable impact becauser we usually only have handfull of
memory ranges but why not. Just make the new implementation less ugly
than it is cuurrently - e.g. opencode first_mem_pfn and you can add
Acked-by: Michal Hocko <mhocko@suse.com>
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/page_alloc: optimize find_min_pfn_for_node() by geting the minimal pfn directly

[Wei Yang]

On Wed, Mar 28, 2018 at 01:58:53PM +0200, Michal Hocko wrote:
>On Wed 28-03-18 11:47:52, Wei Yang wrote:
>[...]
>> +/**
>> + * first_mem_pfn - get the first memory pfn
>> + * @i: an integer used as an indicator
>> + * @nid: node selector, %MAX_NUMNODES for all nodes
>> + * @p_first: ptr to ulong for first pfn of the range, can be %NULL
>> + */
>> +#define first_mem_pfn(i, nid, p_first)				\
>> +	__next_mem_pfn_range(&i, nid, p_first, NULL, NULL)
>> +
>
>Is this really something that we want to export to all users? And if
>that is the case is the documenation really telling user how to use it?
>

Yep, I am not good at the documentation. I really struggled a while on it, but
you know it still looks not that good.

How about changing the document to the following in case this macro is still
alive.

/**
 * first_mem_pfn - get the first memory pfn after index i on node nid
 * @i: index to memblock.memory.regions
 * @nid: node selector, %MAX_NUMNODES for all nodes
 * @p_first: ptr to ulong for first pfn of the range, can be %NULL
 */

>>  /**
>>   * for_each_mem_pfn_range - early memory pfn range iterator
>>   * @i: an integer used as loop variable
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 635d7dd29d7f..8c964dcc3a9e 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -6365,14 +6365,16 @@ unsigned long __init node_map_pfn_alignment(void)
>>  /* Find the lowest pfn for a node */
>>  static unsigned long __init find_min_pfn_for_node(int nid)
>>  {
>> -	unsigned long min_pfn = ULONG_MAX;
>> -	unsigned long start_pfn;
>> -	int i;
>> +	unsigned long min_pfn;
>> +	int i = -1;
>>  
>> -	for_each_mem_pfn_range(i, nid, &start_pfn, NULL, NULL)
>> -		min_pfn = min(min_pfn, start_pfn);
>> +	/*
>> +	 * The first pfn on nid node is the minimal one, as the pfn's are
>> +	 * stored in ascending order.
>> +	 */
>> +	first_mem_pfn(i, nid, &min_pfn);
>>  
>> -	if (min_pfn == ULONG_MAX) {
>> +	if (i == -1) {
>>  		pr_warn("Could not find start_pfn for node %d\n", nid);
>>  		return 0;
>>  	}
>
>I would just open code it. Other than that I strongly suspect this will
>not have any measurable impact becauser we usually only have handfull of
>memory ranges but why not. Just make the new implementation less ugly
>than it is cuurrently - e.g. opencode first_mem_pfn and you can add

Open code here means use __next_mem_pfn_range() directly instead of using
first_mem_pfn()?

>Acked-by: Michal Hocko <mhocko@suse.com>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm/page_alloc: optimize find_min_pfn_for_node() by geting the minimal pfn directly

[Michal Hocko]

On Wed 28-03-18 21:34:56, Wei Yang wrote:
> On Wed, Mar 28, 2018 at 01:58:53PM +0200, Michal Hocko wrote:
> >On Wed 28-03-18 11:47:52, Wei Yang wrote:
> >[...]
[...]
> >> @@ -6365,14 +6365,16 @@ unsigned long __init node_map_pfn_alignment(void)
> >>  /* Find the lowest pfn for a node */
> >>  static unsigned long __init find_min_pfn_for_node(int nid)
> >>  {
> >> -	unsigned long min_pfn = ULONG_MAX;
> >> -	unsigned long start_pfn;
> >> -	int i;
> >> +	unsigned long min_pfn;
> >> +	int i = -1;
> >>  
> >> -	for_each_mem_pfn_range(i, nid, &start_pfn, NULL, NULL)
> >> -		min_pfn = min(min_pfn, start_pfn);
> >> +	/*
> >> +	 * The first pfn on nid node is the minimal one, as the pfn's are
> >> +	 * stored in ascending order.
> >> +	 */
> >> +	first_mem_pfn(i, nid, &min_pfn);
> >>  
> >> -	if (min_pfn == ULONG_MAX) {
> >> +	if (i == -1) {
> >>  		pr_warn("Could not find start_pfn for node %d\n", nid);
> >>  		return 0;
> >>  	}
> >
> >I would just open code it. Other than that I strongly suspect this will
> >not have any measurable impact becauser we usually only have handfull of
> >memory ranges but why not. Just make the new implementation less ugly
> >than it is cuurrently - e.g. opencode first_mem_pfn and you can add
> 
> Open code here means use __next_mem_pfn_range() directly instead of using
> first_mem_pfn()?

Yes with the comment explaining how we rely on sorted ranges the way you
did.
-- 
Michal Hocko
SUSE Labs