Patch "mm: fix remote numa hits statistics" has been added to the 4.9-stable tree

PATCH [email protected] 2 participants 7 messages Jan 17, 2017 – Jan 18, 2017

Subsystems (3)

Stable branch Memory management Page allocator

lore

Share & tools

lore

[email protected]

author maintainer Applied

2017-01-17 16:13 UTC #0 lore raw

This is a note to let you know that I've just added the patch titled

mm: fix remote numa hits statistics

to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
mm-fix-remote-numa-hits-statistics.patch
and it can be found in the queue-4.9 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From 2df26639e708a88dcc22171949da638a9998f3bc Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Tue, 10 Jan 2017 16:57:39 -0800
Subject: mm: fix remote numa hits statistics

From: Michal Hocko <[email protected]>

commit 2df26639e708a88dcc22171949da638a9998f3bc upstream.

Jia He has noticed that commit b9f00e147f27 ("mm, page_alloc: reduce branches in zone_statistics") has an unintentional side effect that remote node allocation requests are accounted as NUMA_MISS rathat than NUMA_HIT and NUMA_OTHER if such a request doesn't use __GFP_OTHER_NODE.

There are many of these potentially because the flag is used very rarely while we have many users of __alloc_pages_node.

Fix this by simply ignoring __GFP_OTHER_NODE (it can be removed in a follow up patch) and treat all allocations that were satisfied from the preferred zone's node as NUMA_HITS because this is the same node we requested the allocation from in most cases. If this is not the local node then we just account it as NUMA_OTHER rather than NUMA_LOCAL.

One downsize would be that an allocation request for a node which is outside of the mempolicy nodemask would be reported as a hit which is a bit weird but that was the case before b9f00e147f27 already.

Fixes b9f00e147f27 ("mm, page_alloc: reduce branches in zone_statistics")

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Michal Hocko <[email protected]>

Reported-by Jia He <[email protected]>

Reviewed-by Vlastimil Babka <[email protected]> # with cbmc[1] superpowers

Acked-by Mel Gorman <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Joonsoo Kim <[email protected]>

Cc Taku Izumi <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 mm/page_alloc.c |   15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

mm/page_alloc.c +4 ~~-11~~

