[patch -mm] mm: slab allocate memory section nodemask for large systems

[patch -mm] mm: slab allocate memory section nodemask for large systems

[David Rientjes]

Nodemasks should not be allocated on the stack for large systems (when it
is larger than 256 bytes) since there is a threat of overflow.

This patch causes the unregister_mem_sect_under_nodes() nodemask to be
allocated on the stack for smaller systems and be allocated by slab for
larger systems.

GFP_KERNEL is used since remove_memory_block() can block.

Cc: Gary Hade <garyhade@us.ibm.com>
Cc: Badari Pulavarty <pbadari@us.ibm.com>
Cc: Alex Chiang <achiang@hp.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 drivers/base/node.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -363,12 +363,16 @@ int register_mem_sect_under_node(struct memory_block *mem_blk, int nid)
 /* unregister memory section under all nodes that it spans */
 int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
 {
-	nodemask_t unlinked_nodes;
+	NODEMASK_ALLOC(nodemask_t, unlinked_nodes, GFP_KERNEL);
 	unsigned long pfn, sect_start_pfn, sect_end_pfn;
 
-	if (!mem_blk)
+	if (!mem_blk) {
+		NODEMASK_FREE(unlinked_nodes);
 		return -EFAULT;
-	nodes_clear(unlinked_nodes);
+	}
+	if (!unlinked_nodes)
+		return -ENOMEM;
+	nodes_clear(*unlinked_nodes);
 	sect_start_pfn = section_nr_to_pfn(mem_blk->phys_index);
 	sect_end_pfn = sect_start_pfn + PAGES_PER_SECTION - 1;
 	for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
@@ -379,13 +383,14 @@ int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
 			continue;
 		if (!node_online(nid))
 			continue;
-		if (node_test_and_set(nid, unlinked_nodes))
+		if (node_test_and_set(nid, *unlinked_nodes))
 			continue;
 		sysfs_remove_link(&node_devices[nid].sysdev.kobj,
 			 kobject_name(&mem_blk->sysdev.kobj));
 		sysfs_remove_link(&mem_blk->sysdev.kobj,
 			 kobject_name(&node_devices[nid].sysdev.kobj));
 	}
+	NODEMASK_FREE(unlinked_nodes);
 	return 0;
 }
 

Re: [patch -mm] mm: slab allocate memory section nodemask for large systems

[Gary Hade]

Hi David,

On Tue, Nov 17, 2009 at 04:19:30PM -0800, David Rientjes wrote:
> Nodemasks should not be allocated on the stack for large systems (when it
> is larger than 256 bytes) since there is a threat of overflow.
> 
> This patch causes the unregister_mem_sect_under_nodes() nodemask to be
> allocated on the stack for smaller systems and be allocated by slab for
> larger systems.

I notice that there are many other functions that always allocate
nodemask_t objects on the stack.  In addition to several that add
a single instance to the stack, cpuset_attach() in kernel/cpuset.c
adds 2 instances and all that are created by using SYSCALL_DEFINE4()
in mm/mempolicy.c add 3 instances.  Are there plans to correct the
other functions as well or is there something about
unregister_mem_sect_under_nodes() that makes it more likely to
cause stack overflows than the others?

Gary

-- 
Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@us.ibm.com
http://www.ibm.com/linux/ltc

Re: [patch -mm] mm: slab allocate memory section nodemask for large systems

[Andrew Morton]

On Tue, 10 Nov 2009 14:26:29 -0700
Alex Chiang <achiang@hp.com> wrote:

> > I'm not aware of any prerequisites for this patchset, Alex's documentation 
> > changes have already been merged by Linus.
> 
> Correct. So I'll respin this series against... Linus's tree? Or
> maybe mmotm? Please advise.

It appears that Linus's tree will be an OK base.

Re: [patch -mm] mm: slab allocate memory section nodemask for large systems

[Alex Chiang]

* David Rientjes <rientjes@google.com>:
> On Tue, 10 Nov 2009, Andrew Morton wrote:
> 
> > The prerequisite Documentation/ patches are a bit of a mess - some have
> > been cherrypicked into Greg's tree I believe and some haven't.  So
> > please also send out whatever is needed to bring linux-next up to date.

Thanks, I'll respin.

> I'm not aware of any prerequisites for this patchset, Alex's documentation 
> changes have already been merged by Linus.

Correct. So I'll respin this series against... Linus's tree? Or
maybe mmotm? Please advise.

Thanks,
/ac

Re: [patch -mm] mm: slab allocate memory section nodemask for large systems

[David Rientjes]

On Tue, 10 Nov 2009, Andrew Morton wrote:

> The prerequisite Documentation/ patches are a bit of a mess - some have
> been cherrypicked into Greg's tree I believe and some haven't.  So
> please also send out whatever is needed to bring linux-next up to date.
> 

I'm not aware of any prerequisites for this patchset, Alex's documentation 
changes have already been merged by Linus.

