Since sparse_index_alloc() can return NULL on memory allocation failure,
we must deal with the failure condition when calling it.
Signed-off-by: WANG Cong <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Rik van Riel <[email protected]>
---
diff --git a/Makefile b/Makefile
diff --git a/mm/sparse.c b/mm/sparse.c
index e06f514..d245e59 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -83,6 +83,8 @@ static int __meminit sparse_index_init(unsigned long section_nr, int nid)
return -EEXIST;
section = sparse_index_alloc(nid);
+ if (!section)
+ return -ENOMEM;
/*
* This lock keeps two different sections from
* reallocating for the same index
On Thu, 15 Nov 2007 21:54:28 +0800
WANG Cong <[email protected]> wrote:
>
> Since sparse_index_alloc() can return NULL on memory allocation failure,
> we must deal with the failure condition when calling it.
>
> Signed-off-by: WANG Cong <[email protected]>
> Cc: Christoph Lameter <[email protected]>
> Cc: Rik van Riel <[email protected]>
>
> ---
>
> diff --git a/Makefile b/Makefile
> diff --git a/mm/sparse.c b/mm/sparse.c
> index e06f514..d245e59 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -83,6 +83,8 @@ static int __meminit sparse_index_init(unsigned long section_nr, int nid)
> return -EEXIST;
>
> section = sparse_index_alloc(nid);
> + if (!section)
> + return -ENOMEM;
> /*
> * This lock keeps two different sections from
> * reallocating for the same index
Sure, but both callers of sparse_index_init() ignore its return value anyway.
On Thu, 2007-11-15 at 21:54 +0800, WANG Cong wrote:
> Since sparse_index_alloc() can return NULL on memory allocation failure,
> we must deal with the failure condition when calling it.
>
> Signed-off-by: WANG Cong <[email protected]>
> Cc: Christoph Lameter <[email protected]>
> Cc: Rik van Riel <[email protected]>
>
> ---
>
> diff --git a/Makefile b/Makefile
> diff --git a/mm/sparse.c b/mm/sparse.c
> index e06f514..d245e59 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -83,6 +83,8 @@ static int __meminit sparse_index_init(unsigned long section_nr, int nid)
> return -EEXIST;
>
> section = sparse_index_alloc(nid);
> + if (!section)
> + return -ENOMEM;
> /*
> * This lock keeps two different sections from
> * reallocating for the same index
Oddly enough, sparse_add_one_section() doesn't seem to like to check
its allocations. The usemap is checked, but not freed on error. If you
want to fix this up, I think it needs a little more love than just two
lines.
Do you want to try to add some actual error handling to
sparse_add_one_section()?
-- Dave
On Mon, Nov 19, 2007 at 01:17:02PM -0800, Dave Hansen wrote:
>On Thu, 2007-11-15 at 21:54 +0800, WANG Cong wrote:
>> Since sparse_index_alloc() can return NULL on memory allocation failure,
>> we must deal with the failure condition when calling it.
>>
>> Signed-off-by: WANG Cong <[email protected]>
>> Cc: Christoph Lameter <[email protected]>
>> Cc: Rik van Riel <[email protected]>
>>
>> ---
>>
>> diff --git a/Makefile b/Makefile
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index e06f514..d245e59 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -83,6 +83,8 @@ static int __meminit sparse_index_init(unsigned long section_nr, int nid)
>> return -EEXIST;
>>
>> section = sparse_index_alloc(nid);
>> + if (!section)
>> + return -ENOMEM;
>> /*
>> * This lock keeps two different sections from
>> * reallocating for the same index
>
>Oddly enough, sparse_add_one_section() doesn't seem to like to check
>its allocations. The usemap is checked, but not freed on error. If you
>want to fix this up, I think it needs a little more love than just two
>lines.
Er, right. I missed this point.
>
>Do you want to try to add some actual error handling to
>sparse_add_one_section()?
Yes, I will have a try. And memory_present() also doesn't check it.
More patches around this will come up soon. Since Andrew has included
the above patch, so I won't remake it with others together.
Andrew, is this OK for you?
Thanks.
Improve the error handling for mm/sparse.c::sparse_add_one_section().
And I see no reason to check 'usemap' until holding the
'pgdat_resize_lock'. If someone knows, please let me know.
Note! This patch is _not_ tested yet, since it seems that I can't
configure sparse memory for i386 box. Sorry for this. ;(
I hope someone can help me to test it.
Cc: Christoph Lameter <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Rik van Riel <[email protected]>
Signed-off-by: WANG Cong <[email protected]>
---
mm/sparse.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
Index: linux-2.6/mm/sparse.c
===================================================================
--- linux-2.6.orig/mm/sparse.c
+++ linux-2.6/mm/sparse.c
@@ -391,9 +391,17 @@ int sparse_add_one_section(struct zone *
* no locking for this, because it does its own
* plus, it does a kmalloc
*/
- sparse_index_init(section_nr, pgdat->node_id);
+ ret = sparse_index_init(section_nr, pgdat->node_id);
+ if (ret < 0)
+ return ret;
memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, nr_pages);
+ if (!memmap)
+ return -ENOMEM;
usemap = __kmalloc_section_usemap();
+ if (!usemap) {
+ __kfree_section_memmap(memmap, nr_pages);
+ return -ENOMEM;
+ }
pgdat_resize_lock(pgdat, &flags);
@@ -403,18 +411,13 @@ int sparse_add_one_section(struct zone *
goto out;
}
- if (!usemap) {
- ret = -ENOMEM;
- goto out;
- }
ms->section_mem_map |= SECTION_MARKED_PRESENT;
ret = sparse_init_one_section(ms, section_nr, memmap, usemap);
out:
pgdat_resize_unlock(pgdat, &flags);
- if (ret <= 0)
- __kfree_section_memmap(memmap, nr_pages);
+
return ret;
}
#endif
Hi, Cong-san.
> ms->section_mem_map |= SECTION_MARKED_PRESENT;
>
> ret = sparse_init_one_section(ms, section_nr, memmap, usemap);
>
> out:
> pgdat_resize_unlock(pgdat, &flags);
> - if (ret <= 0)
> - __kfree_section_memmap(memmap, nr_pages);
> +
> return ret;
> }
> #endif
Hmm. When sparse_init_one_section() returns error, memmap and
usemap should be free.
Thanks for your fixing.
--
Yasunori Goto
On Mon, Nov 26, 2007 at 07:19:49PM +0900, Yasunori Goto wrote:
>Hi, Cong-san.
>
>> ms->section_mem_map |= SECTION_MARKED_PRESENT;
>>
>> ret = sparse_init_one_section(ms, section_nr, memmap, usemap);
>>
>> out:
>> pgdat_resize_unlock(pgdat, &flags);
>> - if (ret <= 0)
>> - __kfree_section_memmap(memmap, nr_pages);
>> +
>> return ret;
>> }
>> #endif
>
>Hmm. When sparse_init_one_section() returns error, memmap and
>usemap should be free.
Hi, Yasunori.
Thanks for your comments. Is the following one fine for you?
Signed-off-by: WANG Cong <[email protected]>
---
Index: linux-2.6/mm/sparse.c
===================================================================
--- linux-2.6.orig/mm/sparse.c
+++ linux-2.6/mm/sparse.c
@@ -391,9 +391,17 @@ int sparse_add_one_section(struct zone *
* no locking for this, because it does its own
* plus, it does a kmalloc
*/
- sparse_index_init(section_nr, pgdat->node_id);
+ ret = sparse_index_init(section_nr, pgdat->node_id);
+ if (ret < 0)
+ return ret;
memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, nr_pages);
+ if (!memmap)
+ return -ENOMEM;
usemap = __kmalloc_section_usemap();
+ if (!usemap) {
+ __kfree_section_memmap(memmap, nr_pages);
+ return -ENOMEM;
+ }
pgdat_resize_lock(pgdat, &flags);
@@ -403,10 +411,6 @@ int sparse_add_one_section(struct zone *
goto out;
}
- if (!usemap) {
- ret = -ENOMEM;
- goto out;
- }
ms->section_mem_map |= SECTION_MARKED_PRESENT;
ret = sparse_init_one_section(ms, section_nr, memmap, usemap);
@@ -414,7 +418,7 @@ int sparse_add_one_section(struct zone *
out:
pgdat_resize_unlock(pgdat, &flags);
if (ret <= 0)
- __kfree_section_memmap(memmap, nr_pages);
+ kfree(usemap);
return ret;
}
#endif
> ret = sparse_init_one_section(ms, section_nr, memmap, usemap);
> @@ -414,7 +418,7 @@ int sparse_add_one_section(struct zone *
> out:
> pgdat_resize_unlock(pgdat, &flags);
> if (ret <= 0)
> - __kfree_section_memmap(memmap, nr_pages);
> + kfree(usemap);
> return ret;
> }
> #endif
>
I guess you think __kfree_section_memmap() is not necessary due to
no implementation. But, it is still available when
CONFIG_SPARSEMEM_VMEMMAP is off. So, it should not be removed.
Bye.
--
Yasunori Goto
On Tue, 2007-11-27 at 10:26 +0800, WANG Cong wrote:
>
> @@ -414,7 +418,7 @@ int sparse_add_one_section(struct zone *
> out:
> pgdat_resize_unlock(pgdat, &flags);
> if (ret <= 0)
> - __kfree_section_memmap(memmap, nr_pages);
> + kfree(usemap);
> return ret;
> }
> #endif
Why did you get rid of the memmap free here? A bad return from
sparse_init_one_section() indicates that we didn't use the memmap, so it
will leak otherwise.
-- Dave
On Tue, Nov 27, 2007 at 10:53:45AM -0800, Dave Hansen wrote:
>On Tue, 2007-11-27 at 10:26 +0800, WANG Cong wrote:
>>
>> @@ -414,7 +418,7 @@ int sparse_add_one_section(struct zone *
>> out:
>> pgdat_resize_unlock(pgdat, &flags);
>> if (ret <= 0)
>> - __kfree_section_memmap(memmap, nr_pages);
>> + kfree(usemap);
>> return ret;
>> }
>> #endif
>
>Why did you get rid of the memmap free here? A bad return from
>sparse_init_one_section() indicates that we didn't use the memmap, so it
>will leak otherwise.
Sorry, I was confused by the recursion. This one should be OK.
Thanks.
Improve the error handling for mm/sparse.c::sparse_add_one_section(). And I
see no reason to check 'usemap' until holding the 'pgdat_resize_lock'.
Cc: Christoph Lameter <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Yasunori Goto <[email protected]>
Cc: Andy Whitcroft <[email protected]>
Signed-off-by: WANG Cong <[email protected]>
---
Index: linux-2.6/mm/sparse.c
===================================================================
--- linux-2.6.orig/mm/sparse.c
+++ linux-2.6/mm/sparse.c
@@ -391,9 +391,17 @@ int sparse_add_one_section(struct zone *
* no locking for this, because it does its own
* plus, it does a kmalloc
*/
- sparse_index_init(section_nr, pgdat->node_id);
+ ret = sparse_index_init(section_nr, pgdat->node_id);
+ if (ret < 0)
+ return ret;
memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, nr_pages);
+ if (!memmap)
+ return -ENOMEM;
usemap = __kmalloc_section_usemap();
+ if (!usemap) {
+ __kfree_section_memmap(memmap, nr_pages);
+ return -ENOMEM;
+ }
pgdat_resize_lock(pgdat, &flags);
@@ -403,18 +411,16 @@ int sparse_add_one_section(struct zone *
goto out;
}
- if (!usemap) {
- ret = -ENOMEM;
- goto out;
- }
ms->section_mem_map |= SECTION_MARKED_PRESENT;
ret = sparse_init_one_section(ms, section_nr, memmap, usemap);
out:
pgdat_resize_unlock(pgdat, &flags);
- if (ret <= 0)
+ if (ret <= 0) {
+ kfree(usemap);
__kfree_section_memmap(memmap, nr_pages);
+ }
return ret;
}
#endif
Looks good to me.
Thanks.
Acked-by: Yasunori Goto <[email protected]>
> On Tue, Nov 27, 2007 at 10:53:45AM -0800, Dave Hansen wrote:
> >On Tue, 2007-11-27 at 10:26 +0800, WANG Cong wrote:
> >>
> >> @@ -414,7 +418,7 @@ int sparse_add_one_section(struct zone *
> >> out:
> >> pgdat_resize_unlock(pgdat, &flags);
> >> if (ret <= 0)
> >> - __kfree_section_memmap(memmap, nr_pages);
> >> + kfree(usemap);
> >> return ret;
> >> }
> >> #endif
> >
> >Why did you get rid of the memmap free here? A bad return from
> >sparse_init_one_section() indicates that we didn't use the memmap, so it
> >will leak otherwise.
>
> Sorry, I was confused by the recursion. This one should be OK.
>
> Thanks.
>
>
>
> Improve the error handling for mm/sparse.c::sparse_add_one_section(). And I
> see no reason to check 'usemap' until holding the 'pgdat_resize_lock'.
>
> Cc: Christoph Lameter <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Rik van Riel <[email protected]>
> Cc: Yasunori Goto <[email protected]>
> Cc: Andy Whitcroft <[email protected]>
> Signed-off-by: WANG Cong <[email protected]>
>
> ---
> Index: linux-2.6/mm/sparse.c
> ===================================================================
> --- linux-2.6.orig/mm/sparse.c
> +++ linux-2.6/mm/sparse.c
> @@ -391,9 +391,17 @@ int sparse_add_one_section(struct zone *
> * no locking for this, because it does its own
> * plus, it does a kmalloc
> */
> - sparse_index_init(section_nr, pgdat->node_id);
> + ret = sparse_index_init(section_nr, pgdat->node_id);
> + if (ret < 0)
> + return ret;
> memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, nr_pages);
> + if (!memmap)
> + return -ENOMEM;
> usemap = __kmalloc_section_usemap();
> + if (!usemap) {
> + __kfree_section_memmap(memmap, nr_pages);
> + return -ENOMEM;
> + }
>
> pgdat_resize_lock(pgdat, &flags);
>
> @@ -403,18 +411,16 @@ int sparse_add_one_section(struct zone *
> goto out;
> }
>
> - if (!usemap) {
> - ret = -ENOMEM;
> - goto out;
> - }
> ms->section_mem_map |= SECTION_MARKED_PRESENT;
>
> ret = sparse_init_one_section(ms, section_nr, memmap, usemap);
>
> out:
> pgdat_resize_unlock(pgdat, &flags);
> - if (ret <= 0)
> + if (ret <= 0) {
> + kfree(usemap);
> __kfree_section_memmap(memmap, nr_pages);
> + }
> return ret;
> }
> #endif
--
Yasunori Goto
This looks fine now.
Acked-by: Dave Hansen <[email protected]>
-- Dave