[PATCH 1/4] page cgroup: using vzalloc instead of vmalloc

[PATCH 1/4] page cgroup: using vzalloc instead of vmalloc

[Bob Liu]

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/page_cgroup.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/mm/page_cgroup.c b/mm/page_cgroup.c
index 39d216d..6bdc67d 100644
--- a/mm/page_cgroup.c
+++ b/mm/page_cgroup.c
@@ -513,11 +513,10 @@ int swap_cgroup_swapon(int type, unsigned long max_pages)
 	length = DIV_ROUND_UP(max_pages, SC_PER_PAGE);
 	array_size = length * sizeof(void *);
 
-	array = vmalloc(array_size);
+	array = vzalloc(array_size);
 	if (!array)
 		goto nomem;
 
-	memset(array, 0, array_size);
 	ctrl = &swap_cgroup_ctrl[type];
 	mutex_lock(&swap_cgroup_mutex);
 	ctrl->length = length;
-- 
1.6.3.3


--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/4] frontswap: using vzalloc instead of vmalloc

[Bob Liu]

This patch also add checking whether alloc frontswap_map memory
failed.

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/swapfile.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index ffdd06a..8fe9e88 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -2124,9 +2124,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 	}
 	/* frontswap enabled? set up bit-per-page map for frontswap */
 	if (frontswap_enabled) {
-		frontswap_map = vmalloc(maxpages / sizeof(long));
-		if (frontswap_map)
-			memset(frontswap_map, 0, maxpages / sizeof(long));
+		frontswap_map = vzalloc(maxpages / sizeof(long));
+		if (!frontswap_map)
+			goto bad_swap;
 	}
 
 	if (p->bdev) {
-- 
1.6.3.3


--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/4] sparse: using kzalloc to clean up code

[Bob Liu]

This patch using kzalloc to clean up sparse_index_alloc() and
__GFP_ZERO to clean up __kmalloc_section_memmap().

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/sparse.c |   24 +++++++-----------------
 1 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 858e1df..9596635 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -65,15 +65,12 @@ static struct mem_section noinline __init_refok *sparse_index_alloc(int nid)
 
 	if (slab_is_available()) {
 		if (node_state(nid, N_HIGH_MEMORY))
-			section = kmalloc_node(array_size, GFP_KERNEL, nid);
+			section = kzalloc_node(array_size, GFP_KERNEL, nid);
 		else
-			section = kmalloc(array_size, GFP_KERNEL);
+			section = kzalloc(array_size, GFP_KERNEL);
 	} else
 		section = alloc_bootmem_node(NODE_DATA(nid), array_size);
 
-	if (section)
-		memset(section, 0, array_size);
-
 	return section;
 }
 
@@ -636,19 +633,12 @@ static struct page *__kmalloc_section_memmap(unsigned long nr_pages)
 	struct page *page, *ret;
 	unsigned long memmap_size = sizeof(struct page) * nr_pages;
 
-	page = alloc_pages(GFP_KERNEL|__GFP_NOWARN, get_order(memmap_size));
+	page = alloc_pages(GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO,
+					get_order(memmap_size));
 	if (page)
-		goto got_map_page;
-
-	ret = vmalloc(memmap_size);
-	if (ret)
-		goto got_map_ptr;
-
-	return NULL;
-got_map_page:
-	ret = (struct page *)pfn_to_kaddr(page_to_pfn(page));
-got_map_ptr:
-	memset(ret, 0, memmap_size);
+		ret = (struct page *)pfn_to_kaddr(page_to_pfn(page));
+	else
+		ret = vzalloc(memmap_size);
 
 	return ret;
 }
-- 
1.6.3.3


--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4/4] percpu: rename pcpu_mem_alloc to pcpu_mem_zalloc

[Bob Liu]

Currently pcpu_mem_alloc() is implemented always return zeroed memory.
So rename it to make user like pcpu_get_pages_and_bitmap() know don't reinit it.

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/percpu-vm.c |    5 ++---
 mm/percpu.c    |   17 +++++++++--------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/mm/percpu-vm.c b/mm/percpu-vm.c
