[PATCH 0/2] mm: fix the names of general cma and hugetlb cma

[PATCH 0/2] mm: fix the names of general cma and hugetlb cma

[Barry Song]

the current code of CMA can only work when users pass a const string as name
parameter. we need to fix the way to handle names in CMA.
On the other hand, to avoid name conflicts after enabling CMA_DEBUGFS, each
hugetlb should get a different CMA name.

Barry Song (2):
  mm: cma: fix the name of CMA areas
  mm: hugetlb: fix the name of hugetlb CMA

 mm/cma.c     | 13 ++++++-------
 mm/cma.h     |  4 +++-
 mm/hugetlb.c |  4 +++-
 3 files changed, 12 insertions(+), 9 deletions(-)

-- 
2.23.0

[PATCH 1/2] mm: cma: fix the name of CMA areas

[Barry Song]

if users give a name saved in stack, the current code will generate magic
pointer.
if users don't give a name(NULL), kasprintf() will always return NULL as
we are at the early stage. that means cma_init_reserved_mem() will return
-ENOMEM if users set name parameter as NULL.

Cc: Roman Gushchin <guro@fb.com>
Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
---
 mm/cma.c | 13 ++++++-------
 mm/cma.h |  4 +++-
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/mm/cma.c b/mm/cma.c
index 0463ad2ce06b..b24151fa2101 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -202,13 +202,12 @@ int __init cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
 	 * subsystems (like slab allocator) are available.
 	 */
 	cma = &cma_areas[cma_area_count];
-	if (name) {
-		cma->name = name;
-	} else {
-		cma->name = kasprintf(GFP_KERNEL, "cma%d\n", cma_area_count);
-		if (!cma->name)
-			return -ENOMEM;
-	}
+
+	if (name)
+		snprintf(cma->name, CMA_MAX_NAME, name);
+	else
+		snprintf(cma->name, CMA_MAX_NAME,  "cma%d\n", cma_area_count);
+
 	cma->base_pfn = PFN_DOWN(base);
 	cma->count = size >> PAGE_SHIFT;
 	cma->order_per_bit = order_per_bit;
diff --git a/mm/cma.h b/mm/cma.h
index 33c0b517733c..27d3f0e9f68f 100644
--- a/mm/cma.h
+++ b/mm/cma.h
@@ -2,6 +2,8 @@
 #ifndef __MM_CMA_H__
 #define __MM_CMA_H__
 
