linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY
@ 2019-03-20  8:07 Baoquan He
  2019-03-20  8:46 ` Michal Hocko
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Baoquan He @ 2019-03-20  8:07 UTC (permalink / raw)
  To: linux-kernel
  Cc: akpm, osalvador, mhocko, david, richard.weiyang, rppt, Baoquan He

In function node_states_check_changes_online(), N_HIGH_MEMORY is used
to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
always has value '3' if CONFIG_HIGHMEM=y, while ZONE_HIGHMEM's value
is not. It depends on whether CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32 are
enabled. Obviously it's not true for CONFIG_ZONE_DMA32 on 32bit system,
and CONFIG_ZONE_DMA is also optional.

Replace it with ZONE_HIGHMEM.

Fixes: 8efe33f40f3e ("mm/memory_hotplug.c: simplify node_states_check_changes_online")
Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/memory_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 6b05576fb4ec..09911d34a3be 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -712,7 +712,7 @@ static void node_states_check_changes_online(unsigned long nr_pages,
 	if (zone_idx(zone) <= ZONE_NORMAL && !node_state(nid, N_NORMAL_MEMORY))
 		arg->status_change_nid_normal = nid;
 #ifdef CONFIG_HIGHMEM
-	if (zone_idx(zone) <= N_HIGH_MEMORY && !node_state(nid, N_HIGH_MEMORY))
+	if (zone_idx(zone) <= ZONE_HIGHMEM && !node_state(nid, N_HIGH_MEMORY))
 		arg->status_change_nid_high = nid;
 #endif
 }
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY
  2019-03-20  8:07 [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY Baoquan He
@ 2019-03-20  8:46 ` Michal Hocko
  2019-03-20  9:06   ` Baoquan He
  2019-03-20 12:27 ` Oscar Salvador
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Michal Hocko @ 2019-03-20  8:46 UTC (permalink / raw)
  To: Baoquan He; +Cc: linux-kernel, akpm, osalvador, david, richard.weiyang, rppt

On Wed 20-03-19 16:07:32, Baoquan He wrote:
> In function node_states_check_changes_online(), N_HIGH_MEMORY is used
> to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
> always has value '3' if CONFIG_HIGHMEM=y, while ZONE_HIGHMEM's value
> is not. It depends on whether CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32 are
> enabled. Obviously it's not true for CONFIG_ZONE_DMA32 on 32bit system,
> and CONFIG_ZONE_DMA is also optional.
> 
> Replace it with ZONE_HIGHMEM.

N*MEMORY is confusing as hell but I am really curious whether we have
ZONE_DMA32 and ZONE_HIGMEM together?

That being said N.*MEMORY is intended to check for nodes rather than
zones so the patch looks good to me but I think the above explanation is
misleading and will add even more mud to the picture when somebody tries
to understand what the heck is going on here.

> Fixes: 8efe33f40f3e ("mm/memory_hotplug.c: simplify node_states_check_changes_online")
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  mm/memory_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 6b05576fb4ec..09911d34a3be 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -712,7 +712,7 @@ static void node_states_check_changes_online(unsigned long nr_pages,
>  	if (zone_idx(zone) <= ZONE_NORMAL && !node_state(nid, N_NORMAL_MEMORY))
>  		arg->status_change_nid_normal = nid;
>  #ifdef CONFIG_HIGHMEM
> -	if (zone_idx(zone) <= N_HIGH_MEMORY && !node_state(nid, N_HIGH_MEMORY))
> +	if (zone_idx(zone) <= ZONE_HIGHMEM && !node_state(nid, N_HIGH_MEMORY))
>  		arg->status_change_nid_high = nid;
>  #endif
>  }
> -- 
> 2.17.2

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY
  2019-03-20  8:46 ` Michal Hocko
@ 2019-03-20  9:06   ` Baoquan He
  2019-03-20  9:11     ` David Hildenbrand
  2019-03-20  9:37     ` Michal Hocko
  0 siblings, 2 replies; 10+ messages in thread
From: Baoquan He @ 2019-03-20  9:06 UTC (permalink / raw)
  To: Michal Hocko; +Cc: linux-kernel, akpm, osalvador, david, richard.weiyang, rppt

