[PATCH v2 1/4] mm/pgtable: Fix continue to preallocate pmds even if failure occurrence

[PATCH v2 1/4] mm/pgtable: Fix continue to preallocate pmds even if failure occurrence

[Wanpeng Li]

v1 -> v2:
 * remove failed.

preallocate_pmds will continue to preallocate pmds even if failure
occurrence, and then free all the preallocate pmds if there is
failure, this patch fix it by stop preallocate if failure occurrence
and go to free path.

Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 arch/x86/mm/pgtable.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index dfa537a..65c2106 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -196,21 +196,18 @@ static void free_pmds(pmd_t *pmds[])
 static int preallocate_pmds(pmd_t *pmds[])
 {
 	int i;
-	bool failed = false;
 
 	for(i = 0; i < PREALLOCATED_PMDS; i++) {
 		pmd_t *pmd = (pmd_t *)__get_free_page(PGALLOC_GFP);
 		if (pmd == NULL)
-			failed = true;
+			goto err;
 		pmds[i] = pmd;
 	}
 
-	if (failed) {
-		free_pmds(pmds);
-		return -ENOMEM;
-	}
-
 	return 0;
+err:
+	free_pmds(pmds);
+	return -ENOMEM;
 }
 
 /*
-- 
1.8.1.2

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

[PATCH v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

v1 -> v2:
 * add comments to describe alloc_usemap_and_memmap 

After commit 9bdac91424075("sparsemem: Put mem map for one node together."),
vmemmap for one node will be allocated together, its logic is similar as
memory allocation for pageblock flags. This patch introduce alloc_usemap_and_memmap
to extract the same logic of memory alloction for pageblock flags and vmemmap.

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/sparse.c | 140 ++++++++++++++++++++++++++++--------------------------------
 1 file changed, 66 insertions(+), 74 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 308d503..d27db9b 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -439,6 +439,14 @@ static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
 					 map_count, nodeid);
 }
 #else
+
+static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
+				unsigned long pnum_begin,
+				unsigned long pnum_end,
+				unsigned long map_count, int nodeid)
+{
+}
+
 static struct page __init *sparse_early_mem_map_alloc(unsigned long pnum)
 {
 	struct page *map;
@@ -460,6 +468,62 @@ void __attribute__((weak)) __meminit vmemmap_populate_print_last(void)
 {
 }
 
+/**
+ *  alloc_usemap_and_memmap - memory alloction for pageblock flags and vmemmap
+ *  @map: usemap_map for pageblock flags or mmap_map for vmemmap
+ *  @use_map: true if memory allocated for pageblock flags, otherwise false
+ */
+static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
+{
+	unsigned long pnum;
+	unsigned long map_count;
+	int nodeid_begin = 0;
+	unsigned long pnum_begin = 0;
+
+	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
+		struct mem_section *ms;
+
+		if (!present_section_nr(pnum))
+			continue;
+		ms = __nr_to_section(pnum);
+		nodeid_begin = sparse_early_nid(ms);
+		pnum_begin = pnum;
+		break;
+	}
+	map_count = 1;
+	for (pnum = pnum_begin + 1; pnum < NR_MEM_SECTIONS; pnum++) {
+		struct mem_section *ms;
+		int nodeid;
+
+		if (!present_section_nr(pnum))
+			continue;
+		ms = __nr_to_section(pnum);
+		nodeid = sparse_early_nid(ms);
+		if (nodeid == nodeid_begin) {
+			map_count++;
+			continue;
+		}
+		/* ok, we need to take cake of from pnum_begin to pnum - 1*/
+		if (use_map)
+			sparse_early_usemaps_alloc_node(map, pnum_begin, pnum,
+						 map_count, nodeid_begin);
+		else
+			sparse_early_mem_maps_alloc_node((struct page **)map,
+				pnum_begin, pnum, map_count, nodeid_begin);
+		/* new start, update count etc*/
+		nodeid_begin = nodeid;
+		pnum_begin = pnum;
+		map_count = 1;
+	}
+	/* ok, last chunk */
+	if (use_map)
+		sparse_early_usemaps_alloc_node(map, pnum_begin,
+				NR_MEM_SECTIONS, map_count, nodeid_begin);
+	else
+		sparse_early_mem_maps_alloc_node((struct page **)map,
+			pnum_begin, NR_MEM_SECTIONS, map_count, nodeid_begin);
+}
+
 /*
  * Allocate the accumulated non-linear sections, allocate a mem_map
  * for each and record the physical to section mapping.
@@ -471,11 +535,7 @@ void __init sparse_init(void)
 	unsigned long *usemap;
 	unsigned long **usemap_map;
 	int size;
-	int nodeid_begin = 0;
-	unsigned long pnum_begin = 0;
-	unsigned long usemap_count;
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
-	unsigned long map_count;
 	int size2;
 	struct page **map_map;
 #endif
@@ -501,82 +561,14 @@ void __init sparse_init(void)
 	usemap_map = alloc_bootmem(size);
 	if (!usemap_map)
 		panic("can not allocate usemap_map\n");
-
-	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
-		struct mem_section *ms;
-
-		if (!present_section_nr(pnum))
-			continue;
-		ms = __nr_to_section(pnum);
-		nodeid_begin = sparse_early_nid(ms);
-		pnum_begin = pnum;
-		break;
-	}
-	usemap_count = 1;
-	for (pnum = pnum_begin + 1; pnum < NR_MEM_SECTIONS; pnum++) {
-		struct mem_section *ms;
-		int nodeid;
-
-		if (!present_section_nr(pnum))
-			continue;
-		ms = __nr_to_section(pnum);
-		nodeid = sparse_early_nid(ms);
-		if (nodeid == nodeid_begin) {
-			usemap_count++;
-			continue;
-		}
-		/* ok, we need to take cake of from pnum_begin to pnum - 1*/
-		sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, pnum,
-						 usemap_count, nodeid_begin);
-		/* new start, update count etc*/
-		nodeid_begin = nodeid;
-		pnum_begin = pnum;
-		usemap_count = 1;
-	}
-	/* ok, last chunk */
-	sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
-					 usemap_count, nodeid_begin);
+	alloc_usemap_and_memmap(usemap_map, true);
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
 	size2 = sizeof(struct page *) * NR_MEM_SECTIONS;
 	map_map = alloc_bootmem(size2);
 	if (!map_map)
 		panic("can not allocate map_map\n");
-
-	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
-		struct mem_section *ms;
-
-		if (!present_section_nr(pnum))
-			continue;
-		ms = __nr_to_section(pnum);
-		nodeid_begin = sparse_early_nid(ms);
-		pnum_begin = pnum;
-		break;
-	}
-	map_count = 1;
-	for (pnum = pnum_begin + 1; pnum < NR_MEM_SECTIONS; pnum++) {
-		struct mem_section *ms;
-		int nodeid;
-
-		if (!present_section_nr(pnum))
-			continue;
-		ms = __nr_to_section(pnum);
-		nodeid = sparse_early_nid(ms);
-		if (nodeid == nodeid_begin) {
-			map_count++;
-			continue;
-		}
-		/* ok, we need to take cake of from pnum_begin to pnum - 1*/
-		sparse_early_mem_maps_alloc_node(map_map, pnum_begin, pnum,
-						 map_count, nodeid_begin);
-		/* new start, update count etc*/
-		nodeid_begin = nodeid;
-		pnum_begin = pnum;
-		map_count = 1;
-	}
-	/* ok, last chunk */
-	sparse_early_mem_maps_alloc_node(map_map, pnum_begin, NR_MEM_SECTIONS,
-					 map_count, nodeid_begin);
+	alloc_usemap_and_memmap((unsigned long **)map_map, false);
 #endif
 
 	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
-- 
1.8.1.2

--
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 v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Andrew Morton]

On Tue, 20 Aug 2013 14:54:54 +0800 Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:

> v1 -> v2:
>  * add comments to describe alloc_usemap_and_memmap 
> 
> After commit 9bdac91424075("sparsemem: Put mem map for one node together."),
> vmemmap for one node will be allocated together, its logic is similar as
> memory allocation for pageblock flags. This patch introduce alloc_usemap_and_memmap
> to extract the same logic of memory alloction for pageblock flags and vmemmap.
> 

9bdac91424075 was written by Yinghai.  He is an excellent reviewer, as
long as people remember to cc him!