index ea53496..29e3730 100644
--- a/mm/percpu-vm.c
+++ b/mm/percpu-vm.c
@@ -50,14 +50,13 @@ static struct page **pcpu_get_pages_and_bitmap(struct pcpu_chunk *chunk,
 
 	if (!pages || !bitmap) {
 		if (may_alloc && !pages)
-			pages = pcpu_mem_alloc(pages_size);
+			pages = pcpu_mem_zalloc(pages_size);
 		if (may_alloc && !bitmap)
-			bitmap = pcpu_mem_alloc(bitmap_size);
+			bitmap = pcpu_mem_zalloc(bitmap_size);
 		if (!pages || !bitmap)
 			return NULL;
 	}
 
-	memset(pages, 0, pages_size);
 	bitmap_copy(bitmap, chunk->populated, pcpu_unit_pages);
 
 	*bitmapp = bitmap;
diff --git a/mm/percpu.c b/mm/percpu.c
index bf80e55..28c37a2 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -273,11 +273,11 @@ static void __maybe_unused pcpu_next_pop(struct pcpu_chunk *chunk,
 	     (rs) = (re) + 1, pcpu_next_pop((chunk), &(rs), &(re), (end)))
 
 /**
- * pcpu_mem_alloc - allocate memory
+ * pcpu_mem_zalloc - allocate memory
  * @size: bytes to allocate
  *
  * Allocate @size bytes.  If @size is smaller than PAGE_SIZE,
- * kzalloc() is used; otherwise, vmalloc() is used.  The returned
+ * kzalloc() is used; otherwise, vzalloc() is used.  The returned
  * memory is always zeroed.
  *
  * CONTEXT:
@@ -286,7 +286,7 @@ static void __maybe_unused pcpu_next_pop(struct pcpu_chunk *chunk,
  * RETURNS:
  * Pointer to the allocated area on success, NULL on failure.
  */
-static void *pcpu_mem_alloc(size_t size)
+static void *pcpu_mem_zalloc(size_t size)
 {
 	if (WARN_ON_ONCE(!slab_is_available()))
 		return NULL;
@@ -302,7 +302,7 @@ static void *pcpu_mem_alloc(size_t size)
  * @ptr: memory to free
  * @size: size of the area
  *
- * Free @ptr.  @ptr should have been allocated using pcpu_mem_alloc().
+ * Free @ptr.  @ptr should have been allocated using pcpu_mem_zalloc().
  */
 static void pcpu_mem_free(void *ptr, size_t size)
 {
@@ -384,7 +384,7 @@ static int pcpu_extend_area_map(struct pcpu_chunk *chunk, int new_alloc)
 	size_t old_size = 0, new_size = new_alloc * sizeof(new[0]);
 	unsigned long flags;
 
-	new = pcpu_mem_alloc(new_size);
+	new = pcpu_mem_zalloc(new_size);
 	if (!new)
 		return -ENOMEM;
 
@@ -604,11 +604,12 @@ static struct pcpu_chunk *pcpu_alloc_chunk(void)
 {
 	struct pcpu_chunk *chunk;
 
-	chunk = pcpu_mem_alloc(pcpu_chunk_struct_size);
+	chunk = pcpu_mem_zalloc(pcpu_chunk_struct_size);
 	if (!chunk)
 		return NULL;
 
-	chunk->map = pcpu_mem_alloc(PCPU_DFL_MAP_ALLOC * sizeof(chunk->map[0]));
+	chunk->map = pcpu_mem_zalloc(PCPU_DFL_MAP_ALLOC *
+						sizeof(chunk->map[0]));
 	if (!chunk->map) {
 		kfree(chunk);
 		return NULL;
@@ -1889,7 +1890,7 @@ void __init percpu_init_late(void)
 
 		BUILD_BUG_ON(size > PAGE_SIZE);
 
-		map = pcpu_mem_alloc(size);
+		map = pcpu_mem_zalloc(size);
 		BUG_ON(!map);
 
 		spin_lock_irqsave(&pcpu_lock, flags);
-- 
1.6.3.3


--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] percpu: rename pcpu_mem_alloc to pcpu_mem_zalloc

[Pekka Enberg]

On Thu, Aug 4, 2011 at 6:09 AM, Bob Liu <lliubbo@gmail.com> wrote:
> Currently pcpu_mem_alloc() is implemented always return zeroed memory.
> So rename it to make user like pcpu_get_pages_and_bitmap() know don't reinit it.
>
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

Reviewed-by: Pekka Enberg <penberg@kernel.org>

> ---
>  mm/percpu-vm.c |    5 ++---
>  mm/percpu.c    |   17 +++++++++--------
>  2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/mm/percpu-vm.c b/mm/percpu-vm.c
> index ea53496..29e3730 100644
> --- a/mm/percpu-vm.c
> +++ b/mm/percpu-vm.c
> @@ -50,14 +50,13 @@ static struct page **pcpu_get_pages_and_bitmap(struct pcpu_chunk *chunk,
>
>        if (!pages || !bitmap) {
>                if (may_alloc && !pages)
> -                       pages = pcpu_mem_alloc(pages_size);
> +                       pages = pcpu_mem_zalloc(pages_size);
>                if (may_alloc && !bitmap)
> -                       bitmap = pcpu_mem_alloc(bitmap_size);
> +                       bitmap = pcpu_mem_zalloc(bitmap_size);
>                if (!pages || !bitmap)
>                        return NULL;
>        }
>
> -       memset(pages, 0, pages_size);
>        bitmap_copy(bitmap, chunk->populated, pcpu_unit_pages);
>
>        *bitmapp = bitmap;
> diff --git a/mm/percpu.c b/mm/percpu.c
> index bf80e55..28c37a2 100644
> --- a/mm/percpu.c
> +++ b/mm/percpu.c
> @@ -273,11 +273,11 @@ static void __maybe_unused pcpu_next_pop(struct pcpu_chunk *chunk,
>             (rs) = (re) + 1, pcpu_next_pop((chunk), &(rs), &(re), (end)))
>
>  /**
> - * pcpu_mem_alloc - allocate memory
> + * pcpu_mem_zalloc - allocate memory
>  * @size: bytes to allocate
>  *
>  * Allocate @size bytes.  If @size is smaller than PAGE_SIZE,
> - * kzalloc() is used; otherwise, vmalloc() is used.  The returned
> + * kzalloc() is used; otherwise, vzalloc() is used.  The returned
>  * memory is always zeroed.
>  *
>  * CONTEXT:
> @@ -286,7 +286,7 @@ static void __maybe_unused pcpu_next_pop(struct pcpu_chunk *chunk,
>  * RETURNS:
>  * Pointer to the allocated area on success, NULL on failure.
>  */
> -static void *pcpu_mem_alloc(size_t size)
> +static void *pcpu_mem_zalloc(size_t size)
>  {
>        if (WARN_ON_ONCE(!slab_is_available()))
>                return NULL;
> @@ -302,7 +302,7 @@ static void *pcpu_mem_alloc(size_t size)
>  * @ptr: memory to free
>  * @size: size of the area
>  *
> - * Free @ptr.  @ptr should have been allocated using pcpu_mem_alloc().
> + * Free @ptr.  @ptr should have been allocated using pcpu_mem_zalloc().
>  */
>  static void pcpu_mem_free(void *ptr, size_t size)
>  {
> @@ -384,7 +384,7 @@ static int pcpu_extend_area_map(struct pcpu_chunk *chunk, int new_alloc)
>        size_t old_size = 0, new_size = new_alloc * sizeof(new[0]);
>        unsigned long flags;
>
> -       new = pcpu_mem_alloc(new_size);
> +       new = pcpu_mem_zalloc(new_size);
>        if (!new)
>                return -ENOMEM;
>
> @@ -604,11 +604,12 @@ static struct pcpu_chunk *pcpu_alloc_chunk(void)
>  {
>        struct pcpu_chunk *chunk;
>
> -       chunk = pcpu_mem_alloc(pcpu_chunk_struct_size);
> +       chunk = pcpu_mem_zalloc(pcpu_chunk_struct_size);
>        if (!chunk)
>                return NULL;
>
> -       chunk->map = pcpu_mem_alloc(PCPU_DFL_MAP_ALLOC * sizeof(chunk->map[0]));
> +       chunk->map = pcpu_mem_zalloc(PCPU_DFL_MAP_ALLOC *
> +                                               sizeof(chunk->map[0]));
>        if (!chunk->map) {
>                kfree(chunk);
>                return NULL;
> @@ -1889,7 +1890,7 @@ void __init percpu_init_late(void)
>
>                BUILD_BUG_ON(size > PAGE_SIZE);
>
> -               map = pcpu_mem_alloc(size);
> +               map = pcpu_mem_zalloc(size);
>                BUG_ON(!map);
>
>                spin_lock_irqsave(&pcpu_lock, flags);
> --
> 1.6.3.3
>
>
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> 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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] percpu: rename pcpu_mem_alloc to pcpu_mem_zalloc

[Michal Hocko]

On Thu 04-08-11 11:09:50, Bob Liu wrote:
> Currently pcpu_mem_alloc() is implemented always return zeroed memory.
> So rename it to make user like pcpu_get_pages_and_bitmap() know don't reinit it.

Makes sense. It fits better with internal kzalloc and vzalloc usage.
 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

Reviewed-by: Michal Hocko <mhocko@suse.cz>

> ---
>  mm/percpu-vm.c |    5 ++---
>  mm/percpu.c    |   17 +++++++++--------
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/percpu-vm.c b/mm/percpu-vm.c
> index ea53496..29e3730 100644
> --- a/mm/percpu-vm.c
> +++ b/mm/percpu-vm.c
> @@ -50,14 +50,13 @@ static struct page **pcpu_get_pages_and_bitmap(struct pcpu_chunk *chunk,
>  
>  	if (!pages || !bitmap) {
>  		if (may_alloc && !pages)
> -			pages = pcpu_mem_alloc(pages_size);
> +			pages = pcpu_mem_zalloc(pages_size);
>  		if (may_alloc && !bitmap)
> -			bitmap = pcpu_mem_alloc(bitmap_size);
> +			bitmap = pcpu_mem_zalloc(bitmap_size);
>  		if (!pages || !bitmap)
>  			return NULL;
>  	}
>  
> -	memset(pages, 0, pages_size);
>  	bitmap_copy(bitmap, chunk->populated, pcpu_unit_pages);
>  
>  	*bitmapp = bitmap;
> diff --git a/mm/percpu.c b/mm/percpu.c
> index bf80e55..28c37a2 100644
> --- a/mm/percpu.c
> +++ b/mm/percpu.c
> @@ -273,11 +273,11 @@ static void __maybe_unused pcpu_next_pop(struct pcpu_chunk *chunk,
>  	     (rs) = (re) + 1, pcpu_next_pop((chunk), &(rs), &(re), (end)))
>  
>  /**
> - * pcpu_mem_alloc - allocate memory
> + * pcpu_mem_zalloc - allocate memory
>   * @size: bytes to allocate
>   *
>   * Allocate @size bytes.  If @size is smaller than PAGE_SIZE,
> - * kzalloc() is used; otherwise, vmalloc() is used.  The returned
> + * kzalloc() is used; otherwise, vzalloc() is used.  The returned
>   * memory is always zeroed.
>   *
>   * CONTEXT:
> @@ -286,7 +286,7 @@ static void __maybe_unused pcpu_next_pop(struct pcpu_chunk *chunk,
>   * RETURNS:
>   * Pointer to the allocated area on success, NULL on failure.
>   */
> -static void *pcpu_mem_alloc(size_t size)
> +static void *pcpu_mem_zalloc(size_t size)
>  {
>  	if (WARN_ON_ONCE(!slab_is_available()))
>  		return NULL;
> @@ -302,7 +302,7 @@ static void *pcpu_mem_alloc(size_t size)
>   * @ptr: memory to free
>   * @size: size of the area
>   *
> - * Free @ptr.  @ptr should have been allocated using pcpu_mem_alloc().
> + * Free @ptr.  @ptr should have been allocated using pcpu_mem_zalloc().
>   */
>  static void pcpu_mem_free(void *ptr, size_t size)
>  {
> @@ -384,7 +384,7 @@ static int pcpu_extend_area_map(struct pcpu_chunk *chunk, int new_alloc)
>  	size_t old_size = 0, new_size = new_alloc * sizeof(new[0]);
>  	unsigned long flags;
>  
> -	new = pcpu_mem_alloc(new_size);
> +	new = pcpu_mem_zalloc(new_size);
>  	if (!new)
>  		return -ENOMEM;
>  
> @@ -604,11 +604,12 @@ static struct pcpu_chunk *pcpu_alloc_chunk(void)
>  {
>  	struct pcpu_chunk *chunk;
>  
> -	chunk = pcpu_mem_alloc(pcpu_chunk_struct_size);
> +	chunk = pcpu_mem_zalloc(pcpu_chunk_struct_size);
>  	if (!chunk)
>  		return NULL;
>  
> -	chunk->map = pcpu_mem_alloc(PCPU_DFL_MAP_ALLOC * sizeof(chunk->map[0]));
> +	chunk->map = pcpu_mem_zalloc(PCPU_DFL_MAP_ALLOC *
> +						sizeof(chunk->map[0]));
>  	if (!chunk->map) {
>  		kfree(chunk);
>  		return NULL;
> @@ -1889,7 +1890,7 @@ void __init percpu_init_late(void)
>  
>  		BUILD_BUG_ON(size > PAGE_SIZE);
>  
> -		map = pcpu_mem_alloc(size);
> +		map = pcpu_mem_zalloc(size);
>  		BUG_ON(!map);
>  
>  		spin_lock_irqsave(&pcpu_lock, flags);
> -- 
> 1.6.3.3
> 
> 
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] percpu: rename pcpu_mem_alloc to pcpu_mem_zalloc

[Tejun Heo]

On Thu, Aug 04, 2011 at 11:09:50AM +0800, Bob Liu wrote:
> Currently pcpu_mem_alloc() is implemented always return zeroed memory.
> So rename it to make user like pcpu_get_pages_and_bitmap() know don't reinit it.
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

applied to percpu#for-3.2, thanks.

-- 
tejun

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] sparse: using kzalloc to clean up code

[Pekka Enberg]

On Thu, Aug 4, 2011 at 6:09 AM, Bob Liu <lliubbo@gmail.com> wrote:
> This patch using kzalloc to clean up sparse_index_alloc() and
> __GFP_ZERO to clean up __kmalloc_section_memmap().
>
> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> ---
>  mm/sparse.c |   24 +++++++-----------------
>  1 files changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 858e1df..9596635 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -65,15 +65,12 @@ static struct mem_section noinline __init_refok *sparse_index_alloc(int nid)
>
>        if (slab_is_available()) {
>                if (node_state(nid, N_HIGH_MEMORY))
> -                       section = kmalloc_node(array_size, GFP_KERNEL, nid);
> +                       section = kzalloc_node(array_size, GFP_KERNEL, nid);
>                else
> -                       section = kmalloc(array_size, GFP_KERNEL);
> +                       section = kzalloc(array_size, GFP_KERNEL);
>        } else
>                section = alloc_bootmem_node(NODE_DATA(nid), array_size);
>
> -       if (section)
> -               memset(section, 0, array_size);
> -

You now broke the alloc_bootmem_node() path.

>        return section;
>  }
>
> @@ -636,19 +633,12 @@ static struct page *__kmalloc_section_memmap(unsigned long nr_pages)
>        struct page *page, *ret;
>        unsigned long memmap_size = sizeof(struct page) * nr_pages;
>
> -       page = alloc_pages(GFP_KERNEL|__GFP_NOWARN, get_order(memmap_size));
> +       page = alloc_pages(GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO,
> +                                       get_order(memmap_size));
>        if (page)
> -               goto got_map_page;
> -
> -       ret = vmalloc(memmap_size);
> -       if (ret)
> -               goto got_map_ptr;
> -
> -       return NULL;
> -got_map_page:
> -       ret = (struct page *)pfn_to_kaddr(page_to_pfn(page));
> -got_map_ptr:
> -       memset(ret, 0, memmap_size);
> +               ret = (struct page *)pfn_to_kaddr(page_to_pfn(page));
> +       else
> +               ret = vzalloc(memmap_size);
>
>        return ret;
>  }
> --
> 1.6.3.3
>
>
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> 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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] sparse: using kzalloc to clean up code

[Bob Liu]

On Thu, Aug 4, 2011 at 2:10 PM, Pekka Enberg <penberg@kernel.org> wrote:
> On Thu, Aug 4, 2011 at 6:09 AM, Bob Liu <lliubbo@gmail.com> wrote:
>> This patch using kzalloc to clean up sparse_index_alloc() and
>> __GFP_ZERO to clean up __kmalloc_section_memmap().
>>
>> Signed-off-by: Bob Liu <lliubbo@gmail.com>
>> ---
>>  mm/sparse.c |   24 +++++++-----------------
>>  1 files changed, 7 insertions(+), 17 deletions(-)
>>
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index 858e1df..9596635 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -65,15 +65,12 @@ static struct mem_section noinline __init_refok *sparse_index_alloc(int nid)
>>
>>        if (slab_is_available()) {
>>                if (node_state(nid, N_HIGH_MEMORY))
>> -                       section = kmalloc_node(array_size, GFP_KERNEL, nid);
>> +                       section = kzalloc_node(array_size, GFP_KERNEL, nid);
>>                else
>> -                       section = kmalloc(array_size, GFP_KERNEL);
>> +                       section = kzalloc(array_size, GFP_KERNEL);
>>        } else
>>                section = alloc_bootmem_node(NODE_DATA(nid), array_size);
>>
>> -       if (section)
>> -               memset(section, 0, array_size);
>> -
>
> You now broke the alloc_bootmem_node() path.
>

Yes.
But In my opinion, the alloc_bootmem_node() will also return zeroed memory.
I saw it has used kzalloc or memset() but i'm not pretty sure.
CC'd yinghai@kernel.org,hpa@zytor.com

Thanks for your review.

>>        return section;
>>  }
>>
>> @@ -636,19 +633,12 @@ static struct page *__kmalloc_section_memmap(unsigned long nr_pages)
>>        struct page *page, *ret;
>>        unsigned long memmap_size = sizeof(struct page) * nr_pages;
>>
>> -       page = alloc_pages(GFP_KERNEL|__GFP_NOWARN, get_order(memmap_size));
>> +       page = alloc_pages(GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO,
>> +                                       get_order(memmap_size));
>>        if (page)
>> -               goto got_map_page;
>> -
>> -       ret = vmalloc(memmap_size);
>> -       if (ret)
>> -               goto got_map_ptr;
>> -
>> -       return NULL;
>> -got_map_page:
>> -       ret = (struct page *)pfn_to_kaddr(page_to_pfn(page));
>> -got_map_ptr:
>> -       memset(ret, 0, memmap_size);
>> +               ret = (struct page *)pfn_to_kaddr(page_to_pfn(page));
>> +       else
>> +               ret = vzalloc(memmap_size);
>>
>>        return ret;
>>  }
>> --
>> 1.6.3.3
>>
>>
>> --
>> 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/ .
>> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>
>

-- 
Regards,
--Bob

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] sparse: using kzalloc to clean up code

[Johannes Weiner]

On Thu, Aug 04, 2011 at 02:55:17PM +0800, Bob Liu wrote:
> On Thu, Aug 4, 2011 at 2:10 PM, Pekka Enberg <penberg@kernel.org> wrote:
> > On Thu, Aug 4, 2011 at 6:09 AM, Bob Liu <lliubbo@gmail.com> wrote:
> >> This patch using kzalloc to clean up sparse_index_alloc() and
> >> __GFP_ZERO to clean up __kmalloc_section_memmap().
> >>
> >> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> >> ---
> >>  mm/sparse.c |   24 +++++++-----------------
> >>  1 files changed, 7 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/mm/sparse.c b/mm/sparse.c
> >> index 858e1df..9596635 100644
> >> --- a/mm/sparse.c
> >> +++ b/mm/sparse.c
> >> @@ -65,15 +65,12 @@ static struct mem_section noinline __init_refok *sparse_index_alloc(int nid)
> >>
> >>        if (slab_is_available()) {
> >>                if (node_state(nid, N_HIGH_MEMORY))
> >> -                       section = kmalloc_node(array_size, GFP_KERNEL, nid);
> >> +                       section = kzalloc_node(array_size, GFP_KERNEL, nid);
> >>                else
> >> -                       section = kmalloc(array_size, GFP_KERNEL);
> >> +                       section = kzalloc(array_size, GFP_KERNEL);
> >>        } else
> >>                section = alloc_bootmem_node(NODE_DATA(nid), array_size);
> >>
> >> -       if (section)
> >> -               memset(section, 0, array_size);
> >> -
> >
> > You now broke the alloc_bootmem_node() path.
> >
> 
> Yes.
> But In my opinion, the alloc_bootmem_node() will also return zeroed memory.
> I saw it has used kzalloc or memset() but i'm not pretty sure.
> CC'd yinghai@kernel.org,hpa@zytor.com

You are right, bootmem always returns zeroed memory.  But it deserves
mentioning in the changelog.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] sparse: using kzalloc to clean up code

[Johannes Weiner]

On Thu, Aug 04, 2011 at 11:09:49AM +0800, Bob Liu wrote:
> This patch using kzalloc to clean up sparse_index_alloc() and
> __GFP_ZERO to clean up __kmalloc_section_memmap().
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] sparse: using kzalloc to clean up code

[Pekka Enberg]

On Thu, Aug 4, 2011 at 10:26 AM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> On Thu, Aug 04, 2011 at 11:09:49AM +0800, Bob Liu wrote:
>> This patch using kzalloc to clean up sparse_index_alloc() and
>> __GFP_ZERO to clean up __kmalloc_section_memmap().
>>
>> Signed-off-by: Bob Liu <lliubbo@gmail.com>
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Reviewed-by: Pekka Enberg <penberg@kernel.org>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] sparse: using kzalloc to clean up code

[Michal Hocko]

On Thu 04-08-11 11:09:49, Bob Liu wrote:
> This patch using kzalloc to clean up sparse_index_alloc() and
> __GFP_ZERO to clean up __kmalloc_section_memmap().
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

looks good.

Reviewed-by: Michal Hocko <mhocko@suse.cz>
> ---
>  mm/sparse.c |   24 +++++++-----------------
>  1 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 858e1df..9596635 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -65,15 +65,12 @@ static struct mem_section noinline __init_refok *sparse_index_alloc(int nid)
>  
>  	if (slab_is_available()) {
>  		if (node_state(nid, N_HIGH_MEMORY))
> -			section = kmalloc_node(array_size, GFP_KERNEL, nid);
> +			section = kzalloc_node(array_size, GFP_KERNEL, nid);
>  		else
> -			section = kmalloc(array_size, GFP_KERNEL);
> +			section = kzalloc(array_size, GFP_KERNEL);
>  	} else
>  		section = alloc_bootmem_node(NODE_DATA(nid), array_size);
>  
> -	if (section)
> -		memset(section, 0, array_size);
> -
>  	return section;
>  }
>  
> @@ -636,19 +633,12 @@ static struct page *__kmalloc_section_memmap(unsigned long nr_pages)
>  	struct page *page, *ret;
>  	unsigned long memmap_size = sizeof(struct page) * nr_pages;
>  
> -	page = alloc_pages(GFP_KERNEL|__GFP_NOWARN, get_order(memmap_size));
> +	page = alloc_pages(GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO,
> +					get_order(memmap_size));
>  	if (page)
> -		goto got_map_page;
> -
> -	ret = vmalloc(memmap_size);
> -	if (ret)
> -		goto got_map_ptr;
> -
> -	return NULL;
> -got_map_page:
> -	ret = (struct page *)pfn_to_kaddr(page_to_pfn(page));
> -got_map_ptr:
> -	memset(ret, 0, memmap_size);
> +		ret = (struct page *)pfn_to_kaddr(page_to_pfn(page));
> +	else
> +		ret = vzalloc(memmap_size);
>  
>  	return ret;
>  }
> -- 
> 1.6.3.3
> 
> 
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc

[Michal Hocko]

On Thu 04-08-11 11:09:48, Bob Liu wrote:
> This patch also add checking whether alloc frontswap_map memory
> failed.
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> ---
>  mm/swapfile.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index ffdd06a..8fe9e88 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -2124,9 +2124,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>  	}
>  	/* frontswap enabled? set up bit-per-page map for frontswap */
>  	if (frontswap_enabled) {
> -		frontswap_map = vmalloc(maxpages / sizeof(long));
> -		if (frontswap_map)
> -			memset(frontswap_map, 0, maxpages / sizeof(long));
> +		frontswap_map = vzalloc(maxpages / sizeof(long));
> +		if (!frontswap_map)
> +			goto bad_swap;

vzalloc part looks good but shouldn't we disable frontswap rather than
fail?

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc

[Johannes Weiner]

On Thu, Aug 04, 2011 at 09:57:30AM +0200, Michal Hocko wrote:
> On Thu 04-08-11 11:09:48, Bob Liu wrote:
> > This patch also add checking whether alloc frontswap_map memory
> > failed.
> > 
> > Signed-off-by: Bob Liu <lliubbo@gmail.com>
> > ---
> >  mm/swapfile.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index ffdd06a..8fe9e88 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -2124,9 +2124,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> >  	}
> >  	/* frontswap enabled? set up bit-per-page map for frontswap */
> >  	if (frontswap_enabled) {
> > -		frontswap_map = vmalloc(maxpages / sizeof(long));
> > -		if (frontswap_map)
> > -			memset(frontswap_map, 0, maxpages / sizeof(long));
> > +		frontswap_map = vzalloc(maxpages / sizeof(long));
> > +		if (!frontswap_map)
> > +			goto bad_swap;
> 
> vzalloc part looks good but shouldn't we disable frontswap rather than
> fail?

Silently dropping explicitely enabled features is not a good idea,
IMO.  But from a quick look, this seems to be actually happening as
frontswap's bitmap tests check for whether there is even a bitmap
allocated and it should essentially never do anything for real if
there isn't.

How about printing a warning as to why the swapon fails and give the
admin a choice to disable it on her own?

It's outside this patch's scope, though, just as changing the
behaviour to fail swapon is.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc

[Michal Hocko]

On Thu 04-08-11 10:14:07, Johannes Weiner wrote:
> On Thu, Aug 04, 2011 at 09:57:30AM +0200, Michal Hocko wrote:
> > On Thu 04-08-11 11:09:48, Bob Liu wrote:
> > > This patch also add checking whether alloc frontswap_map memory
> > > failed.
> > > 
> > > Signed-off-by: Bob Liu <lliubbo@gmail.com>
> > > ---
> > >  mm/swapfile.c |    6 +++---
> > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > > index ffdd06a..8fe9e88 100644
> > > --- a/mm/swapfile.c
> > > +++ b/mm/swapfile.c
> > > @@ -2124,9 +2124,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> > >  	}
> > >  	/* frontswap enabled? set up bit-per-page map for frontswap */
> > >  	if (frontswap_enabled) {
> > > -		frontswap_map = vmalloc(maxpages / sizeof(long));
> > > -		if (frontswap_map)
> > > -			memset(frontswap_map, 0, maxpages / sizeof(long));
> > > +		frontswap_map = vzalloc(maxpages / sizeof(long));
> > > +		if (!frontswap_map)
> > > +			goto bad_swap;
> > 
> > vzalloc part looks good but shouldn't we disable frontswap rather than
> > fail?
> 
> Silently dropping explicitely enabled features is not a good idea,
> IMO.  

Sure, I didn't mean silently. It should be a big fat warning that there
is not enough memory to enable the feature.

> But from a quick look, this seems to be actually happening as
> frontswap's bitmap tests check for whether there is even a bitmap
> allocated and it should essentially never do anything for real if
> there isn't.

Yes, that was my impression as well. I wasn't 100% sure about that
though, because there are many places which check frontswap_enabled and
do not check the map. I though that disabling the feature should be
safer.

> How about printing a warning as to why the swapon fails and give the
> admin a choice to disable it on her own?

I am not that familiar with the code but drivers/staging/zcache/zcache.c
says:
/*
 * zcache initialization
 * NOTE FOR NOW zcache MUST BE PROVIDED AS A KERNEL BOOT PARAMETER OR
 * NOTHING HAPPENS!
 */

Is there something admin can do about it?

> 
> It's outside this patch's scope, though, just as changing the
> behaviour to fail swapon is.

Agreed. The patch should just use vzalloc and the allocation failure
should be handled separately - if needed at all.

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc

[Dan Magenheimer]

> From: Michal Hocko [mailto:mhocko@suse.cz]
> Sent: Thursday, August 04, 2011 3:00 AM
> Subject: Re: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc
> 
> On Thu 04-08-11 10:14:07, Johannes Weiner wrote:
> > On Thu, Aug 04, 2011 at 09:57:30AM +0200, Michal Hocko wrote:
> > > On Thu 04-08-11 11:09:48, Bob Liu wrote:
> > > > This patch also add checking whether alloc frontswap_map memory
> > > > failed.
> > > >
> > > > Signed-off-by: Bob Liu <lliubbo@gmail.com>
> > > > ---
> > > >  mm/swapfile.c |    6 +++---
> > > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > > > index ffdd06a..8fe9e88 100644
> > > > --- a/mm/swapfile.c
> > > > +++ b/mm/swapfile.c
> > > > @@ -2124,9 +2124,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> > > >  	}
> > > >  	/* frontswap enabled? set up bit-per-page map for frontswap */
> > > >  	if (frontswap_enabled) {
> > > > -		frontswap_map = vmalloc(maxpages / sizeof(long));
> > > > -		if (frontswap_map)
> > > > -			memset(frontswap_map, 0, maxpages / sizeof(long));
> > > > +		frontswap_map = vzalloc(maxpages / sizeof(long));
> > > > +		if (!frontswap_map)
> > > > +			goto bad_swap;
> > >
> > > vzalloc part looks good but shouldn't we disable frontswap rather than
> > > fail?
> >
> > Silently dropping explicitely enabled features is not a good idea,
> > IMO.
> 
> Sure, I didn't mean silently. It should be a big fat warning that there
> is not enough memory to enable the feature.
> 
> > But from a quick look, this seems to be actually happening as
> > frontswap's bitmap tests check for whether there is even a bitmap
> > allocated and it should essentially never do anything for real if
> > there isn't.
> 
> Yes, that was my impression as well. I wasn't 100% sure about that
> though, because there are many places which check frontswap_enabled and
> do not check the map. I though that disabling the feature should be
> safer.
> 
> > How about printing a warning as to why the swapon fails and give the
> > admin a choice to disable it on her own?
> 
> I am not that familiar with the code but drivers/staging/zcache/zcache.c
> says:
> /*
>  * zcache initialization
>  * NOTE FOR NOW zcache MUST BE PROVIDED AS A KERNEL BOOT PARAMETER OR
>  * NOTHING HAPPENS!
>  */
> 
> Is there something admin can do about it?
> 
> >
> > It's outside this patch's scope, though, just as changing the
> > behaviour to fail swapon is.
> 
> Agreed. The patch should just use vzalloc and the allocation failure
> should be handled separately - if needed at all.

Agreed here too.  The frontswap_enabled flag is global (enabling frontswap
across all frontswap devices) whereas failure to allocate the frontswap_map
will disable frontswap for only one swap device.  And since frontswap is
strictly a performance enhancement, there's no reason to fail the swapon
for the entire swap device.

I am fairly sure that the failed allocation is handled gracefully
through the remainder of the frontswap code, but will re-audit to
confirm.  A warning might be nice though.

In any case:

> -		frontswap_map = vmalloc(maxpages / sizeof(long));
> -		if (frontswap_map)
> -			memset(frontswap_map, 0, maxpages / sizeof(long));
> +		frontswap_map = vzalloc(maxpages / sizeof(long));

Acked-by: Dan Magenheimer <dan.magenheimer@oracle.com>

> +		if (!frontswap_map)
> +			goto bad_swap;

NAK

Dan

Thanks... for the memory!
I really could use more / my throughput's on the floor
The balloon is flat / my swap disk's fat / I've OOM's in store
Overcommitted so much
(with apologies to Bob Hope)

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc

[Bob Liu]

On Fri, Aug 5, 2011 at 12:47 AM, Dan Magenheimer
<dan.magenheimer@oracle.com> wrote:
>> From: Michal Hocko [mailto:mhocko@suse.cz]
>> Sent: Thursday, August 04, 2011 3:00 AM
>> Subject: Re: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc
>>
>> On Thu 04-08-11 10:14:07, Johannes Weiner wrote:
>> > On Thu, Aug 04, 2011 at 09:57:30AM +0200, Michal Hocko wrote:
>> > > On Thu 04-08-11 11:09:48, Bob Liu wrote:
>> > > > This patch also add checking whether alloc frontswap_map memory
>> > > > failed.
>> > > >
>> > > > Signed-off-by: Bob Liu <lliubbo@gmail.com>
>> > > > ---
>> > > >  mm/swapfile.c |    6 +++---
>> > > >  1 files changed, 3 insertions(+), 3 deletions(-)
>> > > >
>> > > > diff --git a/mm/swapfile.c b/mm/swapfile.c
>> > > > index ffdd06a..8fe9e88 100644
>> > > > --- a/mm/swapfile.c
>> > > > +++ b/mm/swapfile.c
>> > > > @@ -2124,9 +2124,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>> > > >         }
>> > > >         /* frontswap enabled? set up bit-per-page map for frontswap */
>> > > >         if (frontswap_enabled) {
>> > > > -               frontswap_map = vmalloc(maxpages / sizeof(long));
>> > > > -               if (frontswap_map)
>> > > > -                       memset(frontswap_map, 0, maxpages / sizeof(long));
>> > > > +               frontswap_map = vzalloc(maxpages / sizeof(long));
>> > > > +               if (!frontswap_map)
>> > > > +                       goto bad_swap;
>> > >
>> > > vzalloc part looks good but shouldn't we disable frontswap rather than
>> > > fail?
>> >
>> > Silently dropping explicitely enabled features is not a good idea,
>> > IMO.
>>
>> Sure, I didn't mean silently. It should be a big fat warning that there
>> is not enough memory to enable the feature.
>>
>> > But from a quick look, this seems to be actually happening as
>> > frontswap's bitmap tests check for whether there is even a bitmap
>> > allocated and it should essentially never do anything for real if
>> > there isn't.
>>
>> Yes, that was my impression as well. I wasn't 100% sure about that
>> though, because there are many places which check frontswap_enabled and
>> do not check the map. I though that disabling the feature should be
>> safer.
>>
>> > How about printing a warning as to why the swapon fails and give the
>> > admin a choice to disable it on her own?
>>
>> I am not that familiar with the code but drivers/staging/zcache/zcache.c
>> says:
>> /*
>>  * zcache initialization
>>  * NOTE FOR NOW zcache MUST BE PROVIDED AS A KERNEL BOOT PARAMETER OR
>>  * NOTHING HAPPENS!
>>  */
>>
>> Is there something admin can do about it?
>>
>> >
>> > It's outside this patch's scope, though, just as changing the
>> > behaviour to fail swapon is.
>>
>> Agreed. The patch should just use vzalloc and the allocation failure
>> should be handled separately - if needed at all.
>
> Agreed here too.  The frontswap_enabled flag is global (enabling frontswap
> across all frontswap devices) whereas failure to allocate the frontswap_map
> will disable frontswap for only one swap device.  And since frontswap is
> strictly a performance enhancement, there's no reason to fail the swapon
> for the entire swap device.
>

Agreed.

> I am fairly sure that the failed allocation is handled gracefully
> through the remainder of the frontswap code, but will re-audit to
> confirm.  A warning might be nice though.
>

There is a place i think maybe have problem.
function __frontswap_flush_area() in file frontswap.c called
memset(sis->frontswap_map, .., ..);
But if frontswap_map allocation fail there is a null pointer access ?

> In any case:
>
>> -             frontswap_map = vmalloc(maxpages / sizeof(long));
>> -             if (frontswap_map)
>> -                     memset(frontswap_map, 0, maxpages / sizeof(long));
>> +             frontswap_map = vzalloc(maxpages / sizeof(long));
>
> Acked-by: Dan Magenheimer <dan.magenheimer@oracle.com>
>

Thanks,

>> +             if (!frontswap_map)
>> +                     goto bad_swap;
>
> NAK
>
> Dan
>
> Thanks... for the memory!
> I really could use more / my throughput's on the floor
> The balloon is flat / my swap disk's fat / I've OOM's in store
> Overcommitted so much
> (with apologies to Bob Hope)
>

:)