On 03/20/19 at 09:46am, Michal Hocko wrote:
> On Wed 20-03-19 16:07:32, Baoquan He wrote:
> > In function node_states_check_changes_online(), N_HIGH_MEMORY is used
> > to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
> > always has value '3' if CONFIG_HIGHMEM=y, while ZONE_HIGHMEM's value
> > is not. It depends on whether CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32 are
> > enabled. Obviously it's not true for CONFIG_ZONE_DMA32 on 32bit system,
> > and CONFIG_ZONE_DMA is also optional.
> > 
> > Replace it with ZONE_HIGHMEM.
> 
> N*MEMORY is confusing as hell but I am really curious whether we have
> ZONE_DMA32 and ZONE_HIGMEM together?

Not sure. AFAIK, on x86_32 it can't be.

> 
> That being said N.*MEMORY is intended to check for nodes rather than
> zones so the patch looks good to me but I think the above explanation is
> misleading and will add even more mud to the picture when somebody tries
> to understand what the heck is going on here.

Yes, agree. I also thought this again after I sent out patch, feel log is not
good. As you said, they are value of enum node_states and enum zone_type
separately.

How about this:

~~~
In function node_states_check_changes_online(), N_HIGH_MEMORY is used
to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
is to mark the memory state of node. Here zone index is checked, which
should be compared with 'ZONE_HIGHMEM' accordingly.

Replace it with ZONE_HIGHMEM.
~~~
> 
> > Fixes: 8efe33f40f3e ("mm/memory_hotplug.c: simplify node_states_check_changes_online")
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  mm/memory_hotplug.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 6b05576fb4ec..09911d34a3be 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -712,7 +712,7 @@ static void node_states_check_changes_online(unsigned long nr_pages,
> >  	if (zone_idx(zone) <= ZONE_NORMAL && !node_state(nid, N_NORMAL_MEMORY))
> >  		arg->status_change_nid_normal = nid;
> >  #ifdef CONFIG_HIGHMEM
> > -	if (zone_idx(zone) <= N_HIGH_MEMORY && !node_state(nid, N_HIGH_MEMORY))
> > +	if (zone_idx(zone) <= ZONE_HIGHMEM && !node_state(nid, N_HIGH_MEMORY))
> >  		arg->status_change_nid_high = nid;
> >  #endif
> >  }
> > -- 
> > 2.17.2
> 
> -- 
> Michal Hocko
> SUSE Labs

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY
  2019-03-20  9:06   ` Baoquan He
@ 2019-03-20  9:11     ` David Hildenbrand
  2019-03-20  9:29       ` Baoquan He
  2019-03-20  9:37     ` Michal Hocko
  1 sibling, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2019-03-20  9:11 UTC (permalink / raw)
  To: Baoquan He, Michal Hocko
  Cc: linux-kernel, akpm, osalvador, richard.weiyang, rppt

On 20.03.19 10:06, Baoquan He wrote:
> On 03/20/19 at 09:46am, Michal Hocko wrote:
>> On Wed 20-03-19 16:07:32, Baoquan He wrote:
>>> In function node_states_check_changes_online(), N_HIGH_MEMORY is used
>>> to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
>>> always has value '3' if CONFIG_HIGHMEM=y, while ZONE_HIGHMEM's value
>>> is not. It depends on whether CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32 are
>>> enabled. Obviously it's not true for CONFIG_ZONE_DMA32 on 32bit system,
>>> and CONFIG_ZONE_DMA is also optional.
>>>
>>> Replace it with ZONE_HIGHMEM.
>>
>> N*MEMORY is confusing as hell but I am really curious whether we have
>> ZONE_DMA32 and ZONE_HIGMEM together?
> 
> Not sure. AFAIK, on x86_32 it can't be.
> 
>>
>> That being said N.*MEMORY is intended to check for nodes rather than
>> zones so the patch looks good to me but I think the above explanation is
>> misleading and will add even more mud to the picture when somebody tries
>> to understand what the heck is going on here.
> 
> Yes, agree. I also thought this again after I sent out patch, feel log is not
> good. As you said, they are value of enum node_states and enum zone_type
> separately.
> 
> How about this:
> 
> ~~~
> In function node_states_check_changes_online(), N_HIGH_MEMORY is used
> to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
> is to mark the memory state of node. Here zone index is checked, which
> should be compared with 'ZONE_HIGHMEM' accordingly.
> 
> Replace it with ZONE_HIGHMEM.

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 

Thanks,

David / dhildenb

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY
  2019-03-20  9:11     ` David Hildenbrand
@ 2019-03-20  9:29       ` Baoquan He
  0 siblings, 0 replies; 10+ messages in thread
