mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY

PATCH In review Baoquan He 5 participants 10 messages Mar 20, 2019

Subsystems (2)

Memory management Memory hot(un)plug

lore

Share & tools

lore

Baoquan He

author

2019-03-20 08:07 UTC #0 lore raw

PATCH 1/1 mm, memory_hotplug: Fix the wrong usage of N_HIGH_MEMORY

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 <[email protected]>

---
 mm/memory_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memory_hotplug.c +1 -1

index 6b05576fb4ec..09911d34a3be 100644
@@ -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
 }

Signature · 1 line

2.17.2

Michal Hocko

2019-03-20 08:46 UTC #1 lore raw

Quoted 8 lines · 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.

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.

Quoted 21 lines

Fixes 8efe33f40f3e ("mm/memory_hotplug.c: simplify node_states_check_changes_online")

Signed-off-by Baoquan He <[email protected]>

---
 mm/memory_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memory_hotplug.c +1 -1

index 6b05576fb4ec..09911d34a3be 100644
@@ -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
 }

Signature · 1 line

2.17.2

Signature · 2 lines

Michal Hocko
SUSE Labs

Baoquan He

author

2019-03-20 09:06 UTC #2 lore raw

Quoted 8 lines · Michal Hocko

Quoted 7 lines · 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.

Quoted 4 lines

Quoted 1 line
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.

Quoted 5 lines

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

Quoted 26 lines

Quoted 22 lines

Fixes 8efe33f40f3e ("mm/memory_hotplug.c: simplify node_states_check_changes_online")

Signed-off-by Baoquan He <[email protected]>

---
 mm/memory_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memory_hotplug.c +1 -1

index 6b05576fb4ec..09911d34a3be 100644
@@ -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
 }

Signature · 1 line

2.17.2

Signature · 2 lines

Michal Hocko
SUSE Labs

David Hildenbrand

Reviewed-by

2019-03-20 09:11 UTC #3 lore raw

Quoted 28 lines · Baoquan He

Quoted 8 lines · Michal Hocko
Quoted 7 lines · 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.
Quoted 4 lines
Quoted 1 line
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.
Quoted 5 lines
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:

Quoted 7 lines

~~~
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 <[email protected]>

Signature · 3 lines

Thanks,

David / dhildenb

Baoquan He

author

2019-03-20 09:29 UTC #4 lore raw

Quoted 35 lines · David Hildenbrand

Quoted 34 lines · Baoquan He
Quoted 8 lines · Michal Hocko
Quoted 7 lines · 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.
Quoted 4 lines
Quoted 1 line
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.
Quoted 5 lines
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.

Quoted 3 lines

Quoted 1 line
Replace it with ZONE_HIGHMEM.
Reviewed-by David Hildenbrand <[email protected]>

Thanks, both. Will use this log and repost.

Thanks
Baoquan

Michal Hocko

Acked-by

2019-03-20 09:37 UTC #5 lore raw

In reply to Baoquan He

Quoted 28 lines · Baoquan He

Quoted 8 lines · Michal Hocko
Quoted 7 lines · 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.
Quoted 4 lines
Quoted 1 line
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.
Quoted 5 lines
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:

Quoted 7 lines

~~~
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 <[email protected]>

Signature · 2 lines

Michal Hocko
SUSE Labs

Oscar Salvador

maintainer Reviewed-by on v1 1/1

2019-03-20 12:27 UTC #6 lore raw

In reply to Baoquan He

Quoted 11 lines · 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 <[email protected]>

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 <[email protected]>

Quoted 20 lines

---
 mm/memory_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memory_hotplug.c +1 -1

index 6b05576fb4ec..09911d34a3be 100644
@@ -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
 }

Signature · 1 line

2.17.2

Signature · 2 lines

Oscar Salvador
SUSE L3

Oscar Salvador

maintainer

2019-03-20 12:28 UTC #7 lore raw

In reply to Baoquan He

Quoted 11 lines · 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 <[email protected]>

I think you forgot linux-mm list

Quoted 20 lines

---
 mm/memory_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memory_hotplug.c +1 -1

index 6b05576fb4ec..09911d34a3be 100644
@@ -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
 }

Signature · 1 line

2.17.2

Signature · 2 lines

Oscar Salvador
SUSE L3

Andrew Morton

maintainer

2019-03-20 19:12 UTC #8 lore raw

In reply to Baoquan He

Quoted 10 lines · On Wed, 20 Mar 2019 16:07:32 +0800 Baoquan He <[email protected]> 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?

Quoted 11 lines

mm/memory_hotplug.c +1 -1

@@ -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

2019-03-20 19:53 UTC #9 lore raw

Quoted 9 lines · Andrew Morton

Quoted 7 lines · On Wed, 20 Mar 2019 16:07:32 +0800 Baoquan He <[email protected]> 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.

Quoted 5 lines

Quoted 3 lines
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.

Quoted 12 lines

Quoted 11 lines

mm/memory_hotplug.c +1 -1

@@ -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
 }

Signature · 2 lines

Michal Hocko
SUSE Labs