-- 
Regards,
--Bob

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc

[Dan Magenheimer]

> > I am fairly sure that the failed allocation is handled gracefully
> > through the remainder of the frontswap code, but will re-audit to
> > confirm.  A warning might be nice though.
> 
> There is a place i think maybe have problem.
> function __frontswap_flush_area() in file frontswap.c called
> memset(sis->frontswap_map, .., ..);
> But if frontswap_map allocation fail there is a null pointer access ?

Good catch!

I'll fix that when I submit a frontswap update in a few days.

Thanks,
Dan

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc

[Bob Liu]

On Fri, Aug 5, 2011 at 10:45 AM, Dan Magenheimer
<dan.magenheimer@oracle.com> wrote:
>> > I am fairly sure that the failed allocation is handled gracefully
>> > through the remainder of the frontswap code, but will re-audit to
>> > confirm.  A warning might be nice though.
>>
>> There is a place i think maybe have problem.
>> function __frontswap_flush_area() in file frontswap.c called
>> memset(sis->frontswap_map, .., ..);
>> But if frontswap_map allocation fail there is a null pointer access ?
>
> Good catch!
>
> I'll fix that when I submit a frontswap update in a few days.
>

Would you please add current patch to you frontswap update series ?
So I needn't to send a Version 2 separately with only drop the
allocation failed handler.
Thanks.