From: Baoquan He @ 2019-03-20  9:29 UTC (permalink / raw)
  To: Michal Hocko, David Hildenbrand
  Cc: linux-kernel, akpm, osalvador, richard.weiyang, rppt

On 03/20/19 at 10:11am, David Hildenbrand wrote:
> On 20.03.19 10:06, Baoquan He wrote:
> > On 03/20/19 at 09:46am, Michal Hocko wrote:
> >> On Wed 20-03-19 16:07:32, Baoquan He wrote:
> >>> In function node_states_check_changes_online(), N_HIGH_MEMORY is used
> >>> to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
> >>> always has value '3' if CONFIG_HIGHMEM=y, while ZONE_HIGHMEM's value
> >>> is not. It depends on whether CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32 are
> >>> enabled. Obviously it's not true for CONFIG_ZONE_DMA32 on 32bit system,
> >>> and CONFIG_ZONE_DMA is also optional.
> >>>
> >>> Replace it with ZONE_HIGHMEM.
> >>
> >> N*MEMORY is confusing as hell but I am really curious whether we have
> >> ZONE_DMA32 and ZONE_HIGMEM together?
> > 
> > Not sure. AFAIK, on x86_32 it can't be.
> > 
> >>
> >> That being said N.*MEMORY is intended to check for nodes rather than
> >> zones so the patch looks good to me but I think the above explanation is
> >> misleading and will add even more mud to the picture when somebody tries
> >> to understand what the heck is going on here.
> > 
> > Yes, agree. I also thought this again after I sent out patch, feel log is not
> > good. As you said, they are value of enum node_states and enum zone_type
> > separately.
> > 
> > How about this:
> > 
> > ~~~
> > In function node_states_check_changes_online(), N_HIGH_MEMORY is used
> > to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
> > is to mark the memory state of node. Here zone index is checked, which
> > should be compared with 'ZONE_HIGHMEM' accordingly.
> > 
> > Replace it with ZONE_HIGHMEM.
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks, both. Will use this log and repost.

Thanks
Baoquan

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY
  2019-03-20  9:06   ` Baoquan He
  2019-03-20  9:11     ` David Hildenbrand
@ 2019-03-20  9:37     ` Michal Hocko
  1 sibling, 0 replies; 10+ messages in thread
From: Michal Hocko @ 2019-03-20  9:37 UTC (permalink / raw)
  To: Baoquan He; +Cc: linux-kernel, akpm, osalvador, david, richard.weiyang, rppt

On Wed 20-03-19 17:06:24, Baoquan He wrote:
> On 03/20/19 at 09:46am, Michal Hocko wrote:
> > On Wed 20-03-19 16:07:32, Baoquan He wrote:
> > > In function node_states_check_changes_online(), N_HIGH_MEMORY is used
> > > to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
> > > always has value '3' if CONFIG_HIGHMEM=y, while ZONE_HIGHMEM's value
> > > is not. It depends on whether CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32 are
> > > enabled. Obviously it's not true for CONFIG_ZONE_DMA32 on 32bit system,
> > > and CONFIG_ZONE_DMA is also optional.
> > > 
> > > Replace it with ZONE_HIGHMEM.
> > 
> > N*MEMORY is confusing as hell but I am really curious whether we have
> > ZONE_DMA32 and ZONE_HIGMEM together?
> 
> Not sure. AFAIK, on x86_32 it can't be.
> 
> > 
> > That being said N.*MEMORY is intended to check for nodes rather than
> > zones so the patch looks good to me but I think the above explanation is
> > misleading and will add even more mud to the picture when somebody tries
> > to understand what the heck is going on here.
> 
> Yes, agree. I also thought this again after I sent out patch, feel log is not
> good. As you said, they are value of enum node_states and enum zone_type
> separately.
> 
> How about this:
> 
> ~~~
> In function node_states_check_changes_online(), N_HIGH_MEMORY is used
> to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
> is to mark the memory state of node. Here zone index is checked, which
> should be compared with 'ZONE_HIGHMEM' accordingly.
> 
> Replace it with ZONE_HIGHMEM.

OK, this is better. Thanks!

Feel free to stamp the patch with
Acked-by: Michal Hocko <mhocko@suse.com>

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY
  2019-03-20  8:07 [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY Baoquan He
  2019-03-20  8:46 ` Michal Hocko