> ---
>  mm/sparse.c | 140 ++++++++++++++++++++++++++++--------------------------------
>  1 file changed, 66 insertions(+), 74 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 308d503..d27db9b 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -439,6 +439,14 @@ static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
>  					 map_count, nodeid);
>  }
>  #else
> +
> +static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
> +				unsigned long pnum_begin,
> +				unsigned long pnum_end,
> +				unsigned long map_count, int nodeid)
> +{
> +}
> +
>  static struct page __init *sparse_early_mem_map_alloc(unsigned long pnum)
>  {
>  	struct page *map;
> @@ -460,6 +468,62 @@ void __attribute__((weak)) __meminit vmemmap_populate_print_last(void)
>  {
>  }
>  
> +/**
> + *  alloc_usemap_and_memmap - memory alloction for pageblock flags and vmemmap
> + *  @map: usemap_map for pageblock flags or mmap_map for vmemmap
> + *  @use_map: true if memory allocated for pageblock flags, otherwise false
> + */
> +static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
> +{
> +	unsigned long pnum;
> +	unsigned long map_count;
> +	int nodeid_begin = 0;
> +	unsigned long pnum_begin = 0;
> +
> +	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
> +		struct mem_section *ms;
> +
> +		if (!present_section_nr(pnum))
> +			continue;
> +		ms = __nr_to_section(pnum);
> +		nodeid_begin = sparse_early_nid(ms);
> +		pnum_begin = pnum;
> +		break;
> +	}
> +	map_count = 1;
> +	for (pnum = pnum_begin + 1; pnum < NR_MEM_SECTIONS; pnum++) {
> +		struct mem_section *ms;
> +		int nodeid;
> +
> +		if (!present_section_nr(pnum))
> +			continue;
> +		ms = __nr_to_section(pnum);
> +		nodeid = sparse_early_nid(ms);
> +		if (nodeid == nodeid_begin) {
> +			map_count++;
> +			continue;
> +		}
> +		/* ok, we need to take cake of from pnum_begin to pnum - 1*/
> +		if (use_map)
> +			sparse_early_usemaps_alloc_node(map, pnum_begin, pnum,
> +						 map_count, nodeid_begin);
> +		else
> +			sparse_early_mem_maps_alloc_node((struct page **)map,
> +				pnum_begin, pnum, map_count, nodeid_begin);
> +		/* new start, update count etc*/
> +		nodeid_begin = nodeid;
> +		pnum_begin = pnum;
> +		map_count = 1;
> +	}
> +	/* ok, last chunk */
> +	if (use_map)
> +		sparse_early_usemaps_alloc_node(map, pnum_begin,
> +				NR_MEM_SECTIONS, map_count, nodeid_begin);
> +	else
> +		sparse_early_mem_maps_alloc_node((struct page **)map,
> +			pnum_begin, NR_MEM_SECTIONS, map_count, nodeid_begin);
> +}
> +
>  /*
>   * Allocate the accumulated non-linear sections, allocate a mem_map
>   * for each and record the physical to section mapping.
> @@ -471,11 +535,7 @@ void __init sparse_init(void)
>  	unsigned long *usemap;
>  	unsigned long **usemap_map;
>  	int size;
> -	int nodeid_begin = 0;
> -	unsigned long pnum_begin = 0;
> -	unsigned long usemap_count;
>  #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
> -	unsigned long map_count;
>  	int size2;
>  	struct page **map_map;
>  #endif
> @@ -501,82 +561,14 @@ void __init sparse_init(void)
>  	usemap_map = alloc_bootmem(size);
>  	if (!usemap_map)
>  		panic("can not allocate usemap_map\n");
> -
> -	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
> -		struct mem_section *ms;
> -
> -		if (!present_section_nr(pnum))
> -			continue;
> -		ms = __nr_to_section(pnum);
> -		nodeid_begin = sparse_early_nid(ms);
> -		pnum_begin = pnum;
> -		break;
> -	}
> -	usemap_count = 1;
> -	for (pnum = pnum_begin + 1; pnum < NR_MEM_SECTIONS; pnum++) {
> -		struct mem_section *ms;
> -		int nodeid;
> -
> -		if (!present_section_nr(pnum))
> -			continue;
> -		ms = __nr_to_section(pnum);
> -		nodeid = sparse_early_nid(ms);
> -		if (nodeid == nodeid_begin) {
> -			usemap_count++;
> -			continue;
> -		}
> -		/* ok, we need to take cake of from pnum_begin to pnum - 1*/
> -		sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, pnum,
> -						 usemap_count, nodeid_begin);
> -		/* new start, update count etc*/
> -		nodeid_begin = nodeid;
> -		pnum_begin = pnum;
> -		usemap_count = 1;
> -	}
> -	/* ok, last chunk */
> -	sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
> -					 usemap_count, nodeid_begin);
> +	alloc_usemap_and_memmap(usemap_map, true);
>  
>  #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
>  	size2 = sizeof(struct page *) * NR_MEM_SECTIONS;
>  	map_map = alloc_bootmem(size2);
>  	if (!map_map)
>  		panic("can not allocate map_map\n");
> -
> -	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
> -		struct mem_section *ms;
> -
> -		if (!present_section_nr(pnum))
> -			continue;
> -		ms = __nr_to_section(pnum);
> -		nodeid_begin = sparse_early_nid(ms);
> -		pnum_begin = pnum;
> -		break;
> -	}
> -	map_count = 1;
> -	for (pnum = pnum_begin + 1; pnum < NR_MEM_SECTIONS; pnum++) {
> -		struct mem_section *ms;
> -		int nodeid;
> -
> -		if (!present_section_nr(pnum))
> -			continue;
> -		ms = __nr_to_section(pnum);
> -		nodeid = sparse_early_nid(ms);
> -		if (nodeid == nodeid_begin) {
> -			map_count++;
> -			continue;
> -		}
> -		/* ok, we need to take cake of from pnum_begin to pnum - 1*/
> -		sparse_early_mem_maps_alloc_node(map_map, pnum_begin, pnum,
> -						 map_count, nodeid_begin);
> -		/* new start, update count etc*/
> -		nodeid_begin = nodeid;
> -		pnum_begin = pnum;
> -		map_count = 1;
> -	}
> -	/* ok, last chunk */
> -	sparse_early_mem_maps_alloc_node(map_map, pnum_begin, NR_MEM_SECTIONS,
> -					 map_count, nodeid_begin);
> +	alloc_usemap_and_memmap((unsigned long **)map_map, false);
>  #endif
>  
>  	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
> -- 
> 1.8.1.2

--
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 v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Yinghai Lu]

On Tue, Aug 20, 2013 at 4:07 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 20 Aug 2013 14:54:54 +0800 Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>
>> v1 -> v2:
>>  * add comments to describe alloc_usemap_and_memmap
>>
>> After commit 9bdac91424075("sparsemem: Put mem map for one node together."),
>> vmemmap for one node will be allocated together, its logic is similar as
>> memory allocation for pageblock flags. This patch introduce alloc_usemap_and_memmap
>> to extract the same logic of memory alloction for pageblock flags and vmemmap.
>>
>
> 9bdac91424075 was written by Yinghai.  He is an excellent reviewer, as
> long as people remember to cc him!

could be that he forgot to use scripts/get_maintainer.pl

or get_maintainer.pl has some problem.

>
>> ---
>>  mm/sparse.c | 140 ++++++++++++++++++++++++++++--------------------------------
>>  1 file changed, 66 insertions(+), 74 deletions(-)
>>
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index 308d503..d27db9b 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -439,6 +439,14 @@ static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
>>                                        map_count, nodeid);
>>  }
>>  #else
>> +
>> +static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
>> +                             unsigned long pnum_begin,
>> +                             unsigned long pnum_end,
>> +                             unsigned long map_count, int nodeid)
>> +{
>> +}
>> +
...
could be avoided, if passing function pointer instead.

>>  static struct page __init *sparse_early_mem_map_alloc(unsigned long pnum)
>>  {
>>       struct page *map;
>> @@ -460,6 +468,62 @@ void __attribute__((weak)) __meminit vmemmap_populate_print_last(void)
>>  {
>>  }
>>
>> +/**
>> + *  alloc_usemap_and_memmap - memory alloction for pageblock flags and vmemmap
>> + *  @map: usemap_map for pageblock flags or mmap_map for vmemmap
>> + *  @use_map: true if memory allocated for pageblock flags, otherwise false
>> + */
>> +static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
...
>> @@ -471,11 +535,7 @@ void __init sparse_init(void)
>>       unsigned long *usemap;
>>       unsigned long **usemap_map;
>>       int size;
...
>> -     /* ok, last chunk */
>> -     sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
>> -                                      usemap_count, nodeid_begin);
>> +     alloc_usemap_and_memmap(usemap_map, true);

alloc_usemap_and_memmap() is somehow confusing.

Please check if you can pass function pointer instead of true/false.

Thanks

Yinghai

--
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 v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

Hi Yinghai,
On Tue, Aug 20, 2013 at 05:02:17PM -0700, Yinghai Lu wrote:
>On Tue, Aug 20, 2013 at 4:07 PM, Andrew Morton
><akpm@linux-foundation.org> wrote:
>> On Tue, 20 Aug 2013 14:54:54 +0800 Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>>
>>> v1 -> v2:
>>>  * add comments to describe alloc_usemap_and_memmap
>>>
>>> After commit 9bdac91424075("sparsemem: Put mem map for one node together."),
>>> vmemmap for one node will be allocated together, its logic is similar as
>>> memory allocation for pageblock flags. This patch introduce alloc_usemap_and_memmap
>>> to extract the same logic of memory alloction for pageblock flags and vmemmap.
>>>
>>
>> 9bdac91424075 was written by Yinghai.  He is an excellent reviewer, as
>> long as people remember to cc him!
>
>could be that he forgot to use scripts/get_maintainer.pl
>
>or get_maintainer.pl has some problem.
>

Sorry for forget cc you. 

>>
>>> ---
>>>  mm/sparse.c | 140 ++++++++++++++++++++++++++++--------------------------------
>>>  1 file changed, 66 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/mm/sparse.c b/mm/sparse.c
>>> index 308d503..d27db9b 100644
>>> --- a/mm/sparse.c
>>> +++ b/mm/sparse.c
>>> @@ -439,6 +439,14 @@ static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
>>>                                        map_count, nodeid);
>>>  }
>>>  #else
>>> +
>>> +static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
>>> +                             unsigned long pnum_begin,
>>> +                             unsigned long pnum_end,
>>> +                             unsigned long map_count, int nodeid)
>>> +{
>>> +}
>>> +
>...
>could be avoided, if passing function pointer instead.
>
>>>  static struct page __init *sparse_early_mem_map_alloc(unsigned long pnum)
>>>  {
>>>       struct page *map;
>>> @@ -460,6 +468,62 @@ void __attribute__((weak)) __meminit vmemmap_populate_print_last(void)
>>>  {
>>>  }
>>>
>>> +/**
>>> + *  alloc_usemap_and_memmap - memory alloction for pageblock flags and vmemmap
>>> + *  @map: usemap_map for pageblock flags or mmap_map for vmemmap
>>> + *  @use_map: true if memory allocated for pageblock flags, otherwise false
>>> + */
>>> +static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
>...
>>> @@ -471,11 +535,7 @@ void __init sparse_init(void)
>>>       unsigned long *usemap;
>>>       unsigned long **usemap_map;
>>>       int size;
>...
>>> -     /* ok, last chunk */
>>> -     sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
>>> -                                      usemap_count, nodeid_begin);
>>> +     alloc_usemap_and_memmap(usemap_map, true);
>
>alloc_usemap_and_memmap() is somehow confusing.
>
>Please check if you can pass function pointer instead of true/false.
>

sparse_early_usemaps_alloc_node and sparse_early_mem_maps_alloc_node is
similar, however, one has a parameter unsigned long ** and the other has 
struct page **. function pointer can't help, isn't it? ;-)

Regards,
Wanpeng Li 

>Thanks
>
>Yinghai
>
>--
>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 v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

Hi Yinghai,
On Tue, Aug 20, 2013 at 05:02:17PM -0700, Yinghai Lu wrote:
>On Tue, Aug 20, 2013 at 4:07 PM, Andrew Morton
><akpm@linux-foundation.org> wrote:
>> On Tue, 20 Aug 2013 14:54:54 +0800 Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>>
>>> v1 -> v2:
>>>  * add comments to describe alloc_usemap_and_memmap
>>>
>>> After commit 9bdac91424075("sparsemem: Put mem map for one node together."),
>>> vmemmap for one node will be allocated together, its logic is similar as
>>> memory allocation for pageblock flags. This patch introduce alloc_usemap_and_memmap
>>> to extract the same logic of memory alloction for pageblock flags and vmemmap.
>>>
>>
>> 9bdac91424075 was written by Yinghai.  He is an excellent reviewer, as
>> long as people remember to cc him!
>
>could be that he forgot to use scripts/get_maintainer.pl
>
>or get_maintainer.pl has some problem.
>

Sorry for forget cc you. 