-- 
Regards,
--Bob

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc

[Dan Magenheimer]

> From: Bob Liu [mailto:lliubbo@gmail.com]
> Subject: Re: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc
> 
> On Fri, Aug 5, 2011 at 10:45 AM, Dan Magenheimer
> <dan.magenheimer@oracle.com> wrote:
> >> > I am fairly sure that the failed allocation is handled gracefully
> >> > through the remainder of the frontswap code, but will re-audit to
> >> > confirm.  A warning might be nice though.
> >>
> >> There is a place i think maybe have problem.
> >> function __frontswap_flush_area() in file frontswap.c called
> >> memset(sis->frontswap_map, .., ..);
> >> But if frontswap_map allocation fail there is a null pointer access ?
> >
> > Good catch!
> >
> > I'll fix that when I submit a frontswap update in a few days.
> 
> Would you please add current patch to you frontswap update series ?
> So I needn't to send a Version 2 separately with only drop the
> allocation failed handler.
> Thanks.
> Regards,
> --Bob

Hi Bob --

I'm not an expert here, so you or others can feel free to correct me if I've
got this wrong or if I misunderstood you, but I don't think that's the way
patchsets are supposed to be done, at least until they are merged into Linus'
tree.  I think you are asking me to add a fifth patch in the frontswap
patch series that fixes this bug, rather than incorporate the fix into
the next posted version of the frontswap patchset.  However, I expect
to post V5 soon with some additional (minor syntactic) changes to the
patchset from Konrad Wilk's very thorough review.  Then this V5 will
replace the current version in linux-next soon thereafter (and hopefully
then into linux-3.2.)  So I think it would be the correct process for me
to include your bugfix (with an acknowledgement in the commit log) in
that posted V5.