@ 2019-03-20 12:27 ` Oscar Salvador
  2019-03-20 12:28 ` Oscar Salvador
  2019-03-20 19:12 ` Andrew Morton
  3 siblings, 0 replies; 10+ messages in thread
From: Oscar Salvador @ 2019-03-20 12:27 UTC (permalink / raw)
  To: Baoquan He; +Cc: linux-kernel, akpm, mhocko, david, richard.weiyang, rppt

On Wed, Mar 20, 2019 at 04:07:32PM +0800, Baoquan He wrote:
> In function node_states_check_changes_online(), N_HIGH_MEMORY is used
> to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
> always has value '3' if CONFIG_HIGHMEM=y, while ZONE_HIGHMEM's value
> is not. It depends on whether CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32 are
> enabled. Obviously it's not true for CONFIG_ZONE_DMA32 on 32bit system,
> and CONFIG_ZONE_DMA is also optional.
> 
> Replace it with ZONE_HIGHMEM.
> 
> Fixes: 8efe33f40f3e ("mm/memory_hotplug.c: simplify node_states_check_changes_online")
> Signed-off-by: Baoquan He <bhe@redhat.com>

Yeah, that was an oversight.
I got it right in node_states_check_changes_offline but not here.

Thanks to catch this.

Reviewed-by: Oscar Salvador <osalvador@suse.de>

> ---
>  mm/memory_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 6b05576fb4ec..09911d34a3be 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -712,7 +712,7 @@ static void node_states_check_changes_online(unsigned long nr_pages,
>  	if (zone_idx(zone) <= ZONE_NORMAL && !node_state(nid, N_NORMAL_MEMORY))
>  		arg->status_change_nid_normal = nid;
>  #ifdef CONFIG_HIGHMEM
> -	if (zone_idx(zone) <= N_HIGH_MEMORY && !node_state(nid, N_HIGH_MEMORY))
> +	if (zone_idx(zone) <= ZONE_HIGHMEM && !node_state(nid, N_HIGH_MEMORY))
>  		arg->status_change_nid_high = nid;
>  #endif
>  }
> -- 
> 2.17.2
> 

-- 
Oscar Salvador
SUSE L3

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY
  2019-03-20  8:07 [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY Baoquan He
  2019-03-20  8:46 ` Michal Hocko
  2019-03-20 12:27 ` Oscar Salvador
@ 2019-03-20 12:28 ` Oscar Salvador
  2019-03-20 19:12 ` Andrew Morton
  3 siblings, 0 replies; 10+ messages in thread
From: Oscar Salvador @ 2019-03-20 12:28 UTC (permalink / raw)
  To: Baoquan He; +Cc: linux-kernel, akpm, mhocko, david, richard.weiyang, rppt

On Wed, Mar 20, 2019 at 04:07:32PM +0800, Baoquan He wrote:
> In function node_states_check_changes_online(), N_HIGH_MEMORY is used
> to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
> always has value '3' if CONFIG_HIGHMEM=y, while ZONE_HIGHMEM's value
> is not. It depends on whether CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32 are
> enabled. Obviously it's not true for CONFIG_ZONE_DMA32 on 32bit system,
> and CONFIG_ZONE_DMA is also optional.
> 
> Replace it with ZONE_HIGHMEM.
> 
> Fixes: 8efe33f40f3e ("mm/memory_hotplug.c: simplify node_states_check_changes_online")
> Signed-off-by: Baoquan He <bhe@redhat.com>

I think you forgot linux-mm list

> ---
>  mm/memory_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 6b05576fb4ec..09911d34a3be 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -712,7 +712,7 @@ static void node_states_check_changes_online(unsigned long nr_pages,
>  	if (zone_idx(zone) <= ZONE_NORMAL && !node_state(nid, N_NORMAL_MEMORY))
>  		arg->status_change_nid_normal = nid;
>  #ifdef CONFIG_HIGHMEM
> -	if (zone_idx(zone) <= N_HIGH_MEMORY && !node_state(nid, N_HIGH_MEMORY))
> +	if (zone_idx(zone) <= ZONE_HIGHMEM && !node_state(nid, N_HIGH_MEMORY))
>  		arg->status_change_nid_high = nid;
>  #endif
>  }
> -- 
> 2.17.2
> 

-- 
Oscar Salvador
SUSE L3

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY
  2019-03-20  8:07 [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY Baoquan He
                   ` (2 preceding siblings ...)
  2019-03-20 12:28 ` Oscar Salvador
@ 2019-03-20 19:12 ` Andrew Morton
  2019-03-20 19:53   ` Michal Hocko
  3 siblings, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2019-03-20 19:12 UTC (permalink / raw)
  To: Baoquan He
  Cc: linux-kernel, osalvador, mhocko, david, richard.weiyang, rppt, linux-mm

On Wed, 20 Mar 2019 16:07:32 +0800 Baoquan He <bhe@redhat.com> wrote:

> In function node_states_check_changes_online(), N_HIGH_MEMORY is used
> to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
> always has value '3' if CONFIG_HIGHMEM=y, while ZONE_HIGHMEM's value
> is not. It depends on whether CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32 are
> enabled. Obviously it's not true for CONFIG_ZONE_DMA32 on 32bit system,
> and CONFIG_ZONE_DMA is also optional.
> 
> Replace it with ZONE_HIGHMEM.
> 
> Fixes: 8efe33f40f3e ("mm/memory_hotplug.c: simplify node_states_check_changes_online")

What are the runtime effects of this change?

> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -712,7 +712,7 @@ static void node_states_check_changes_online(unsigned long nr_pages,
>  	if (zone_idx(zone) <= ZONE_NORMAL && !node_state(nid, N_NORMAL_MEMORY))
>  		arg->status_change_nid_normal = nid;
>  #ifdef CONFIG_HIGHMEM
> -	if (zone_idx(zone) <= N_HIGH_MEMORY && !node_state(nid, N_HIGH_MEMORY))
> +	if (zone_idx(zone) <= ZONE_HIGHMEM && !node_state(nid, N_HIGH_MEMORY))
>  		arg->status_change_nid_high = nid;
>  #endif
>  }


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY
  2019-03-20 19:12 ` Andrew Morton