>>
>>> ---
>>>  mm/sparse.c | 140 ++++++++++++++++++++++++++++--------------------------------
>>>  1 file changed, 66 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/mm/sparse.c b/mm/sparse.c
>>> index 308d503..d27db9b 100644
>>> --- a/mm/sparse.c
>>> +++ b/mm/sparse.c
>>> @@ -439,6 +439,14 @@ static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
>>>                                        map_count, nodeid);
>>>  }
>>>  #else
>>> +
>>> +static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
>>> +                             unsigned long pnum_begin,
>>> +                             unsigned long pnum_end,
>>> +                             unsigned long map_count, int nodeid)
>>> +{
>>> +}
>>> +
>...
>could be avoided, if passing function pointer instead.
>
>>>  static struct page __init *sparse_early_mem_map_alloc(unsigned long pnum)
>>>  {
>>>       struct page *map;
>>> @@ -460,6 +468,62 @@ void __attribute__((weak)) __meminit vmemmap_populate_print_last(void)
>>>  {
>>>  }
>>>
>>> +/**
>>> + *  alloc_usemap_and_memmap - memory alloction for pageblock flags and vmemmap
>>> + *  @map: usemap_map for pageblock flags or mmap_map for vmemmap
>>> + *  @use_map: true if memory allocated for pageblock flags, otherwise false
>>> + */
>>> +static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
>...
>>> @@ -471,11 +535,7 @@ void __init sparse_init(void)
>>>       unsigned long *usemap;
>>>       unsigned long **usemap_map;
>>>       int size;
>...
>>> -     /* ok, last chunk */
>>> -     sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
>>> -                                      usemap_count, nodeid_begin);
>>> +     alloc_usemap_and_memmap(usemap_map, true);
>
>alloc_usemap_and_memmap() is somehow confusing.
>
>Please check if you can pass function pointer instead of true/false.
>

sparse_early_usemaps_alloc_node and sparse_early_mem_maps_alloc_node is
similar, however, one has a parameter unsigned long ** and the other has 
struct page **. function pointer can't help, isn't it? ;-)

Regards,
Wanpeng Li 

>Thanks
>
>Yinghai
>
>--
>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 v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Yinghai Lu]

On Tue, Aug 20, 2013 at 8:11 PM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
> Hi Yinghai,
> On Tue, Aug 20, 2013 at 05:02:17PM -0700, Yinghai Lu wrote:
>>>> -     /* ok, last chunk */
>>>> -     sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
>>>> -                                      usemap_count, nodeid_begin);
>>>> +     alloc_usemap_and_memmap(usemap_map, true);
>>
>>alloc_usemap_and_memmap() is somehow confusing.
>>
>>Please check if you can pass function pointer instead of true/false.
>>
>
> sparse_early_usemaps_alloc_node and sparse_early_mem_maps_alloc_node is
> similar, however, one has a parameter unsigned long ** and the other has
> struct page **. function pointer can't help, isn't it? ;-)

you could have one generic function pointer like
void *alloc_func(void *data);

and in the every alloc function, have own struct data to pass in/out...

Yinghai

--
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 v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

Hi Yinghai,
On Tue, Aug 20, 2013 at 09:28:29PM -0700, Yinghai Lu wrote:
>On Tue, Aug 20, 2013 at 8:11 PM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>> Hi Yinghai,
>> On Tue, Aug 20, 2013 at 05:02:17PM -0700, Yinghai Lu wrote:
>>>>> -     /* ok, last chunk */
>>>>> -     sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
>>>>> -                                      usemap_count, nodeid_begin);
>>>>> +     alloc_usemap_and_memmap(usemap_map, true);
>>>
>>>alloc_usemap_and_memmap() is somehow confusing.
>>>
>>>Please check if you can pass function pointer instead of true/false.
>>>
>>
>> sparse_early_usemaps_alloc_node and sparse_early_mem_maps_alloc_node is
>> similar, however, one has a parameter unsigned long ** and the other has
>> struct page **. function pointer can't help, isn't it? ;-)
>
>you could have one generic function pointer like
>void *alloc_func(void *data);
>
>and in the every alloc function, have own struct data to pass in/out...
>
>Yinghai

How about this?


[-- Attachment #2: 0001-sparse.patch --]
[-- Type: text/x-diff, Size: 5285 bytes --]

>From a78e12a9ff31f2a73b87145ce7ad943a0f712708 Mon Sep 17 00:00:00 2001
From: Wanpeng Li <liwanp@linux.vnet.ibm.com>
Date: Wed, 21 Aug 2013 15:23:08 +0800
Subject: [PATCH] mm/sparse: introduce alloc_usemap_and_memmap fix 

Pass function pointer to alloc_usemap_and_memmap() instead of true/false. 

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/sparse.c |   54 +++++++++++++++++++++++++-----------------------------
 1 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 55e5752..06adf3c 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -339,14 +339,16 @@ static void __init check_usemap_section_nr(int nid, unsigned long *usemap)
 }
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
-static void __init sparse_early_usemaps_alloc_node(unsigned long**usemap_map,
+static void __init sparse_early_usemaps_alloc_node(void **usemap_map,
 				 unsigned long pnum_begin,
 				 unsigned long pnum_end,
 				 unsigned long usemap_count, int nodeid)
 {
 	void *usemap;
 	unsigned long pnum;
+	unsigned long **map;
 	int size = usemap_size();
+	map = (unsigned long **) usemap_map;
 
 	usemap = sparse_early_usemaps_alloc_pgdat_section(NODE_DATA(nodeid),
 							  size * usemap_count);
@@ -358,9 +360,9 @@ static void __init sparse_early_usemaps_alloc_node(unsigned long**usemap_map,
 	for (pnum = pnum_begin; pnum < pnum_end; pnum++) {
 		if (!present_section_nr(pnum))
 			continue;
-		usemap_map[pnum] = usemap;
+		map[pnum] = usemap;
 		usemap += size;
-		check_usemap_section_nr(nodeid, usemap_map[pnum]);
+		check_usemap_section_nr(nodeid, map[pnum]);
 	}
 }
 
@@ -430,23 +432,16 @@ void __init sparse_mem_maps_populate_node(struct page **map_map,
 #endif /* !CONFIG_SPARSEMEM_VMEMMAP */
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
-static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
+static void __init sparse_early_mem_maps_alloc_node(void **map_map,
 				 unsigned long pnum_begin,
 				 unsigned long pnum_end,
 				 unsigned long map_count, int nodeid)
 {
-	sparse_mem_maps_populate_node(map_map, pnum_begin, pnum_end,
+	struct page **map = (struct page **)map_map;
+	sparse_mem_maps_populate_node(map, pnum_begin, pnum_end,
 					 map_count, nodeid);
 }
 #else
-
-static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
-				unsigned long pnum_begin,
-				unsigned long pnum_end,
-				unsigned long map_count, int nodeid)
-{
-}
-
 static struct page __init *sparse_early_mem_map_alloc(unsigned long pnum)
 {
 	struct page *map;
@@ -471,9 +466,10 @@ void __attribute__((weak)) __meminit vmemmap_populate_print_last(void)
 /**
  *  alloc_usemap_and_memmap - memory alloction for pageblock flags and vmemmap
  *  @map: usemap_map for pageblock flags or mmap_map for vmemmap
- *  @use_map: true if memory allocated for pageblock flags, otherwise false
  */
-static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
+static void alloc_usemap_and_memmap(void (*sparse_early_maps_alloc_node)
+				(void **, unsigned long, unsigned long,
+				unsigned long, int), void **map)
 {
 	unsigned long pnum;
 	unsigned long map_count;
@@ -504,24 +500,16 @@ static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
 			continue;
 		}
 		/* ok, we need to take cake of from pnum_begin to pnum - 1*/
-		if (use_map)
-			sparse_early_usemaps_alloc_node(map, pnum_begin, pnum,
-						 map_count, nodeid_begin);
-		else
-			sparse_early_mem_maps_alloc_node((struct page **)map,
-				pnum_begin, pnum, map_count, nodeid_begin);
+		(*sparse_early_maps_alloc_node)(map, pnum_begin, pnum,
+					map_count, nodeid_begin);
 		/* new start, update count etc*/
 		nodeid_begin = nodeid;
 		pnum_begin = pnum;
 		map_count = 1;
 	}
 	/* ok, last chunk */
-	if (use_map)
-		sparse_early_usemaps_alloc_node(map, pnum_begin,
-				NR_MEM_SECTIONS, map_count, nodeid_begin);
-	else
-		sparse_early_mem_maps_alloc_node((struct page **)map,
-			pnum_begin, NR_MEM_SECTIONS, map_count, nodeid_begin);
+	(*sparse_early_maps_alloc_node)(map, pnum_begin, NR_MEM_SECTIONS,
+					map_count, nodeid_begin);
 }
 
 /*
@@ -540,6 +528,10 @@ void __init sparse_init(void)
 	struct page **map_map;
 #endif
 
+	void (*sparse_early_maps_alloc_node)(void **map,
+			unsigned long pnum_begin, unsigned long pnum_end,
+				unsigned long map_count, int nodeid);
+
 	/* see include/linux/mmzone.h 'struct mem_section' definition */
 	BUILD_BUG_ON(!is_power_of_2(sizeof(struct mem_section)));
 
@@ -561,14 +553,18 @@ void __init sparse_init(void)
 	usemap_map = alloc_bootmem(size);
 	if (!usemap_map)
 		panic("can not allocate usemap_map\n");
-	alloc_usemap_and_memmap(usemap_map, true);
+	sparse_early_maps_alloc_node = sparse_early_usemaps_alloc_node;
+	alloc_usemap_and_memmap(sparse_early_maps_alloc_node,
+						(void **)usemap_map);
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
 	size2 = sizeof(struct page *) * NR_MEM_SECTIONS;
 	map_map = alloc_bootmem(size2);
 	if (!map_map)
 		panic("can not allocate map_map\n");
-	alloc_usemap_and_memmap((unsigned long **)map_map, false);
+	sparse_early_maps_alloc_node = sparse_early_mem_maps_alloc_node;
+	alloc_usemap_and_memmap(sparse_early_maps_alloc_node,
+						(void **)map_map);
 #endif
 
 	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
-- 
1.7.7.6

Re: [PATCH v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

Hi Yinghai,
On Tue, Aug 20, 2013 at 09:28:29PM -0700, Yinghai Lu wrote:
>On Tue, Aug 20, 2013 at 8:11 PM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>> Hi Yinghai,
>> On Tue, Aug 20, 2013 at 05:02:17PM -0700, Yinghai Lu wrote:
>>>>> -     /* ok, last chunk */
>>>>> -     sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
>>>>> -                                      usemap_count, nodeid_begin);
>>>>> +     alloc_usemap_and_memmap(usemap_map, true);
>>>
>>>alloc_usemap_and_memmap() is somehow confusing.
>>>
>>>Please check if you can pass function pointer instead of true/false.
>>>
>>
>> sparse_early_usemaps_alloc_node and sparse_early_mem_maps_alloc_node is
>> similar, however, one has a parameter unsigned long ** and the other has
>> struct page **. function pointer can't help, isn't it? ;-)
>
>you could have one generic function pointer like
>void *alloc_func(void *data);
>
>and in the every alloc function, have own struct data to pass in/out...
>
>Yinghai

How about this?


[-- Attachment #2: 0001-sparse.patch --]
[-- Type: text/x-diff, Size: 0 bytes --]

Re: [PATCH v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Yinghai Lu]

On Wed, Aug 21, 2013 at 12:29 AM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
> Hi Yinghai,
> On Tue, Aug 20, 2013 at 09:28:29PM -0700, Yinghai Lu wrote:
>>On Tue, Aug 20, 2013 at 8:11 PM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>>> Hi Yinghai,
>>> On Tue, Aug 20, 2013 at 05:02:17PM -0700, Yinghai Lu wrote:
>>>>>> -     /* ok, last chunk */
>>>>>> -     sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
>>>>>> -                                      usemap_count, nodeid_begin);
>>>>>> +     alloc_usemap_and_memmap(usemap_map, true);
>>>>
>>>>alloc_usemap_and_memmap() is somehow confusing.
>>>>
>>>>Please check if you can pass function pointer instead of true/false.
>>>>
>>>
>>> sparse_early_usemaps_alloc_node and sparse_early_mem_maps_alloc_node is
>>> similar, however, one has a parameter unsigned long ** and the other has
>>> struct page **. function pointer can't help, isn't it? ;-)
>>
>>you could have one generic function pointer like
>>void *alloc_func(void *data);
>>
>>and in the every alloc function, have own struct data to pass in/out...
>>
>>Yinghai
>
> How about this?