That said, if you are using frontswap V4 (the version currently in
linux-next), the bug fix we've discussed needs to be fixed but is
exceedingly unlikely to occur in the real world because it would
require the malloc of swap_map to succeed (which is 8 bits per swap page
in the swapon'ed swap device) but the malloc of frontswap_map immediately
thereafter to fail (which is 1 bit per swap page in the swapon'ed swap
device).  (And also this is not a problem for the vast majority of
kernel developers... it's only possible for frontswap users like you that
have enabled zcache or tmem or RAMster via a kernel boot option.)

Thanks,
Dan

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc

[Bob Liu]

On Sat, Aug 6, 2011 at 2:13 AM, Dan Magenheimer
<dan.magenheimer@oracle.com> wrote:
>> From: Bob Liu [mailto:lliubbo@gmail.com]
>> Subject: Re: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc
>>
>> On Fri, Aug 5, 2011 at 10:45 AM, Dan Magenheimer
>> <dan.magenheimer@oracle.com> wrote:
>> >> > I am fairly sure that the failed allocation is handled gracefully
>> >> > through the remainder of the frontswap code, but will re-audit to
>> >> > confirm.  A warning might be nice though.
>> >>
>> >> There is a place i think maybe have problem.
>> >> function __frontswap_flush_area() in file frontswap.c called
>> >> memset(sis->frontswap_map, .., ..);
>> >> But if frontswap_map allocation fail there is a null pointer access ?
>> >
>> > Good catch!
>> >
>> > I'll fix that when I submit a frontswap update in a few days.
>>
>> Would you please add current patch to you frontswap update series ?
>> So I needn't to send a Version 2 separately with only drop the
>> allocation failed handler.
>> Thanks.
>> Regards,
>> --Bob
>
> Hi Bob --
>
> I'm not an expert here, so you or others can feel free to correct me if I've
> got this wrong or if I misunderstood you, but I don't think that's the way
> patchsets are supposed to be done, at least until they are merged into Linus'
> tree.  I think you are asking me to add a fifth patch in the frontswap
> patch series that fixes this bug, rather than incorporate the fix into
> the next posted version of the frontswap patchset.  However, I expect
> to post V5 soon with some additional (minor syntactic) changes to the
> patchset from Konrad Wilk's very thorough review.  Then this V5 will
> replace the current version in linux-next soon thereafter (and hopefully
> then into linux-3.2.)  So I think it would be the correct process for me
> to include your bugfix (with an acknowledgement in the commit log) in
> that posted V5.
>

Yes, but current patch "frontswap: using vzalloc instead of vmalloc"
has the error handler
which is unneeded.
+               if (!frontswap_map)
+                       goto bad_swap;

If you want to include it into your series you must delete it by
yourself(or I send an new one) and then add an
extra patch which fix the frontswap_map null pointer bug into your series too.

That's what I want. Sorry for the noise :).

> That said, if you are using frontswap V4 (the version currently in
> linux-next), the bug fix we've discussed needs to be fixed but is
> exceedingly unlikely to occur in the real world because it would
> require the malloc of swap_map to succeed (which is 8 bits per swap page
> in the swapon'ed swap device) but the malloc of frontswap_map immediately
> thereafter to fail (which is 1 bit per swap page in the swapon'ed swap
> device).  (And also this is not a problem for the vast majority of
> kernel developers... it's only possible for frontswap users like you that
> have enabled zcache or tmem or RAMster via a kernel boot option.)
>
> Thanks,
> Dan
>

-- 
Regards,
--Bob

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] page cgroup: using vzalloc instead of vmalloc

[Pekka Enberg]

On Thu, Aug 4, 2011 at 6:09 AM, Bob Liu <lliubbo@gmail.com> wrote:
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

Reviewed-by: Pekka Enberg <penberg@kernel.org>

> ---
>  mm/page_cgroup.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/mm/page_cgroup.c b/mm/page_cgroup.c
> index 39d216d..6bdc67d 100644
> --- a/mm/page_cgroup.c
> +++ b/mm/page_cgroup.c
> @@ -513,11 +513,10 @@ int swap_cgroup_swapon(int type, unsigned long max_pages)
>        length = DIV_ROUND_UP(max_pages, SC_PER_PAGE);
>        array_size = length * sizeof(void *);
>
> -       array = vmalloc(array_size);
> +       array = vzalloc(array_size);
>        if (!array)
>                goto nomem;
>
> -       memset(array, 0, array_size);
>        ctrl = &swap_cgroup_ctrl[type];
>        mutex_lock(&swap_cgroup_mutex);
>        ctrl->length = length;
> --
> 1.6.3.3
>
>
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> 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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] page cgroup: using vzalloc instead of vmalloc