@@ -2556,30 +2556,23 @@ int __isolate_free_page(struct page *pag
  * Update NUMA hit/miss statistics
  *
  * Must be called with interrupts disabled.
- *
- * When __GFP_OTHER_NODE is set assume the node of the preferred
- * zone is the local node. This is useful for daemons who allocate
- * memory on behalf of other processes.
  */
 static inline void zone_statistics(struct zone *preferred_zone, struct zone *z,
 								gfp_t flags)
 {
 #ifdef CONFIG_NUMA
-	int local_nid = numa_node_id();
 	enum zone_stat_item local_stat = NUMA_LOCAL;
 
-	if (unlikely(flags & __GFP_OTHER_NODE)) {
+	if (z->node != numa_node_id())
 		local_stat = NUMA_OTHER;
-		local_nid = preferred_zone->node;
-	}
 
-	if (z->node == local_nid) {
+	if (z->node == preferred_zone->node)
 		__inc_zone_state(z, NUMA_HIT);
-		__inc_zone_state(z, local_stat);
-	} else {
+	else {
 		__inc_zone_state(z, NUMA_MISS);
 		__inc_zone_state(preferred_zone, NUMA_FOREIGN);
 	}
+	__inc_zone_state(z, local_stat);
 #endif
 }

Patches currently in stable-queue which might be from [email protected] are

queue-4.9/mm-memcg-fix-the-active-list-aging-for-lowmem-requests-when-memcg-is-enabled.patch queue-4.9/mm-fix-remote-numa-hits-statistics.patch queue-4.9/mm-pmd-dirty-emulation-in-page-fault-handler.patch

Michal Hocko

2017-01-17 16:22 UTC #1 lore raw

Quoted 8 lines · Greg KH

This is a note to let you know that I've just added the patch titled
mm: fix remote numa hits statistics
to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

Quoted 6 lines

The filename of the patch is:
mm-fix-remote-numa-hits-statistics.patch
and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

It is not marked for stable so why it has been chosen? Me or Andrew would have marked it for stable if it was needed. In fact I do not really thing it is necessary. It fixes counters which haven't been accurate for quite some time and nobody has noticed.

Quoted 88 lines

Quoted 1 line
From 2df26639e708a88dcc22171949da638a9998f3bc Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Tue, 10 Jan 2017 16:57:39 -0800
Subject: mm: fix remote numa hits statistics
From: Michal Hocko <[email protected]>
commit 2df26639e708a88dcc22171949da638a9998f3bc upstream.
Jia He has noticed that commit b9f00e147f27 ("mm, page_alloc: reduce branches in zone_statistics") has an unintentional side effect that remote node allocation requests are accounted as NUMA_MISS rathat than NUMA_HIT and NUMA_OTHER if such a request doesn't use __GFP_OTHER_NODE.
There are many of these potentially because the flag is used very rarely while we have many users of __alloc_pages_node.
Fix this by simply ignoring __GFP_OTHER_NODE (it can be removed in a follow up patch) and treat all allocations that were satisfied from the preferred zone's node as NUMA_HITS because this is the same node we requested the allocation from in most cases. If this is not the local node then we just account it as NUMA_OTHER rather than NUMA_LOCAL.
One downsize would be that an allocation request for a node which is outside of the mempolicy nodemask would be reported as a hit which is a bit weird but that was the case before b9f00e147f27 already.
Fixes b9f00e147f27 ("mm, page_alloc: reduce branches in zone_statistics")
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Michal Hocko <[email protected]>
Reported-by Jia He <[email protected]>
Reviewed-by Vlastimil Babka <[email protected]> # with cbmc[1] superpowers
Acked-by Mel Gorman <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Joonsoo Kim <[email protected]>
Cc Taku Izumi <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 mm/page_alloc.c |   15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)
mm/page_alloc.c +4 ~~-11~~
@@ -2556,30 +2556,23 @@ int __isolate_free_page(struct page *pag
  * Update NUMA hit/miss statistics
  *
  * Must be called with interrupts disabled.
- *
- * When __GFP_OTHER_NODE is set assume the node of the preferred
- * zone is the local node. This is useful for daemons who allocate
- * memory on behalf of other processes.
  */
 static inline void zone_statistics(struct zone *preferred_zone, struct zone *z,
 								gfp_t flags)
 {
 #ifdef CONFIG_NUMA
-	int local_nid = numa_node_id();
 	enum zone_stat_item local_stat = NUMA_LOCAL;
 
-	if (unlikely(flags & __GFP_OTHER_NODE)) {
+	if (z->node != numa_node_id())
 		local_stat = NUMA_OTHER;
-		local_nid = preferred_zone->node;
-	}
 
-	if (z->node == local_nid) {
+	if (z->node == preferred_zone->node)
 		__inc_zone_state(z, NUMA_HIT);
-		__inc_zone_state(z, local_stat);
-	} else {
+	else {
 		__inc_zone_state(z, NUMA_MISS);
 		__inc_zone_state(preferred_zone, NUMA_FOREIGN);
 	}
+	__inc_zone_state(z, local_stat);
 #endif
 }
 
Patches currently in stable-queue which might be from [email protected] are
queue-4.9/mm-memcg-fix-the-active-list-aging-for-lowmem-requests-when-memcg-is-enabled.patch queue-4.9/mm-fix-remote-numa-hits-statistics.patch queue-4.9/mm-pmd-dirty-emulation-in-page-fault-handler.patch

Signature · 2 lines

Michal Hocko
SUSE Labs

Greg KH

author maintainer

2017-01-17 16:39 UTC #2 lore raw

Quoted 9 lines · Michal Hocko

Quoted 8 lines · Greg KH
This is a note to let you know that I've just added the patch titled
mm: fix remote numa hits statistics
to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

Quoted 8 lines

Quoted 6 lines
The filename of the patch is:
mm-fix-remote-numa-hits-statistics.patch
and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.
It is not marked for stable so why it has been chosen?

I found it by digging through the git commit logs.

Quoted 3 lines

Me or Andrew would have marked it for stable if it was needed. In fact I do not really thing it is necessary. It fixes counters which haven't been accurate for quite some time and nobody has noticed.

Ok, that's fair enough, I'll go drop it from the tree.

thanks,

greg k-h

Michal Hocko

2017-01-18 10:02 UTC #3 lore raw

Quoted 14 lines · Greg KH

Quoted 13 lines · Michal Hocko
Quoted 12 lines · Greg KH
This is a note to let you know that I've just added the patch titled
mm: fix remote numa hits statistics
to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
mm-fix-remote-numa-hits-statistics.patch
and it can be found in the queue-4.9 subdirectory.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.
It is not marked for stable so why it has been chosen?
I found it by digging through the git commit logs.

Was the Fixes tag the reason to pick it up?

Signature · 2 lines

Michal Hocko
SUSE Labs

Greg KH

author maintainer

2017-01-18 10:25 UTC #4 lore raw

Quoted 15 lines · Michal Hocko

Quoted 14 lines · Greg KH
Quoted 13 lines · Michal Hocko
Quoted 12 lines · Greg KH
This is a note to let you know that I've just added the patch titled
mm: fix remote numa hits statistics
to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
mm-fix-remote-numa-hits-statistics.patch
and it can be found in the queue-4.9 subdirectory.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.
It is not marked for stable so why it has been chosen?
I found it by digging through the git commit logs.
Was the Fixes tag the reason to pick it up?

It made me pay a bit more attention to it, yes. I search for that and other keywords, in the git log to see if there are patches that need to be backported where people forget to add the stable tags. I have to do this because some subsystems still never set them :(

thanks,

greg k-h

Michal Hocko

2017-01-18 10:33 UTC #5 lore raw

Quoted 21 lines · Greg KH

Quoted 20 lines · Michal Hocko
Quoted 19 lines · Greg KH
Quoted 17 lines · Michal Hocko
Quoted 8 lines · Greg KH
This is a note to let you know that I've just added the patch titled
mm: fix remote numa hits statistics
to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
Quoted 6 lines
The filename of the patch is:
mm-fix-remote-numa-hits-statistics.patch
and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.
It is not marked for stable so why it has been chosen?

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
I found it by digging through the git commit logs.
Was the Fixes tag the reason to pick it up?
It made me pay a bit more attention to it, yes. I search for that and other keywords, in the git log to see if there are patches that need to be backported where people forget to add the stable tags. I have to do this because some subsystems still never set them :(

I am pretty sure that Andrew is really trying hard to mark all the core MM changes for stable. Picking up changes just because they have Fixes:
tag in them is imho a bad idea. If somebody made the effort to add this tag I am pretty sure Cc: stable was considered as well. If that is not the case let's just push back and ask directly when a patch is submitted rather than picking up the patch and hope that somebody will object...

Don't get me wrong, I really do appreciate your effort here I am just worried that conflating Fixes: and Cc: stable is a wrong thing. I absolutely see a reason CC: stable should imply Fixes: but not other way around.

Signature · 2 lines

Michal Hocko
SUSE Labs

Greg KH

author maintainer

2017-01-18 11:08 UTC #6 lore raw

Quoted 38 lines · Michal Hocko

Quoted 21 lines · Greg KH
Quoted 20 lines · Michal Hocko
Quoted 19 lines · Greg KH
Quoted 17 lines · Michal Hocko
Quoted 8 lines · Greg KH
This is a note to let you know that I've just added the patch titled
mm: fix remote numa hits statistics
to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
Quoted 6 lines
The filename of the patch is:
mm-fix-remote-numa-hits-statistics.patch
and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.
It is not marked for stable so why it has been chosen?
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
I found it by digging through the git commit logs.
Was the Fixes tag the reason to pick it up?
It made me pay a bit more attention to it, yes. I search for that and other keywords, in the git log to see if there are patches that need to be backported where people forget to add the stable tags. I have to do this because some subsystems still never set them :(
I am pretty sure that Andrew is really trying hard to mark all the core MM changes for stable. Picking up changes just because they have Fixes:
tag in them is imho a bad idea. If somebody made the effort to add this tag I am pretty sure Cc: stable was considered as well. If that is not the case let's just push back and ask directly when a patch is submitted rather than picking up the patch and hope that somebody will object...

Quoted 4 lines

Don't get me wrong, I really do appreciate your effort here I am just worried that conflating Fixes: and Cc: stable is a wrong thing. I absolutely see a reason CC: stable should imply Fixes: but not other way around.

Yes, for lots of subsystems fixes: does not mean stable, but for a non-small number of other ones, it is needed still :(

Andrew does a great job marking stable fixes, I'll try to remember that
mm: stuff should not be backported if it only has a fixes: tag in the future.

thanks,

greg k-h