Re: [PATCH v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]

On Wed, Aug 21, 2013 at 10:19:52PM -0700, Yinghai Lu wrote:
>On Wed, Aug 21, 2013 at 12:29 AM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>> Hi Yinghai,
>> On Tue, Aug 20, 2013 at 09:28:29PM -0700, Yinghai Lu wrote:
>>>On Tue, Aug 20, 2013 at 8:11 PM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>>>> Hi Yinghai,
>>>> On Tue, Aug 20, 2013 at 05:02:17PM -0700, Yinghai Lu wrote:
>>>>>>> -     /* ok, last chunk */
>>>>>>> -     sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
>>>>>>> -                                      usemap_count, nodeid_begin);
>>>>>>> +     alloc_usemap_and_memmap(usemap_map, true);
>>>>>
>>>>>alloc_usemap_and_memmap() is somehow confusing.
>>>>>
>>>>>Please check if you can pass function pointer instead of true/false.
>>>>>
>>>>
>>>> sparse_early_usemaps_alloc_node and sparse_early_mem_maps_alloc_node is
>>>> similar, however, one has a parameter unsigned long ** and the other has
>>>> struct page **. function pointer can't help, isn't it? ;-)
>>>
>>>you could have one generic function pointer like
>>>void *alloc_func(void *data);
>>>
>>>and in the every alloc function, have own struct data to pass in/out...
>>>

How about this one?


[-- Attachment #2: 0001-2.patch --]
[-- Type: text/x-diff, Size: 0 bytes --]

Re: [PATCH v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On Thu, Aug 22, 2013 at 08:08:09PM +0800, Wanpeng Li wrote:
>On Wed, Aug 21, 2013 at 10:19:52PM -0700, Yinghai Lu wrote:
>>On Wed, Aug 21, 2013 at 12:29 AM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>>> Hi Yinghai,
>>> On Tue, Aug 20, 2013 at 09:28:29PM -0700, Yinghai Lu wrote:
>>>>On Tue, Aug 20, 2013 at 8:11 PM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>>>>> Hi Yinghai,
>>>>> On Tue, Aug 20, 2013 at 05:02:17PM -0700, Yinghai Lu wrote:
>>>>>>>> -     /* ok, last chunk */
>>>>>>>> -     sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
>>>>>>>> -                                      usemap_count, nodeid_begin);
>>>>>>>> +     alloc_usemap_and_memmap(usemap_map, true);
>>>>>>
>>>>>>alloc_usemap_and_memmap() is somehow confusing.
>>>>>>
>>>>>>Please check if you can pass function pointer instead of true/false.
>>>>>>
>>>>>
>>>>> sparse_early_usemaps_alloc_node and sparse_early_mem_maps_alloc_node is
>>>>> similar, however, one has a parameter unsigned long ** and the other has
>>>>> struct page **. function pointer can't help, isn't it? ;-)
>>>>
>>>>you could have one generic function pointer like
>>>>void *alloc_func(void *data);
>>>>
>>>>and in the every alloc function, have own struct data to pass in/out...
>>>>
>

Sorry send you the wrong version, how about this one?



[-- Attachment #2: 0001-2.patch --]
[-- Type: text/x-diff, Size: 0 bytes --]

Re: [PATCH v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On Thu, Aug 22, 2013 at 08:08:09PM +0800, Wanpeng Li wrote:
>On Wed, Aug 21, 2013 at 10:19:52PM -0700, Yinghai Lu wrote:
>>On Wed, Aug 21, 2013 at 12:29 AM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>>> Hi Yinghai,
>>> On Tue, Aug 20, 2013 at 09:28:29PM -0700, Yinghai Lu wrote:
>>>>On Tue, Aug 20, 2013 at 8:11 PM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>>>>> Hi Yinghai,
>>>>> On Tue, Aug 20, 2013 at 05:02:17PM -0700, Yinghai Lu wrote:
>>>>>>>> -     /* ok, last chunk */
>>>>>>>> -     sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
>>>>>>>> -                                      usemap_count, nodeid_begin);
>>>>>>>> +     alloc_usemap_and_memmap(usemap_map, true);
>>>>>>
>>>>>>alloc_usemap_and_memmap() is somehow confusing.
>>>>>>
>>>>>>Please check if you can pass function pointer instead of true/false.
>>>>>>
>>>>>
>>>>> sparse_early_usemaps_alloc_node and sparse_early_mem_maps_alloc_node is
>>>>> similar, however, one has a parameter unsigned long ** and the other has
>>>>> struct page **. function pointer can't help, isn't it? ;-)
>>>>
>>>>you could have one generic function pointer like
>>>>void *alloc_func(void *data);
>>>>
>>>>and in the every alloc function, have own struct data to pass in/out...
>>>>
>

Sorry send you the wrong version, how about this one?



[-- Attachment #2: 0001-2.patch --]
[-- Type: text/x-diff, Size: 5906 bytes --]

>From f21640b6dc15c76ac10fccada96e6b9fdce5a092 Mon Sep 17 00:00:00 2001
From: Wanpeng Li <liwanp@linux.vnet.ibm.com>
Date: Thu, 22 Aug 2013 19:57:54 +0800
Subject: [PATCH] mm/sparse: introduce alloc_usemap_and_memmap fix

Pass function pointer to alloc_usemap_and_memmap() instead of true/false.

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/sparse.c | 78 ++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 55e5752..22a1a26 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -14,6 +14,14 @@
 #include <asm/pgalloc.h>
 #include <asm/pgtable.h>
 
+struct alloc_info {
+	unsigned long **map_map;
+	unsigned long pnum_begin;
+	unsigned long pnum_end;
+	unsigned long map_count;
+	int nodeid;
+};
+
 /*
  * Permanent SPARSEMEM data:
  *
@@ -339,13 +347,16 @@ static void __init check_usemap_section_nr(int nid, unsigned long *usemap)
 }
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
-static void __init sparse_early_usemaps_alloc_node(unsigned long**usemap_map,
-				 unsigned long pnum_begin,
-				 unsigned long pnum_end,
-				 unsigned long usemap_count, int nodeid)
+static void __init sparse_early_usemaps_alloc_node(void *data)
 {
 	void *usemap;
 	unsigned long pnum;
+	struct alloc_info *info = (struct alloc_info *)data;
+	unsigned long **usemap_map = info->map_map;
+	unsigned long pnum_begin = info->pnum_begin;
+	unsigned long pnum_end = info->pnum_end;
+	unsigned long usemap_count = info->map_count;
+	int nodeid = info->nodeid;
 	int size = usemap_size();
 
 	usemap = sparse_early_usemaps_alloc_pgdat_section(NODE_DATA(nodeid),
@@ -430,23 +441,13 @@ void __init sparse_mem_maps_populate_node(struct page **map_map,
 #endif /* !CONFIG_SPARSEMEM_VMEMMAP */
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
-static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
-				 unsigned long pnum_begin,
-				 unsigned long pnum_end,
-				 unsigned long map_count, int nodeid)
+static void __init sparse_early_mem_maps_alloc_node(void *data)
 {
-	sparse_mem_maps_populate_node(map_map, pnum_begin, pnum_end,
-					 map_count, nodeid);
+	struct alloc_info *info = (struct alloc_info *)data;
+	sparse_mem_maps_populate_node((struct page **)info->map_map,
+	info->pnum_begin, info->pnum_end, info->map_count, info->nodeid);
 }
 #else
-
-static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
-				unsigned long pnum_begin,
-				unsigned long pnum_end,
-				unsigned long map_count, int nodeid)
-{
-}
-
 static struct page __init *sparse_early_mem_map_alloc(unsigned long pnum)
 {
 	struct page *map;
@@ -471,14 +472,15 @@ void __attribute__((weak)) __meminit vmemmap_populate_print_last(void)
 /**
  *  alloc_usemap_and_memmap - memory alloction for pageblock flags and vmemmap
  *  @map: usemap_map for pageblock flags or mmap_map for vmemmap
- *  @use_map: true if memory allocated for pageblock flags, otherwise false
  */
-static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
+static void alloc_usemap_and_memmap(void (*sparse_early_maps_alloc_node)
+				(void *data), void *data)
 {
 	unsigned long pnum;
 	unsigned long map_count;
 	int nodeid_begin = 0;
 	unsigned long pnum_begin = 0;
+	struct alloc_info *info = (struct alloc_info *)data;
 
 	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
 		struct mem_section *ms;
@@ -503,25 +505,24 @@ static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
 			map_count++;
 			continue;
 		}
+
+		info->pnum_begin = pnum_begin;
+		info->pnum_end = pnum;
+		info->map_count = map_count;
+		info->nodeid = nodeid_begin;
 		/* ok, we need to take cake of from pnum_begin to pnum - 1*/
-		if (use_map)
-			sparse_early_usemaps_alloc_node(map, pnum_begin, pnum,
-						 map_count, nodeid_begin);
-		else
-			sparse_early_mem_maps_alloc_node((struct page **)map,
-				pnum_begin, pnum, map_count, nodeid_begin);
+		sparse_early_maps_alloc_node((void *)info);
 		/* new start, update count etc*/
 		nodeid_begin = nodeid;
 		pnum_begin = pnum;
 		map_count = 1;
 	}
 	/* ok, last chunk */