[Johannes Weiner]

On Thu, Aug 04, 2011 at 11:09:47AM +0800, Bob Liu wrote:
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] page cgroup: using vzalloc instead of vmalloc

[Michal Hocko]

On Thu 04-08-11 11:09:47, Bob Liu wrote:
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

Reviewed-by: Michal Hocko <mhocko@suse.cz>

> ---
>  mm/page_cgroup.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_cgroup.c b/mm/page_cgroup.c
> index 39d216d..6bdc67d 100644
> --- a/mm/page_cgroup.c
> +++ b/mm/page_cgroup.c
> @@ -513,11 +513,10 @@ int swap_cgroup_swapon(int type, unsigned long max_pages)
>  	length = DIV_ROUND_UP(max_pages, SC_PER_PAGE);
>  	array_size = length * sizeof(void *);
>  
> -	array = vmalloc(array_size);
> +	array = vzalloc(array_size);
>  	if (!array)
>  		goto nomem;
>  
> -	memset(array, 0, array_size);
>  	ctrl = &swap_cgroup_ctrl[type];
>  	mutex_lock(&swap_cgroup_mutex);
>  	ctrl->length = length;
> -- 
> 1.6.3.3
> 
> 

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] page cgroup: using vzalloc instead of vmalloc

[KAMEZAWA Hiroyuki]

On Thu, 4 Aug 2011 11:09:47 +0800
Bob Liu <lliubbo@gmail.com> wrote:

> Signed-off-by: Bob Liu <lliubbo@gmail.com>

Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

> ---
>  mm/page_cgroup.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_cgroup.c b/mm/page_cgroup.c
> index 39d216d..6bdc67d 100644
> --- a/mm/page_cgroup.c
> +++ b/mm/page_cgroup.c
> @@ -513,11 +513,10 @@ int swap_cgroup_swapon(int type, unsigned long max_pages)
>  	length = DIV_ROUND_UP(max_pages, SC_PER_PAGE);
>  	array_size = length * sizeof(void *);
>  
> -	array = vmalloc(array_size);
> +	array = vzalloc(array_size);
>  	if (!array)
>  		goto nomem;
>  
> -	memset(array, 0, array_size);
>  	ctrl = &swap_cgroup_ctrl[type];
>  	mutex_lock(&swap_cgroup_mutex);
>  	ctrl->length = length;
> -- 
> 1.6.3.3
> 
> 
> 

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>