Re: [patch -mm] mm: slab allocate memory section nodemask for large systems

[Andrew Morton]

On Mon, 2 Nov 2009 13:47:26 -0700
Alex Chiang <achiang@hp.com> wrote:

> I can respin this series once more, including David's Acked-by:
> and adding his patch if that makes life easier for you.

Yes, please redo and resend.

The prerequisite Documentation/ patches are a bit of a mess - some have
been cherrypicked into Greg's tree I believe and some haven't.  So
please also send out whatever is needed to bring linux-next up to date.

Re: [patch -mm] mm: slab allocate memory section nodemask for large systems

[David Rientjes]

On Mon, 2 Nov 2009, Alex Chiang wrote:

> Any comments on this patch series?
> 
> Turns out that Kame-san's fear about a memory section spanning
> several nodes on certain architectures (S390) isn't really
> applicable and even if it were, we have code to handle situation
> anyway.
> 
> Kame-san was generally supportive of these convenience symlinks
> although he did not give a formal ACK.
> 
> David has given an ACK on the two patches that do real work, as
> well as supplied the below patch.
> 
> I can respin this series once more, including David's Acked-by:
> and adding his patch if that makes life easier for you.
> 

It's probably in Andrew's queue after getting back from the kernel summit, 
it would be best to wait a week or so.

Re: [patch -mm] mm: slab allocate memory section nodemask for large systems

[Alex Chiang]

Hi Andrew,

* David Rientjes <rientjes@google.com>:
> On Wed, 28 Oct 2009, Alex Chiang wrote:
> 
> > Am I not understanding the code? It looks like we do this
> > already...
> > 
> > /* unregister memory section under all nodes that it spans */
> > int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
> > {
> > 	nodemask_t unlinked_nodes;
> > 	unsigned long pfn, sect_start_pfn, sect_end_pfn;
> > 
> > 	if (!mem_blk)
> > 		return -EFAULT;
> > 	nodes_clear(unlinked_nodes);
> > 	sect_start_pfn = section_nr_to_pfn(mem_blk->phys_index);
> > 	sect_end_pfn = sect_start_pfn + PAGES_PER_SECTION - 1;
> > 	for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
> > 		int nid;
> > 
> > 		nid = get_nid_for_pfn(pfn);
> > 		if (nid < 0)
> > 			continue;
> > 		if (!node_online(nid))
> > 			continue;
> > 		if (node_test_and_set(nid, unlinked_nodes))
> > 			continue;
> > 		sysfs_remove_link(&node_devices[nid].sysdev.kobj,
> > 			 kobject_name(&mem_blk->sysdev.kobj));
> > 		sysfs_remove_link(&mem_blk->sysdev.kobj,
> > 			 kobject_name(&node_devices[nid].sysdev.kobj));
> > 	}
> > 	return 0;
> > }
> > 
> 
> That shound be sufficient with the exception that allocating nodemask_t 
> on the stack is usually dangerous because it can be extremely large; we 
> typically use NODEMASK_ALLOC() for such code.  It's had some changes in 
> -mm, but since this patchset will likely be going through that tree anyway 
> we can fix it now with the patch below.
> 
> Otherwise, it looks like the iteration is already there and will remove 
> links for memory sections bound to multiple nodes if they exist through 
> hotplug.

Any comments on this patch series?

Turns out that Kame-san's fear about a memory section spanning
several nodes on certain architectures (S390) isn't really
applicable and even if it were, we have code to handle situation
anyway.

Kame-san was generally supportive of these convenience symlinks
although he did not give a formal ACK.

David has given an ACK on the two patches that do real work, as
well as supplied the below patch.

I can respin this series once more, including David's Acked-by:
and adding his patch if that makes life easier for you.

Thanks,
/ac


> mm: slab allocate memory section nodemask for large systems
> 
> Nodemasks should not be allocated on the stack for large systems (when it
> is larger than 256 bytes) since there is a threat of overflow.
> 
> This patch causes the unregister_mem_sect_under_nodes() nodemask to be
> allocated on the stack for smaller systems and be allocated by slab for
> larger systems.
> 
> GFP_KERNEL is used since remove_memory_block() can block.
> 
> Cc: Gary Hade <garyhade@us.ibm.com>
> Cc: Badari Pulavarty <pbadari@us.ibm.com>
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  Depends on NODEMASK_ALLOC() changes currently present only in -mm.
> 
>  drivers/base/node.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -363,12 +363,14 @@ int register_mem_sect_under_node(struct memory_block *mem_blk, int nid)
>  /* unregister memory section under all nodes that it spans */
>  int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
>  {
> -	nodemask_t unlinked_nodes;
> +	NODEMASK_ALLOC(nodemask_t, unlinked_nodes, GFP_KERNEL);
>  	unsigned long pfn, sect_start_pfn, sect_end_pfn;
>  
> -	if (!mem_blk)
> +	if (!mem_blk) {
> +		NODEMASK_FREE(unlinked_nodes);
>  		return -EFAULT;
> -	nodes_clear(unlinked_nodes);
> +	}
> +	nodes_clear(*unlinked_nodes);
>  	sect_start_pfn = section_nr_to_pfn(mem_blk->phys_index);
>  	sect_end_pfn = sect_start_pfn + PAGES_PER_SECTION - 1;
>  	for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
> @@ -379,13 +381,14 @@ int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
>  			continue;
>  		if (!node_online(nid))
>  			continue;
> -		if (node_test_and_set(nid, unlinked_nodes))
> +		if (node_test_and_set(nid, *unlinked_nodes))
>  			continue;
>  		sysfs_remove_link(&node_devices[nid].sysdev.kobj,
>  			 kobject_name(&mem_blk->sysdev.kobj));
>  		sysfs_remove_link(&mem_blk->sysdev.kobj,
>  			 kobject_name(&node_devices[nid].sysdev.kobj));
>  	}
> +	NODEMASK_FREE(unlinked_nodes);
>  	return 0;
>  }
>  
> 