-	if (use_map)
-		sparse_early_usemaps_alloc_node(map, pnum_begin,
-				NR_MEM_SECTIONS, map_count, nodeid_begin);
-	else
-		sparse_early_mem_maps_alloc_node((struct page **)map,
-			pnum_begin, NR_MEM_SECTIONS, map_count, nodeid_begin);
+	info->pnum_begin = pnum_begin;
+	info->pnum_end = NR_MEM_SECTIONS;
+	info->map_count = map_count;
+	info->nodeid = nodeid_begin;
+	sparse_early_maps_alloc_node((void *)info);
 }
 
 /*
@@ -535,11 +536,14 @@ void __init sparse_init(void)
 	unsigned long *usemap;
 	unsigned long **usemap_map;
 	int size;
+	struct alloc_info data;
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
 	int size2;
 	struct page **map_map;
 #endif
 
+	void (*sparse_early_maps_alloc_node)(void *data);
+
 	/* see include/linux/mmzone.h 'struct mem_section' definition */
 	BUILD_BUG_ON(!is_power_of_2(sizeof(struct mem_section)));
 
@@ -561,14 +565,20 @@ void __init sparse_init(void)
 	usemap_map = alloc_bootmem(size);
 	if (!usemap_map)
 		panic("can not allocate usemap_map\n");
-	alloc_usemap_and_memmap(usemap_map, true);
+	sparse_early_maps_alloc_node = sparse_early_usemaps_alloc_node;
+	data.map_map = usemap_map;
+	alloc_usemap_and_memmap(sparse_early_maps_alloc_node, (void *)&data);
+	usemap_map = data.map_map;
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
 	size2 = sizeof(struct page *) * NR_MEM_SECTIONS;
 	map_map = alloc_bootmem(size2);
 	if (!map_map)
 		panic("can not allocate map_map\n");
-	alloc_usemap_and_memmap((unsigned long **)map_map, false);
+	sparse_early_maps_alloc_node = sparse_early_mem_maps_alloc_node;
+	data.map_map = (unsigned long **)map_map;
+	alloc_usemap_and_memmap(sparse_early_maps_alloc_node, (void *)&data);
+	map_map = (struct page **)data.map_map;
 #endif
 
 	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
-- 
1.8.1.2

Re: [PATCH v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]

On Wed, Aug 21, 2013 at 10:19:52PM -0700, Yinghai Lu wrote:
>On Wed, Aug 21, 2013 at 12:29 AM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>> Hi Yinghai,
>> On Tue, Aug 20, 2013 at 09:28:29PM -0700, Yinghai Lu wrote:
>>>On Tue, Aug 20, 2013 at 8:11 PM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>>>> Hi Yinghai,
>>>> On Tue, Aug 20, 2013 at 05:02:17PM -0700, Yinghai Lu wrote:
>>>>>>> -     /* ok, last chunk */
>>>>>>> -     sparse_early_usemaps_alloc_node(usemap_map, pnum_begin, NR_MEM_SECTIONS,
>>>>>>> -                                      usemap_count, nodeid_begin);
>>>>>>> +     alloc_usemap_and_memmap(usemap_map, true);
>>>>>
>>>>>alloc_usemap_and_memmap() is somehow confusing.
>>>>>
>>>>>Please check if you can pass function pointer instead of true/false.
>>>>>
>>>>
>>>> sparse_early_usemaps_alloc_node and sparse_early_mem_maps_alloc_node is
>>>> similar, however, one has a parameter unsigned long ** and the other has
>>>> struct page **. function pointer can't help, isn't it? ;-)
>>>
>>>you could have one generic function pointer like
>>>void *alloc_func(void *data);
>>>
>>>and in the every alloc function, have own struct data to pass in/out...
>>>

How about this one?


[-- Attachment #2: 0001-2.patch --]
[-- Type: text/x-diff, Size: 5912 bytes --]

>From f21640b6dc15c76ac10fccada96e6b9fdce5a092 Mon Sep 17 00:00:00 2001
From: Wanpeng Li <liwanp@linux.vnet.ibm.com>
Date: Thu, 22 Aug 2013 19:57:54 +0800
Subject: [PATCH] mm/sparse: introduce alloc_usemap_and_memmap fix

Pass function pointer to alloc_usemap_and_memmap() instead of true/false.

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/sparse.c | 78 ++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 55e5752..22a1a26 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -14,6 +14,14 @@
 #include <asm/pgalloc.h>
 #include <asm/pgtable.h>
 
+struct alloc_info {
+	unsigned long **map_map;
+	unsigned long pnum_begin;
+	unsigned long pnum_end;
+	unsigned long map_count;
+	int nodeid;
+};
+
 /*
  * Permanent SPARSEMEM data:
  *
@@ -339,13 +347,16 @@ static void __init check_usemap_section_nr(int nid, unsigned long *usemap)
 }
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
-static void __init sparse_early_usemaps_alloc_node(unsigned long**usemap_map,
-				 unsigned long pnum_begin,
-				 unsigned long pnum_end,
-				 unsigned long usemap_count, int nodeid)
+static void __init sparse_early_usemaps_alloc_node(void *data)
 {
 	void *usemap;
 	unsigned long pnum;
+	struct alloc_info *info = (struct alloc_info *)data;
+	unsigned long **usemap_map = info->map_map;
+	unsigned long pnum_begin = info->pnum_begin;
+	unsigned long pnum_end = info->pnum_end;
+	unsigned long usemap_count = info->map_count;
+	int nodeid = info->nodeid;
 	int size = usemap_size();
 
 	usemap = sparse_early_usemaps_alloc_pgdat_section(NODE_DATA(nodeid),
@@ -430,23 +441,13 @@ void __init sparse_mem_maps_populate_node(struct page **map_map,
 #endif /* !CONFIG_SPARSEMEM_VMEMMAP */
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
-static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
-				 unsigned long pnum_begin,
-				 unsigned long pnum_end,
-				 unsigned long map_count, int nodeid)
+static void __init sparse_early_mem_maps_alloc_node(void *data)
 {
-	sparse_mem_maps_populate_node(map_map, pnum_begin, pnum_end,
-					 map_count, nodeid);
+	struct alloc_info *info = (struct alloc_info *)data;
+	sparse_mem_maps_populate_node((struct page **)info->map_map,
+	info->pnum_begin, info->pnum_end, info->map_count, info->nodeid);
 }
 #else
-
-static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
-				unsigned long pnum_begin,
-				unsigned long pnum_end,
-				unsigned long map_count, int nodeid)
-{
-}
-
 static struct page __init *sparse_early_mem_map_alloc(unsigned long pnum)
 {
 	struct page *map;
@@ -471,14 +472,15 @@ void __attribute__((weak)) __meminit vmemmap_populate_print_last(void)
 /**
  *  alloc_usemap_and_memmap - memory alloction for pageblock flags and vmemmap
  *  @map: usemap_map for pageblock flags or mmap_map for vmemmap
- *  @use_map: true if memory allocated for pageblock flags, otherwise false
  */
-static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
+static void alloc_usemap_and_memmap(void (*sparse_early_maps_alloc_node)
+				(void *data), void *data)
 {
 	unsigned long pnum;
 	unsigned long map_count;
 	int nodeid_begin = 0;
 	unsigned long pnum_begin = 0;
+	struct alloc_info *info = (struct alloc_info *)data;
 
 	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
 		struct mem_section *ms;
@@ -503,25 +505,24 @@ static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
 			map_count++;
 			continue;
 		}
+
+		info->pnum_begin = pnum_begin;
+		info->pnum_end = pnum;
+		info->map_count = map_count;
+		info->nodeid = nodeid_begin;
 		/* ok, we need to take cake of from pnum_begin to pnum - 1*/
-		if (use_map)
-			sparse_early_usemaps_alloc_node(map, pnum_begin, pnum,
-						 map_count, nodeid_begin);
-		else
-			sparse_early_mem_maps_alloc_node((struct page **)map,
-				pnum_begin, pnum, map_count, nodeid_begin);
+		(*sparse_early_maps_alloc_node)((void *)info);
 		/* new start, update count etc*/
 		nodeid_begin = nodeid;
 		pnum_begin = pnum;
 		map_count = 1;
 	}
 	/* ok, last chunk */
-	if (use_map)
-		sparse_early_usemaps_alloc_node(map, pnum_begin,
-				NR_MEM_SECTIONS, map_count, nodeid_begin);
-	else
-		sparse_early_mem_maps_alloc_node((struct page **)map,
-			pnum_begin, NR_MEM_SECTIONS, map_count, nodeid_begin);
+	info->pnum_begin = pnum_begin;
+	info->pnum_end = NR_MEM_SECTIONS;
+	info->map_count = map_count;
+	info->nodeid = nodeid_begin;
+	(*sparse_early_maps_alloc_node)((void *)info);
 }
 
 /*
@@ -535,11 +536,14 @@ void __init sparse_init(void)
 	unsigned long *usemap;
 	unsigned long **usemap_map;
 	int size;
+	struct alloc_info data;
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
 	int size2;
 	struct page **map_map;
 #endif
 
+	void (*sparse_early_maps_alloc_node)(void *data);
+
 	/* see include/linux/mmzone.h 'struct mem_section' definition */
 	BUILD_BUG_ON(!is_power_of_2(sizeof(struct mem_section)));
 
@@ -561,14 +565,20 @@ void __init sparse_init(void)
 	usemap_map = alloc_bootmem(size);
 	if (!usemap_map)
 		panic("can not allocate usemap_map\n");
-	alloc_usemap_and_memmap(usemap_map, true);
+	sparse_early_maps_alloc_node = sparse_early_usemaps_alloc_node;
+	data.map_map = usemap_map;
+	alloc_usemap_and_memmap(sparse_early_maps_alloc_node, (void *)&data);
+	usemap_map = data.map_map;
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
 	size2 = sizeof(struct page *) * NR_MEM_SECTIONS;
 	map_map = alloc_bootmem(size2);
 	if (!map_map)
 		panic("can not allocate map_map\n");
-	alloc_usemap_and_memmap((unsigned long **)map_map, false);
+	sparse_early_maps_alloc_node = sparse_early_mem_maps_alloc_node;
+	data.map_map = (unsigned long **)map_map;
+	alloc_usemap_and_memmap(sparse_early_maps_alloc_node, (void *)&data);
+	map_map = (struct page **)data.map_map;
 #endif
 
 	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
-- 
1.8.1.2

Re: [PATCH v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Yinghai Lu]

On Thu, Aug 22, 2013 at 5:14 AM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:

Re: [PATCH v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

On Wed, Aug 28, 2013 at 7:18 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> please change to function pointer to
> void  (*alloc_func)(void *data,
>                                  unsigned long pnum_begin,
>                                  unsigned long pnum_end,
>                                  unsigned long map_count, int nodeid)
>
> pnum_begin, pnum_end, map_coun, nodeid, should not be in the struct.

looks like that is what is your first version did.

I updated it a little bit. please check it.

Thanks

Yinghai

[-- Attachment #2: 0001-sparse_v2.patch --]
[-- Type: application/octet-stream, Size: 4870 bytes --]

From a78e12a9ff31f2a73b87145ce7ad943a0f712708 Mon Sep 17 00:00:00 2001
From: Wanpeng Li <liwanp@linux.vnet.ibm.com>
Date: Wed, 21 Aug 2013 15:23:08 +0800
Subject: [PATCH] mm/sparse: introduce alloc_usemap_and_memmap fix 

Pass function pointer to alloc_usemap_and_memmap() instead of true/false. 

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/sparse.c |   54 +++++++++++++++++++++++++-----------------------------
 1 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 55e5752..06adf3c 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -339,14 +339,16 @@ static void __init check_usemap_section_nr(int nid, unsigned long *usemap)
 }
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
-static void __init sparse_early_usemaps_alloc_node(unsigned long**usemap_map,
+static void __init sparse_early_usemaps_alloc_node(void **usemap_map,
 				 unsigned long pnum_begin,
 				 unsigned long pnum_end,
 				 unsigned long usemap_count, int nodeid)
 {
 	void *usemap;
 	unsigned long pnum;
+	unsigned long **map;
 	int size = usemap_size();
+	map = (unsigned long **) usemap_map;
 
 	usemap = sparse_early_usemaps_alloc_pgdat_section(NODE_DATA(nodeid),
 							  size * usemap_count);
@@ -358,9 +360,9 @@ static void __init sparse_early_usemaps_alloc_node(unsigned long**usemap_map,
 	for (pnum = pnum_begin; pnum < pnum_end; pnum++) {
 		if (!present_section_nr(pnum))
 			continue;
-		usemap_map[pnum] = usemap;
+		map[pnum] = usemap;
 		usemap += size;
-		check_usemap_section_nr(nodeid, usemap_map[pnum]);
+		check_usemap_section_nr(nodeid, map[pnum]);
 	}
 }
 
@@ -430,23 +432,16 @@ void __init sparse_mem_maps_populate_node(struct page **map_map,
 #endif /* !CONFIG_SPARSEMEM_VMEMMAP */
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
-static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
+static void __init sparse_early_mem_maps_alloc_node(void **map_map,
 				 unsigned long pnum_begin,
 				 unsigned long pnum_end,
 				 unsigned long map_count, int nodeid)
 {
-	sparse_mem_maps_populate_node(map_map, pnum_begin, pnum_end,
+	struct page **map = (struct page **)map_map;
+	sparse_mem_maps_populate_node(map, pnum_begin, pnum_end,
 					 map_count, nodeid);
 }
 #else
-
-static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
-				unsigned long pnum_begin,
-				unsigned long pnum_end,
-				unsigned long map_count, int nodeid)
-{
-}
-
 static struct page __init *sparse_early_mem_map_alloc(unsigned long pnum)
 {
 	struct page *map;
@@ -471,9 +466,10 @@ void __attribute__((weak)) __meminit vmemmap_populate_print_last(void)
 /**
  *  alloc_usemap_and_memmap - memory alloction for pageblock flags and vmemmap
  *  @map: usemap_map for pageblock flags or mmap_map for vmemmap
- *  @use_map: true if memory allocated for pageblock flags, otherwise false
  */
-static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
+static void alloc_usemap_and_memmap(void (*alloc_func)
+				(void **, unsigned long, unsigned long,
+				unsigned long, int), void **map)
 {
 	unsigned long pnum;
 	unsigned long map_count;
@@ -504,24 +500,16 @@ static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
 			continue;
 		}
 		/* ok, we need to take cake of from pnum_begin to pnum - 1*/
-		if (use_map)
-			sparse_early_usemaps_alloc_node(map, pnum_begin, pnum,
-						 map_count, nodeid_begin);
-		else
-			sparse_early_mem_maps_alloc_node((struct page **)map,
-				pnum_begin, pnum, map_count, nodeid_begin);
+		alloc_func(map, pnum_begin, pnum,
+					map_count, nodeid_begin);
 		/* new start, update count etc*/
 		nodeid_begin = nodeid;
 		pnum_begin = pnum;
 		map_count = 1;
 	}
 	/* ok, last chunk */
-	if (use_map)
-		sparse_early_usemaps_alloc_node(map, pnum_begin,
-				NR_MEM_SECTIONS, map_count, nodeid_begin);
-	else
-		sparse_early_mem_maps_alloc_node((struct page **)map,
-			pnum_begin, NR_MEM_SECTIONS, map_count, nodeid_begin);
+	alloc_func(map, pnum_begin, NR_MEM_SECTIONS,
+					map_count, nodeid_begin);
 }
 
 /*
@@ -561,14 +553,16 @@ void __init sparse_init(void)
 	usemap_map = alloc_bootmem(size);
 	if (!usemap_map)
 		panic("can not allocate usemap_map\n");
-	alloc_usemap_and_memmap(usemap_map, true);
+	alloc_usemap_and_memmap(sparse_early_usemaps_alloc_node,
+						(void **)usemap_map);
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
 	size2 = sizeof(struct page *) * NR_MEM_SECTIONS;
 	map_map = alloc_bootmem(size2);
 	if (!map_map)
 		panic("can not allocate map_map\n");
-	alloc_usemap_and_memmap((unsigned long **)map_map, false);
+	alloc_usemap_and_memmap(sparse_early_mem_maps_alloc_node,
+						(void **)map_map);
 #endif
 
 	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
-- 
1.7.7.6

Re: [PATCH v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 626 bytes --]

On Wed, Aug 28, 2013 at 7:34 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Aug 28, 2013 at 7:18 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> please change to function pointer to
>> void  (*alloc_func)(void *data,
>>                                  unsigned long pnum_begin,
>>                                  unsigned long pnum_end,
>>                                  unsigned long map_count, int nodeid)
>>
>> pnum_begin, pnum_end, map_coun, nodeid, should not be in the struct.
>
> looks like that is what is your first version did.
>
> I updated it a little bit. please check it.
>

removed more lines.

Yinghai

[-- Attachment #2: 0001-sparse_v3.patch --]
[-- Type: application/octet-stream, Size: 4399 bytes --]

From a78e12a9ff31f2a73b87145ce7ad943a0f712708 Mon Sep 17 00:00:00 2001
From: Wanpeng Li <liwanp@linux.vnet.ibm.com>
Date: Wed, 21 Aug 2013 15:23:08 +0800
Subject: [PATCH] mm/sparse: introduce alloc_usemap_and_memmap fix 

Pass function pointer to alloc_usemap_and_memmap() instead of true/false. 

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/sparse.c |   54 +++++++++++++++++++++++++-----------------------------
 1 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 55e5752..06adf3c 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -339,14 +339,15 @@ static void __init check_usemap_section_nr(int nid, unsigned long *usemap)
 }
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
-static void __init sparse_early_usemaps_alloc_node(unsigned long**usemap_map,
+static void __init sparse_early_usemaps_alloc_node(void *data,
 				 unsigned long pnum_begin,
 				 unsigned long pnum_end,
 				 unsigned long usemap_count, int nodeid)
 {
 	void *usemap;
 	unsigned long pnum;
+	unsigned long **usemap_map = (unsigned long **)data;
 	int size = usemap_size();
 
 	usemap = sparse_early_usemaps_alloc_pgdat_section(NODE_DATA(nodeid),
 							  size * usemap_count);
@@ -430,23 +432,15 @@ void __init sparse_mem_maps_populate_node(struct page **map_map,
 #endif /* !CONFIG_SPARSEMEM_VMEMMAP */
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
-static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
+static void __init sparse_early_mem_maps_alloc_node(void *data,
 				 unsigned long pnum_begin,
 				 unsigned long pnum_end,
 				 unsigned long map_count, int nodeid)
 {
+	struct page **map_map = (struct page **)data;
 	sparse_mem_maps_populate_node(map_map, pnum_begin, pnum_end,
 					 map_count, nodeid);
 }
 #else
-
-static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
-				unsigned long pnum_begin,
-				unsigned long pnum_end,
-				unsigned long map_count, int nodeid)
-{
-}
-
 static struct page __init *sparse_early_mem_map_alloc(unsigned long pnum)
 {
 	struct page *map;
@@ -471,9 +466,10 @@ void __attribute__((weak)) __meminit vmemmap_populate_print_last(void)
 /**
  *  alloc_usemap_and_memmap - memory alloction for pageblock flags and vmemmap
  *  @map: usemap_map for pageblock flags or mmap_map for vmemmap
- *  @use_map: true if memory allocated for pageblock flags, otherwise false
  */
-static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
+static void alloc_usemap_and_memmap(void (*alloc_func)
+				(void **, unsigned long, unsigned long,
+				unsigned long, int), void *data)
 {
 	unsigned long pnum;
 	unsigned long map_count;
@@ -504,24 +500,16 @@ static void alloc_usemap_and_memmap(unsigned long **map, bool use_map)
 			continue;
 		}
 		/* ok, we need to take cake of from pnum_begin to pnum - 1*/
-		if (use_map)
-			sparse_early_usemaps_alloc_node(map, pnum_begin, pnum,
-						 map_count, nodeid_begin);
-		else
-			sparse_early_mem_maps_alloc_node((struct page **)map,
-				pnum_begin, pnum, map_count, nodeid_begin);
+		alloc_func(data, pnum_begin, pnum,
+					map_count, nodeid_begin);
 		/* new start, update count etc*/
 		nodeid_begin = nodeid;
 		pnum_begin = pnum;
 		map_count = 1;
 	}
 	/* ok, last chunk */
-	if (use_map)
-		sparse_early_usemaps_alloc_node(map, pnum_begin,
-				NR_MEM_SECTIONS, map_count, nodeid_begin);
-	else
-		sparse_early_mem_maps_alloc_node((struct page **)map,
-			pnum_begin, NR_MEM_SECTIONS, map_count, nodeid_begin);
+	alloc_func(data, pnum_begin, NR_MEM_SECTIONS,
+					map_count, nodeid_begin);
 }
 
 /*
@@ -561,14 +553,16 @@ void __init sparse_init(void)
 	usemap_map = alloc_bootmem(size);
 	if (!usemap_map)
 		panic("can not allocate usemap_map\n");
-	alloc_usemap_and_memmap(usemap_map, true);
+	alloc_usemap_and_memmap(sparse_early_usemaps_alloc_node,
+						(void *)usemap_map);
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
 	size2 = sizeof(struct page *) * NR_MEM_SECTIONS;
 	map_map = alloc_bootmem(size2);
 	if (!map_map)
 		panic("can not allocate map_map\n");
-	alloc_usemap_and_memmap((unsigned long **)map_map, false);
+	alloc_usemap_and_memmap(sparse_early_mem_maps_alloc_node,
+						(void *)map_map);
 #endif
 
 	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
-- 
1.7.7.6

Re: [PATCH v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

Hi Yinghai,
On Wed, Aug 28, 2013 at 07:42:29PM -0700, Yinghai Lu wrote:
>On Wed, Aug 28, 2013 at 7:34 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Wed, Aug 28, 2013 at 7:18 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> please change to function pointer to
>>> void  (*alloc_func)(void *data,
>>>                                  unsigned long pnum_begin,
>>>                                  unsigned long pnum_end,
>>>                                  unsigned long map_count, int nodeid)
>>>
>>> pnum_begin, pnum_end, map_coun, nodeid, should not be in the struct.
>>
>> looks like that is what is your first version did.
>>
>> I updated it a little bit. please check it.
>>
>
>removed more lines.

Thanks for your great work!

The fixed patch looks good to me. If this is the last fix and I can
ignore http://marc.info/?l=linux-mm&m=137774271220239&w=2?

Regards,
Wanpeng Li 

>
>Yinghai


--
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 v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

Hi Yinghai,
On Wed, Aug 28, 2013 at 07:42:29PM -0700, Yinghai Lu wrote:
>On Wed, Aug 28, 2013 at 7:34 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Wed, Aug 28, 2013 at 7:18 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> please change to function pointer to
>>> void  (*alloc_func)(void *data,
>>>                                  unsigned long pnum_begin,
>>>                                  unsigned long pnum_end,
>>>                                  unsigned long map_count, int nodeid)
>>>
>>> pnum_begin, pnum_end, map_coun, nodeid, should not be in the struct.
>>
>> looks like that is what is your first version did.
>>
>> I updated it a little bit. please check it.
>>
>
>removed more lines.

Thanks for your great work!

The fixed patch looks good to me. If this is the last fix and I can
ignore http://marc.info/?l=linux-mm&m=137774271220239&w=2?

Regards,
Wanpeng Li 

>
>Yinghai


--
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 v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Yinghai Lu]

On Wed, Aug 28, 2013 at 7:51 PM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
> Hi Yinghai,
>>> looks like that is what is your first version did.
>>>
>>> I updated it a little bit. please check it.
>>>
>>
>>removed more lines.
>
> Thanks for your great work!
>
> The fixed patch looks good to me. If this is the last fix and I can
> ignore http://marc.info/?l=linux-mm&m=137774271220239&w=2?

Yes, you can ignore that.

Yinghai

--
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 v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

On Wed, Aug 28, 2013 at 09:10:25PM -0700, Yinghai Lu wrote:
>On Wed, Aug 28, 2013 at 7:51 PM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>> Hi Yinghai,
>>>> looks like that is what is your first version did.
>>>>
>>>> I updated it a little bit. please check it.
>>>>
>>>
>>>removed more lines.
>>
>> Thanks for your great work!
>>
>> The fixed patch looks good to me. If this is the last fix and I can
>> ignore http://marc.info/?l=linux-mm&m=137774271220239&w=2?
>
>Yes, you can ignore that.

Thanks, a little adjustment to fix compile warning. 

>
>Yinghai

Hi Andrew,

The patch in attachment is rebased on mm-sparse-introduce-alloc_usemap_and_memmap-fix.patch,
Could you pick this one?



[-- Attachment #2: 0001-sparse.patch --]
[-- Type: text/x-diff, Size: 0 bytes --]

Re: [PATCH v2 2/4] mm/sparse: introduce alloc_usemap_and_memmap

[Wanpeng Li]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

On Wed, Aug 28, 2013 at 09:10:25PM -0700, Yinghai Lu wrote:
>On Wed, Aug 28, 2013 at 7:51 PM, Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>> Hi Yinghai,
>>>> looks like that is what is your first version did.
>>>>
>>>> I updated it a little bit. please check it.
>>>>
>>>
>>>removed more lines.
>>
>> Thanks for your great work!
>>
>> The fixed patch looks good to me. If this is the last fix and I can
>> ignore http://marc.info/?l=linux-mm&m=137774271220239&w=2?
>
>Yes, you can ignore that.

Thanks, a little adjustment to fix compile warning. 

>
>Yinghai

Hi Andrew,

The patch in attachment is rebased on mm-sparse-introduce-alloc_usemap_and_memmap-fix.patch,
Could you pick this one?



[-- Attachment #2: 0001-sparse.patch --]
[-- Type: text/x-diff, Size: 4261 bytes --]

>From b69866fb1baa9963daf91e66fb9826d6f62879fe Mon Sep 17 00:00:00 2001
From: Wanpeng Li <liwanp@linux.vnet.ibm.com>
Date: Thu, 29 Aug 2013 12:34:17 +0800
Subject: [PATCH] mm/sparse: introduce alloc_usemap_and_memmap fix-2

Pass function pointer to alloc_usemap_and_memmap() instead of true/false.

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/sparse.c | 41 +++++++++++++++--------------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 3bb221a..6734d56 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -339,13 +339,14 @@ static void __init check_usemap_section_nr(int nid, unsigned long *usemap)
 }
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
-static void __init sparse_early_usemaps_alloc_node(unsigned long**usemap_map,
+static void __init sparse_early_usemaps_alloc_node(void **data,
 				 unsigned long pnum_begin,
 				 unsigned long pnum_end,
 				 unsigned long usemap_count, int nodeid)
 {
 	void *usemap;
 	unsigned long pnum;
+	unsigned long **usemap_map = (unsigned long **)data;
 	int size = usemap_size();
 
 	usemap = sparse_early_usemaps_alloc_pgdat_section(NODE_DATA(nodeid),
@@ -430,23 +431,16 @@ void __init sparse_mem_maps_populate_node(struct page **map_map,
 #endif /* !CONFIG_SPARSEMEM_VMEMMAP */
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
-static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
+static void __init sparse_early_mem_maps_alloc_node(void **data,
 				 unsigned long pnum_begin,
 				 unsigned long pnum_end,
 				 unsigned long map_count, int nodeid)
 {
+	struct page **map_map = (struct page **)data;
 	sparse_mem_maps_populate_node(map_map, pnum_begin, pnum_end,
 					 map_count, nodeid);
 }
 #else
-
-static void __init sparse_early_mem_maps_alloc_node(struct page **map_map,
-				unsigned long pnum_begin,
-				unsigned long pnum_end,
-				unsigned long map_count, int nodeid)
-{
-}
-
 static struct page __init *sparse_early_mem_map_alloc(unsigned long pnum)
 {
 	struct page *map;
@@ -471,9 +465,10 @@ void __attribute__((weak)) __meminit vmemmap_populate_print_last(void)
 /**
  *  alloc_usemap_and_memmap - memory alloction for pageblock flags and vmemmap
  *  @map: usemap_map for pageblock flags or mmap_map for vmemmap
- *  @use_map: true if memory allocated for pageblock flags, otherwise false
  */
-static void __init alloc_usemap_and_memmap(unsigned long **map, bool use_map)
+static void __init alloc_usemap_and_memmap(void (*alloc_func)
+					(void **, unsigned long, unsigned long,
+					unsigned long, int), void **data)
 {
 	unsigned long pnum;
 	unsigned long map_count;
@@ -504,24 +499,16 @@ static void __init alloc_usemap_and_memmap(unsigned long **map, bool use_map)
 			continue;
 		}
 		/* ok, we need to take cake of from pnum_begin to pnum - 1*/
-		if (use_map)
-			sparse_early_usemaps_alloc_node(map, pnum_begin, pnum,
-						 map_count, nodeid_begin);
-		else
-			sparse_early_mem_maps_alloc_node((struct page **)map,
-				pnum_begin, pnum, map_count, nodeid_begin);
+		alloc_func(data, pnum_begin, pnum,
+						map_count, nodeid_begin);
 		/* new start, update count etc*/
 		nodeid_begin = nodeid;
 		pnum_begin = pnum;
 		map_count = 1;
 	}
 	/* ok, last chunk */
-	if (use_map)
-		sparse_early_usemaps_alloc_node(map, pnum_begin,
-				NR_MEM_SECTIONS, map_count, nodeid_begin);
-	else
-		sparse_early_mem_maps_alloc_node((struct page **)map,
-			pnum_begin, NR_MEM_SECTIONS, map_count, nodeid_begin);
+	alloc_func(data, pnum_begin, NR_MEM_SECTIONS,
+						map_count, nodeid_begin);
 }
 
 /*
@@ -561,14 +548,16 @@ void __init sparse_init(void)
 	usemap_map = alloc_bootmem(size);
 	if (!usemap_map)
 		panic("can not allocate usemap_map\n");
-	alloc_usemap_and_memmap(usemap_map, true);
+	alloc_usemap_and_memmap(sparse_early_usemaps_alloc_node,
+							(void **)usemap_map);
 
 #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER
 	size2 = sizeof(struct page *) * NR_MEM_SECTIONS;
 	map_map = alloc_bootmem(size2);
 	if (!map_map)
 		panic("can not allocate map_map\n");
-	alloc_usemap_and_memmap((unsigned long **)map_map, false);
+	alloc_usemap_and_memmap(sparse_early_mem_maps_alloc_node,
+							(void **)map_map);
 #endif
 
 	for (pnum = 0; pnum < NR_MEM_SECTIONS; pnum++) {
-- 
1.8.1.2

[PATCH v2 3/4] mm/writeback: make writeback_inodes_wb static

[Wanpeng Li]

It's not used globally and could be static.

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 fs/fs-writeback.c         | 2 +-
 include/linux/writeback.h | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 87d7781..54b3c31 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -723,7 +723,7 @@ static long __writeback_inodes_wb(struct bdi_writeback *wb,
 	return wrote;
 }
 
-long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages,
+static long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages,
 				enum wb_reason reason)
 {
 	struct wb_writeback_work work = {
diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index 4e198ca..021b8a3 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -98,8 +98,6 @@ int try_to_writeback_inodes_sb(struct super_block *, enum wb_reason reason);
 int try_to_writeback_inodes_sb_nr(struct super_block *, unsigned long nr,
 				  enum wb_reason reason);
 void sync_inodes_sb(struct super_block *);
-long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages,
-				enum wb_reason reason);
 void wakeup_flusher_threads(long nr_pages, enum wb_reason reason);
 void inode_wait_for_writeback(struct inode *inode);
 
-- 
1.8.1.2

--
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 v2 3/4] mm/writeback: make writeback_inodes_wb static

[Seth Jennings]

On Tue, Aug 20, 2013 at 02:54:55PM +0800, Wanpeng Li wrote:
> It's not used globally and could be static.
> 

Reviewed-by: Seth Jennings <sjenning@linux.vnet.ibm.com>

> Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>

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

[PATCH v2 4/4] mm/vmalloc: use wrapper function get_vm_area_size to caculate size of vm area

[Wanpeng Li]

v1 -> v2:
 * replace all the spots by function get_vm_area_size().

Use wrapper function get_vm_area_size to calculate size of vm area.

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/vmalloc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 93d3182..1074543 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1258,7 +1258,7 @@ void unmap_kernel_range(unsigned long addr, unsigned long size)
 int map_vm_area(struct vm_struct *area, pgprot_t prot, struct page ***pages)
 {
 	unsigned long addr = (unsigned long)area->addr;
-	unsigned long end = addr + area->size - PAGE_SIZE;
+	unsigned long end = addr + get_vm_area_size(area);
 	int err;
 
 	err = vmap_page_range(addr, end, prot, *pages);
@@ -1553,7 +1553,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 	unsigned int nr_pages, array_size, i;
 	gfp_t nested_gfp = (gfp_mask & GFP_RECLAIM_MASK) | __GFP_ZERO;
 
-	nr_pages = (area->size - PAGE_SIZE) >> PAGE_SHIFT;
+	nr_pages = get_vm_area_size(area) >> PAGE_SHIFT;
 	array_size = (nr_pages * sizeof(struct page *));
 
 	area->nr_pages = nr_pages;
@@ -1985,7 +1985,7 @@ long vread(char *buf, char *addr, unsigned long count)
 
 		vm = va->vm;
 		vaddr = (char *) vm->addr;
-		if (addr >= vaddr + vm->size - PAGE_SIZE)
+		if (addr >= vaddr + get_vm_area_size(vm))
 			continue;
 		while (addr < vaddr) {
 			if (count == 0)
@@ -1995,7 +1995,7 @@ long vread(char *buf, char *addr, unsigned long count)
 			addr++;
 			count--;
 		}
-		n = vaddr + vm->size - PAGE_SIZE - addr;
+		n = vaddr + get_vm_area_size(vm) - addr;
 		if (n > count)
 			n = count;
 		if (!(vm->flags & VM_IOREMAP))
@@ -2067,7 +2067,7 @@ long vwrite(char *buf, char *addr, unsigned long count)
 
 		vm = va->vm;
 		vaddr = (char *) vm->addr;
-		if (addr >= vaddr + vm->size - PAGE_SIZE)
+		if (addr >= vaddr + get_vm_area_size(vm))
 			continue;
 		while (addr < vaddr) {
 			if (count == 0)
@@ -2076,7 +2076,7 @@ long vwrite(char *buf, char *addr, unsigned long count)
 			addr++;
 			count--;
 		}
-		n = vaddr + vm->size - PAGE_SIZE - addr;
+		n = vaddr + get_vm_area_size(vm) - addr;
 		if (n > count)
 			n = count;
 		if (!(vm->flags & VM_IOREMAP)) {
-- 
1.8.1.2

--
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 v2 4/4] mm/vmalloc: use wrapper function get_vm_area_size to caculate size of vm area

[Seth Jennings]

On Tue, Aug 20, 2013 at 02:54:56PM +0800, Wanpeng Li wrote:
> v1 -> v2:
>  * replace all the spots by function get_vm_area_size().
> 
> Use wrapper function get_vm_area_size to calculate size of vm area.
> 

Reviewed-by: Seth Jennings <sjenning@linux.vnet.ibm.com>

> Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>

--
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 v2 1/4] mm/pgtable: Fix continue to preallocate pmds even if failure occurrence

[Seth Jennings]

On Tue, Aug 20, 2013 at 02:54:53PM +0800, Wanpeng Li wrote:
> v1 -> v2:
>  * remove failed.
> 
> preallocate_pmds will continue to preallocate pmds even if failure
> occurrence, and then free all the preallocate pmds if there is
> failure, this patch fix it by stop preallocate if failure occurrence
> and go to free path.
> 

Reviewed-by: Seth Jennings <sjenning@linux.vnet.ibm.com>

> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>

--
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 v2 1/4] mm/pgtable: Fix continue to preallocate pmds even if failure occurrence

[Andrew Morton]

On Tue, 20 Aug 2013 14:54:53 +0800 Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:

> preallocate_pmds will continue to preallocate pmds even if failure
> occurrence, and then free all the preallocate pmds if there is
> failure, this patch fix it by stop preallocate if failure occurrence
> and go to free path.
>
> ...
>
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -196,21 +196,18 @@ static void free_pmds(pmd_t *pmds[])
>  static int preallocate_pmds(pmd_t *pmds[])
>  {
>  	int i;
> -	bool failed = false;
>  
>  	for(i = 0; i < PREALLOCATED_PMDS; i++) {
>  		pmd_t *pmd = (pmd_t *)__get_free_page(PGALLOC_GFP);
>  		if (pmd == NULL)
> -			failed = true;
> +			goto err;
>  		pmds[i] = pmd;
>  	}
>  
> -	if (failed) {
> -		free_pmds(pmds);
> -		return -ENOMEM;
> -	}
> -
>  	return 0;
> +err:
> +	free_pmds(pmds);
> +	return -ENOMEM;
>  }

Nope.  If the error path is taken, free_pmds() will free uninitialised
items from pmds[], which is a local in pgd_alloc() and contains random
stack junk.  The kernel will crash.

You could pass an nr_pmds argument to free_pmds(), or zero out the
remaining items on the error path.  However, although the current code
is a bit kooky, I don't see that it is harmful in any way.

> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>

Ahem.

--
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 v2 1/4] mm/pgtable: Fix continue to preallocate pmds even if failure occurrence

[Wanpeng Li]

On Tue, Aug 20, 2013 at 04:04:18PM -0700, Andrew Morton wrote:
>On Tue, 20 Aug 2013 14:54:53 +0800 Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>
>> preallocate_pmds will continue to preallocate pmds even if failure
>> occurrence, and then free all the preallocate pmds if there is
>> failure, this patch fix it by stop preallocate if failure occurrence
>> and go to free path.
>>
>> ...
>>
>> --- a/arch/x86/mm/pgtable.c
>> +++ b/arch/x86/mm/pgtable.c
>> @@ -196,21 +196,18 @@ static void free_pmds(pmd_t *pmds[])
>>  static int preallocate_pmds(pmd_t *pmds[])
>>  {
>>  	int i;
>> -	bool failed = false;
>>  
>>  	for(i = 0; i < PREALLOCATED_PMDS; i++) {
>>  		pmd_t *pmd = (pmd_t *)__get_free_page(PGALLOC_GFP);
>>  		if (pmd == NULL)
>> -			failed = true;
>> +			goto err;
>>  		pmds[i] = pmd;
>>  	}
>>  
>> -	if (failed) {
>> -		free_pmds(pmds);
>> -		return -ENOMEM;
>> -	}
>> -
>>  	return 0;
>> +err:
>> +	free_pmds(pmds);
>> +	return -ENOMEM;
>>  }
>

Hi Andrew,

>Nope.  If the error path is taken, free_pmds() will free uninitialised
>items from pmds[], which is a local in pgd_alloc() and contains random
>stack junk.  The kernel will crash.
>
>You could pass an nr_pmds argument to free_pmds(), or zero out the
>remaining items on the error path.  However, although the current code
>is a bit kooky, I don't see that it is harmful in any way.
>

There is a check in free_pmds():

if (pmds[i])
	free_page((unsigned long)pmds[i]);

which will avoid the issue you mentioned.

In addition, the codes in pgd_alloc will skip free pmds if preallocate pmds 
failure which will avoid free pmds twice. Am I miss something? ;-)

Regards,
Wanpeng Li 

>> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
>
>Ahem.

--
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 v2 1/4] mm/pgtable: Fix continue to preallocate pmds even if failure occurrence

[Wanpeng Li]

On Tue, Aug 20, 2013 at 04:04:18PM -0700, Andrew Morton wrote:
>On Tue, 20 Aug 2013 14:54:53 +0800 Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:
>
>> preallocate_pmds will continue to preallocate pmds even if failure
>> occurrence, and then free all the preallocate pmds if there is
>> failure, this patch fix it by stop preallocate if failure occurrence
>> and go to free path.
>>
>> ...
>>
>> --- a/arch/x86/mm/pgtable.c
>> +++ b/arch/x86/mm/pgtable.c
>> @@ -196,21 +196,18 @@ static void free_pmds(pmd_t *pmds[])
>>  static int preallocate_pmds(pmd_t *pmds[])
>>  {
>>  	int i;
>> -	bool failed = false;
>>  
>>  	for(i = 0; i < PREALLOCATED_PMDS; i++) {
>>  		pmd_t *pmd = (pmd_t *)__get_free_page(PGALLOC_GFP);
>>  		if (pmd == NULL)
>> -			failed = true;
>> +			goto err;
>>  		pmds[i] = pmd;
>>  	}
>>  
>> -	if (failed) {
>> -		free_pmds(pmds);
>> -		return -ENOMEM;
>> -	}
>> -
>>  	return 0;
>> +err:
>> +	free_pmds(pmds);
>> +	return -ENOMEM;
>>  }
>

Hi Andrew,

>Nope.  If the error path is taken, free_pmds() will free uninitialised
>items from pmds[], which is a local in pgd_alloc() and contains random
>stack junk.  The kernel will crash.
>
>You could pass an nr_pmds argument to free_pmds(), or zero out the
>remaining items on the error path.  However, although the current code
>is a bit kooky, I don't see that it is harmful in any way.
>

There is a check in free_pmds():

if (pmds[i])
	free_page((unsigned long)pmds[i]);

which will avoid the issue you mentioned.

In addition, the codes in pgd_alloc will skip free pmds if preallocate pmds 
failure which will avoid free pmds twice. Am I miss something? ;-)

Regards,
Wanpeng Li 

>> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
>
>Ahem.

--
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 v2 1/4] mm/pgtable: Fix continue to preallocate pmds even if failure occurrence

[Andrew Morton]

On Wed, 21 Aug 2013 07:39:35 +0800 Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:

> >Nope.  If the error path is taken, free_pmds() will free uninitialised
> >items from pmds[], which is a local in pgd_alloc() and contains random
> >stack junk.  The kernel will crash.
> >
> >You could pass an nr_pmds argument to free_pmds(), or zero out the
> >remaining items on the error path.  However, although the current code
> >is a bit kooky, I don't see that it is harmful in any way.
> >
> 
> There is a check in free_pmds():
> 
> if (pmds[i])
> 	free_page((unsigned long)pmds[i]);
> 
> which will avoid the issue you mentioned.

pmds[i] is uninitialized.  It gets allocated
on the stack in pgd_alloc() and does not get zeroed.

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