[PATCH] mm/buddy: get the allownodes for dump at once

[PATCH] mm/buddy: get the allownodes for dump at once

[Gavin Shan]

When dumping the statistics for zones in the allowed nodes in the
function show_free_areas(), skip_free_areas_node() got called for
multiple times to figure out the same information: the allowed nodes
for dump. It's reasonable to get the allowed nodes at once.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 mm/page_alloc.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 7892f84..211004e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2765,11 +2765,19 @@ out:
  */
 void show_free_areas(unsigned int filter)
 {
-	int cpu;
+	int nid, cpu;
+	nodemask_t allownodes;
 	struct zone *zone;
 
+	/* Figure out the allowed nodes for dump */
+	nodes_clear(allownodes);
+	for_each_online_node(nid) {
+		if (!skip_free_areas_node(filter, nid))
+			node_set(nid, allownodes);
+	}
+
 	for_each_populated_zone(zone) {
-		if (skip_free_areas_node(filter, zone_to_nid(zone)))
+		if (!node_isset(zone_to_nid(zone), allownodes))
 			continue;
 		show_node(zone);
 		printk("%s per-cpu:\n", zone->name);
@@ -2812,7 +2820,7 @@ void show_free_areas(unsigned int filter)
 	for_each_populated_zone(zone) {
 		int i;
 
-		if (skip_free_areas_node(filter, zone_to_nid(zone)))
+		if (!node_isset(zone_to_nid(zone), allownodes))
 			continue;
 		show_node(zone);
 		printk("%s"
@@ -2881,7 +2889,7 @@ void show_free_areas(unsigned int filter)
 	for_each_populated_zone(zone) {
  		unsigned long nr[MAX_ORDER], flags, order, total = 0;
 
-		if (skip_free_areas_node(filter, zone_to_nid(zone)))
+		if (!node_isset(zone_to_nid(zone), allownodes))
 			continue;
 		show_node(zone);
 		printk("%s: ", zone->name);
-- 
1.7.9.5

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

Re: [PATCH] mm/buddy: get the allownodes for dump at once

[Cong Wang]

On Thu, 14 Jun 2012 at 08:35 GMT, Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
> When dumping the statistics for zones in the allowed nodes in the
> function show_free_areas(), skip_free_areas_node() got called for
> multiple times to figure out the same information: the allowed nodes
> for dump. It's reasonable to get the allowed nodes at once.
>

I am not sure if cpuset_current_mems_allowed could be changed
during show_free_areas(), also show_free_areas() is not called
in any hot path...

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

Re: [PATCH] mm/buddy: get the allownodes for dump at once

[Gavin Shan]

>> When dumping the statistics for zones in the allowed nodes in the
>> function show_free_areas(), skip_free_areas_node() got called for
>> multiple times to figure out the same information: the allowed nodes
>> for dump. It's reasonable to get the allowed nodes at once.
>>

>
>I am not sure if cpuset_current_mems_allowed could be changed
>during show_free_areas(), also show_free_areas() is not called
>in any hot path...
>

Yeah, but I think it's reasonable to dump consistent nodes here.
If cpuset_current_mems_allowed gets changed on the fly, we won't
get consistent dump. So the code change would avoid non-consistent
case if possible.

Thanks,
Gavin

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

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

Re: [PATCH] mm/buddy: get the allownodes for dump at once

[David Rientjes]

On Thu, 14 Jun 2012, Gavin Shan wrote:

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 7892f84..211004e 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2765,11 +2765,19 @@ out:
>   */
>  void show_free_areas(unsigned int filter)
>  {
> -	int cpu;
> +	int nid, cpu;
> +	nodemask_t allownodes;
>  	struct zone *zone;
>  

I saw this added to the -mm tree today, but it has to be nacked with 
apologies for not seeing the patch on the mailing list earlier.

show_free_areas() is called by the oom killer, so we know two things: it 
can be called potentially very deep in the callchain and current is out of 
memory.  Both are killers for this patch since you're allocating 
nodemask_t on the stack here which could cause an overflow and because you 
can't easily fix that case with NODEMASK_ALLOC() since it allocates slab 
with GFP_KERNEL when we we're oom, which would simply suppress vital 
meminfo from being shown.

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

Re: [PATCH] mm/buddy: get the allownodes for dump at once

[Gavin Shan]

>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 7892f84..211004e 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -2765,11 +2765,19 @@ out:
>>   */
>>  void show_free_areas(unsigned int filter)
>>  {
>> -	int cpu;
>> +	int nid, cpu;
>> +	nodemask_t allownodes;
>>  	struct zone *zone;
>>  
>
>I saw this added to the -mm tree today, but it has to be nacked with 
>apologies for not seeing the patch on the mailing list earlier.
>

Thanks, David.

>show_free_areas() is called by the oom killer, so we know two things: it 
>can be called potentially very deep in the callchain and current is out of 
>memory.  Both are killers for this patch since you're allocating 
>nodemask_t on the stack here which could cause an overflow and because you 
>can't easily fix that case with NODEMASK_ALLOC() since it allocates slab 
>with GFP_KERNEL when we we're oom, which would simply suppress vital 
>meminfo from being shown.
>

I'm not sure it's the possible to resolve the concerns with "static" here
since "allownodes" will be cleared for each call to show_free_areas().

	static nodemask_t allownodes;

Thanks,
Gavin 

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

Re: [PATCH] mm/buddy: get the allownodes for dump at once

[David Rientjes]

On Thu, 21 Jun 2012, Gavin Shan wrote:

> I'm not sure it's the possible to resolve the concerns with "static" here
> since "allownodes" will be cleared for each call to show_free_areas().
> 
> 	static nodemask_t allownodes;
> 

There's nothing protecting concurrent access to it.  This function 
certainly isn't in a performance sensitive path so I would be inclined to 
just leave it as is.

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

Re: [PATCH] mm/buddy: get the allownodes for dump at once

[Gavin Shan]

>
>> I'm not sure it's the possible to resolve the concerns with "static" here
>> since "allownodes" will be cleared for each call to show_free_areas().
>> 
>> 	static nodemask_t allownodes;
>> 
>
>There's nothing protecting concurrent access to it.  This function 
>certainly isn't in a performance sensitive path so I would be inclined to 
>just leave it as is.
>

Ok. Thanks for comment, David.

Thanks,
Gavin

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

Re: [PATCH] mm/buddy: get the allownodes for dump at once

[Cong Wang]

On Thu, 21 Jun 2012 at 01:19 GMT, David Rientjes <rientjes@google.com> wrote:
>
> show_free_areas() is called by the oom killer, so we know two things: it 
> can be called potentially very deep in the callchain and current is out of 
> memory.  Both are killers for this patch since you're allocating 
> nodemask_t on the stack here which could cause an overflow and because you 
> can't easily fix that case with NODEMASK_ALLOC() since it allocates slab 
> with GFP_KERNEL when we we're oom, which would simply suppress vital 
> meminfo from being shown.
>

Adding a comment in the beginning of show_free_areas() would be nice,
to tell people not to allocate more memory either on stack or in heap.

Thanks.

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

Re: [PATCH] mm/buddy: get the allownodes for dump at once

[Gavin Shan]

>>
>> show_free_areas() is called by the oom killer, so we know two things: it 
>> can be called potentially very deep in the callchain and current is out of 
>> memory.  Both are killers for this patch since you're allocating 
>> nodemask_t on the stack here which could cause an overflow and because you 
>> can't easily fix that case with NODEMASK_ALLOC() since it allocates slab 
>> with GFP_KERNEL when we we're oom, which would simply suppress vital 
>> meminfo from being shown.
>>
>
>Adding a comment in the beginning of show_free_areas() would be nice,
>to tell people not to allocate more memory either on stack or in heap.
>

Thanks, Cong. I'll do it later :-)

Thanks,
Gavin

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

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