+#define CMA_MAX_NAME 64
+
 struct cma {
 	unsigned long   base_pfn;
 	unsigned long   count;
@@ -12,7 +14,7 @@ struct cma {
 	struct hlist_head mem_head;
 	spinlock_t mem_head_lock;
 #endif
-	const char *name;
+	char name[CMA_MAX_NAME];
 };
 
 extern struct cma cma_areas[MAX_CMA_AREAS];
-- 
2.23.0

Re: [PATCH 1/2] mm: cma: fix the name of CMA areas

[Roman Gushchin]

On Wed, Jun 03, 2020 at 08:40:24PM +1200, Barry Song wrote:
> if users give a name saved in stack, the current code will generate magic
> pointer.
> if users don't give a name(NULL), kasprintf() will always return NULL as
> we are at the early stage. that means cma_init_reserved_mem() will return
> -ENOMEM if users set name parameter as NULL.
> 
> Cc: Roman Gushchin <guro@fb.com>
> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>

Acked-by: Roman Gushchin <guro@fb.com>

Thanks!

> ---
>  mm/cma.c | 13 ++++++-------
>  mm/cma.h |  4 +++-
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/cma.c b/mm/cma.c
> index 0463ad2ce06b..b24151fa2101 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -202,13 +202,12 @@ int __init cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
>  	 * subsystems (like slab allocator) are available.
>  	 */
>  	cma = &cma_areas[cma_area_count];
> -	if (name) {
> -		cma->name = name;
> -	} else {
> -		cma->name = kasprintf(GFP_KERNEL, "cma%d\n", cma_area_count);
> -		if (!cma->name)
> -			return -ENOMEM;
> -	}
> +
> +	if (name)
> +		snprintf(cma->name, CMA_MAX_NAME, name);
> +	else
> +		snprintf(cma->name, CMA_MAX_NAME,  "cma%d\n", cma_area_count);
> +
>  	cma->base_pfn = PFN_DOWN(base);
>  	cma->count = size >> PAGE_SHIFT;
>  	cma->order_per_bit = order_per_bit;
> diff --git a/mm/cma.h b/mm/cma.h
> index 33c0b517733c..27d3f0e9f68f 100644
> --- a/mm/cma.h
> +++ b/mm/cma.h
> @@ -2,6 +2,8 @@
>  #ifndef __MM_CMA_H__
>  #define __MM_CMA_H__
>  
> +#define CMA_MAX_NAME 64
> +
>  struct cma {
>  	unsigned long   base_pfn;
>  	unsigned long   count;
> @@ -12,7 +14,7 @@ struct cma {
>  	struct hlist_head mem_head;
>  	spinlock_t mem_head_lock;
>  #endif
> -	const char *name;
> +	char name[CMA_MAX_NAME];
>  };
>  
>  extern struct cma cma_areas[MAX_CMA_AREAS];
> -- 
> 2.23.0
> 
> 
> 

Re: [PATCH 1/2] mm: cma: fix the name of CMA areas

[Mike Kravetz]

On 6/3/20 1:40 AM, Barry Song wrote:
> if users give a name saved in stack, the current code will generate magic
> pointer.
> if users don't give a name(NULL), kasprintf() will always return NULL as
> we are at the early stage. that means cma_init_reserved_mem() will return
> -ENOMEM if users set name parameter as NULL.
> 
> Cc: Roman Gushchin <guro@fb.com>
> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>

Thank you

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

[PATCH 2/2] mm: hugetlb: fix the name of hugetlb CMA

[Barry Song]

once we enable CMA_DEBUGFS, we will get the below errors:
directory 'cma-hugetlb' with parent 'cma' already present

only the first numa node will get a directory in debugfs.
we should have different names for different CMA areas.

Cc: Roman Gushchin <guro@fb.com>
Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
---
 mm/hugetlb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index bcabbe02192b..4ebc4edc3b40 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5586,12 +5586,14 @@ void __init hugetlb_cma_reserve(int order)
 	reserved = 0;
 	for_each_node_state(nid, N_ONLINE) {
 		int res;
+		char name[20];
 
 		size = min(per_node, hugetlb_cma_size - reserved);
 		size = round_up(size, PAGE_SIZE << order);
 
+		snprintf(name, 20, "hugetlb%d", nid);
 		res = cma_declare_contiguous_nid(0, size, 0, PAGE_SIZE << order,
-						 0, false, "hugetlb",
+						 0, false, name,
 						 &hugetlb_cma[nid], nid);
 		if (res) {
 			pr_warn("hugetlb_cma: reservation failed: err %d, node %d",
-- 
2.23.0

Re: [PATCH 2/2] mm: hugetlb: fix the name of hugetlb CMA

[Roman Gushchin]

On Wed, Jun 03, 2020 at 08:40:25PM +1200, Barry Song wrote:
> once we enable CMA_DEBUGFS, we will get the below errors:
> directory 'cma-hugetlb' with parent 'cma' already present
> 
> only the first numa node will get a directory in debugfs.
> we should have different names for different CMA areas.
> 
> Cc: Roman Gushchin <guro@fb.com>
> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>

Acked-by: Roman Gushchin <guro@fb.com>

> ---
>  mm/hugetlb.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index bcabbe02192b..4ebc4edc3b40 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5586,12 +5586,14 @@ void __init hugetlb_cma_reserve(int order)
>  	reserved = 0;
>  	for_each_node_state(nid, N_ONLINE) {
>  		int res;
> +		char name[20];
>  
>  		size = min(per_node, hugetlb_cma_size - reserved);
>  		size = round_up(size, PAGE_SIZE << order);
>  
> +		snprintf(name, 20, "hugetlb%d", nid);
>  		res = cma_declare_contiguous_nid(0, size, 0, PAGE_SIZE << order,
> -						 0, false, "hugetlb",
> +						 0, false, name,
>  						 &hugetlb_cma[nid], nid);
>  		if (res) {
>  			pr_warn("hugetlb_cma: reservation failed: err %d, node %d",
> -- 
> 2.23.0
> 
> 
> 

Re: [PATCH 2/2] mm: hugetlb: fix the name of hugetlb CMA

[Mike Kravetz]

On 6/3/20 1:40 AM, Barry Song wrote:
> once we enable CMA_DEBUGFS, we will get the below errors:
> directory 'cma-hugetlb' with parent 'cma' already present
> 
> only the first numa node will get a directory in debugfs.
> we should have different names for different CMA areas.
> 
> Cc: Roman Gushchin <guro@fb.com>
> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>

Thank you

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

--
Mike Kravetz