@ 2019-03-20 19:53   ` Michal Hocko
  0 siblings, 0 replies; 10+ messages in thread
From: Michal Hocko @ 2019-03-20 19:53 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Baoquan He, linux-kernel, osalvador, david, richard.weiyang,
	rppt, linux-mm

On Wed 20-03-19 12:12:09, Andrew Morton wrote:
> On Wed, 20 Mar 2019 16:07:32 +0800 Baoquan He <bhe@redhat.com> wrote:
> 
> > In function node_states_check_changes_online(), N_HIGH_MEMORY is used
> > to substitute ZONE_HIGHMEM directly. This is not right. N_HIGH_MEMORY
> > always has value '3' if CONFIG_HIGHMEM=y, while ZONE_HIGHMEM's value
> > is not. It depends on whether CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32 are
> > enabled. Obviously it's not true for CONFIG_ZONE_DMA32 on 32bit system,
> > and CONFIG_ZONE_DMA is also optional.
> > 
> > Replace it with ZONE_HIGHMEM.
> > 
> > Fixes: 8efe33f40f3e ("mm/memory_hotplug.c: simplify node_states_check_changes_online")
> 
> What are the runtime effects of this change?

There shouldn't be none. The code is just messy. The newer changelog
should make it more clear I believe.

> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -712,7 +712,7 @@ static void node_states_check_changes_online(unsigned long nr_pages,
> >  	if (zone_idx(zone) <= ZONE_NORMAL && !node_state(nid, N_NORMAL_MEMORY))
> >  		arg->status_change_nid_normal = nid;
> >  #ifdef CONFIG_HIGHMEM
> > -	if (zone_idx(zone) <= N_HIGH_MEMORY && !node_state(nid, N_HIGH_MEMORY))
> > +	if (zone_idx(zone) <= ZONE_HIGHMEM && !node_state(nid, N_HIGH_MEMORY))
> >  		arg->status_change_nid_high = nid;
> >  #endif
> >  }
> 

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-03-20 19:53 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-20  8:07 [PATCH] mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY Baoquan He
2019-03-20  8:46 ` Michal Hocko
2019-03-20  9:06   ` Baoquan He
2019-03-20  9:11     ` David Hildenbrand
2019-03-20  9:29       ` Baoquan He
2019-03-20  9:37     ` Michal Hocko
2019-03-20 12:27 ` Oscar Salvador
2019-03-20 12:28 ` Oscar Salvador
2019-03-20 19:12 ` Andrew Morton
2019-03-20 19:53   ` Michal Hocko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).