[patch -mm] mm: slab allocate memory section nodemask for large systems

[David Rientjes]

On Wed, 28 Oct 2009, Alex Chiang wrote:

> Am I not understanding the code? It looks like we do this
> already...
> 
> /* unregister memory section under all nodes that it spans */
> int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
> {
> 	nodemask_t unlinked_nodes;
> 	unsigned long pfn, sect_start_pfn, sect_end_pfn;
> 
> 	if (!mem_blk)
> 		return -EFAULT;
> 	nodes_clear(unlinked_nodes);
> 	sect_start_pfn = section_nr_to_pfn(mem_blk->phys_index);
> 	sect_end_pfn = sect_start_pfn + PAGES_PER_SECTION - 1;
> 	for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
> 		int nid;
> 
> 		nid = get_nid_for_pfn(pfn);
> 		if (nid < 0)
> 			continue;
> 		if (!node_online(nid))
> 			continue;
> 		if (node_test_and_set(nid, unlinked_nodes))
> 			continue;
> 		sysfs_remove_link(&node_devices[nid].sysdev.kobj,
> 			 kobject_name(&mem_blk->sysdev.kobj));
> 		sysfs_remove_link(&mem_blk->sysdev.kobj,
> 			 kobject_name(&node_devices[nid].sysdev.kobj));
> 	}
> 	return 0;
> }
> 

That shound be sufficient with the exception that allocating nodemask_t 
on the stack is usually dangerous because it can be extremely large; we 
typically use NODEMASK_ALLOC() for such code.  It's had some changes in 
-mm, but since this patchset will likely be going through that tree anyway 
we can fix it now with the patch below.

Otherwise, it looks like the iteration is already there and will remove 
links for memory sections bound to multiple nodes if they exist through 
hotplug.



mm: slab allocate memory section nodemask for large systems

Nodemasks should not be allocated on the stack for large systems (when it
is larger than 256 bytes) since there is a threat of overflow.

This patch causes the unregister_mem_sect_under_nodes() nodemask to be
allocated on the stack for smaller systems and be allocated by slab for
larger systems.

GFP_KERNEL is used since remove_memory_block() can block.

Cc: Gary Hade <garyhade@us.ibm.com>
Cc: Badari Pulavarty <pbadari@us.ibm.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 Depends on NODEMASK_ALLOC() changes currently present only in -mm.

 drivers/base/node.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -363,12 +363,14 @@ int register_mem_sect_under_node(struct memory_block *mem_blk, int nid)
 /* unregister memory section under all nodes that it spans */
 int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
 {
-	nodemask_t unlinked_nodes;
+	NODEMASK_ALLOC(nodemask_t, unlinked_nodes, GFP_KERNEL);
 	unsigned long pfn, sect_start_pfn, sect_end_pfn;
 
-	if (!mem_blk)
+	if (!mem_blk) {
+		NODEMASK_FREE(unlinked_nodes);
 		return -EFAULT;
-	nodes_clear(unlinked_nodes);
+	}
+	nodes_clear(*unlinked_nodes);
 	sect_start_pfn = section_nr_to_pfn(mem_blk->phys_index);
 	sect_end_pfn = sect_start_pfn + PAGES_PER_SECTION - 1;
 	for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
@@ -379,13 +381,14 @@ int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
 			continue;
 		if (!node_online(nid))
 			continue;
-		if (node_test_and_set(nid, unlinked_nodes))
+		if (node_test_and_set(nid, *unlinked_nodes))
 			continue;
 		sysfs_remove_link(&node_devices[nid].sysdev.kobj,
 			 kobject_name(&mem_blk->sysdev.kobj));
 		sysfs_remove_link(&mem_blk->sysdev.kobj,
 			 kobject_name(&node_devices[nid].sysdev.kobj));
 	}
+	NODEMASK_FREE(unlinked_nodes);
 	return 0;
 }