mm/cma_debug: Avoid to use global cma_debugfs_root

PATCH New Yue Hu 3 participants 8 messages Feb 21, 2019

Subsystems (2)

Memory management MM misc

lore

Share & tools

lore

Yue Hu

author

2019-02-21 04:01 UTC #0 lore raw

PATCH 1/1 mm/cma_debug: Avoid to use global cma_debugfs_root

From: Yue Hu <[email protected]>

Currently cma_debugfs_root is at global space. That is unnecessary since it will be only used by next cma_debugfs_add_one(). We can just pass it to following calling, it will save global space. Also remove useless idx parameter.

Signed-off-by Yue Hu <[email protected]>

---
 mm/cma_debug.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

mm/cma_debug.c +4 -5

index f234672..2c2c869 100644
@@ -21,8 +21,6 @@ struct cma_mem {
 	unsigned long n;
 };
 
-static struct dentry *cma_debugfs_root;
-
 static int cma_debugfs_get(void *data, u64 *val)
 {
 	unsigned long *p = data;

@@ -162,7 +160,7 @@ static int cma_alloc_write(void *data, u64 val)
 }
 DEFINE_SIMPLE_ATTRIBUTE(cma_alloc_fops, NULL, cma_alloc_write, "%llu\n");
 
-static void cma_debugfs_add_one(struct cma *cma, int idx)
+static void cma_debugfs_add_one(struct cma *cma, struct dentry *root_dentry)
 {
 	struct dentry *tmp;
 	char name[16];

@@ -170,7 +168,7 @@ static void cma_debugfs_add_one(struct cma *cma, int idx)
 
 	scnprintf(name, sizeof(name), "cma-%s", cma->name);
 
-	tmp = debugfs_create_dir(name, cma_debugfs_root);
+	tmp = debugfs_create_dir(name, root_dentry);
 
 	debugfs_create_file("alloc", 0200, tmp, cma, &cma_alloc_fops);
 	debugfs_create_file("free", 0200, tmp, cma, &cma_free_fops);

@@ -188,6 +186,7 @@ static void cma_debugfs_add_one(struct cma *cma, int idx)
 
 static int __init cma_debugfs_init(void)
 {
+	struct dentry *cma_debugfs_root;
 	int i;
 
 	cma_debugfs_root = debugfs_create_dir("cma", NULL);

@@ -195,7 +194,7 @@ static int __init cma_debugfs_init(void)
 		return -ENOMEM;
 
 	for (i = 0; i < cma_area_count; i++)
-		cma_debugfs_add_one(&cma_areas[i], i);
+		cma_debugfs_add_one(&cma_areas[i], cma_debugfs_root);
 
 	return 0;
 }

Signature · 1 line

1.9.1

Yue Hu

author

2019-02-21 04:01 UTC #1 lore raw

PATCH 1/1 mm/cma_debug: Check for null tmp in cma_debugfs_add_one()

From: Yue Hu <[email protected]>

If debugfs_create_dir() failed, the following debugfs_create_file() will be meanless since it depends on non-NULL tmp dentry and it will only waste CPU resource.

Signed-off-by Yue Hu <[email protected]>

---
 mm/cma_debug.c | 2 ++
 1 file changed, 2 insertions(+)

mm/cma_debug.c +2 -0

index 2c2c869..3e9d984 100644
@@ -169,6 +169,8 @@ static void cma_debugfs_add_one(struct cma *cma, struct dentry *root_dentry)
 	scnprintf(name, sizeof(name), "cma-%s", cma->name);
 
 	tmp = debugfs_create_dir(name, root_dentry);
+	if (!tmp)
+		return;
 
 	debugfs_create_file("alloc", 0200, tmp, cma, &cma_alloc_fops);
 	debugfs_create_file("free", 0200, tmp, cma, &cma_free_fops);

Signature · 1 line

1.9.1

Michal Hocko

2019-02-21 08:23 UTC #2 lore raw

Re: [PATCH] mm/cma_debug: Check for null tmp in cma_debugfs_add_one()

Quoted 5 lines · Yue Hu

From: Yue Hu <[email protected]>
If debugfs_create_dir() failed, the following debugfs_create_file() will be meanless since it depends on non-NULL tmp dentry and it will only waste CPU resource.

The file will be created in the debugfs root. But, more importantly. Greg (CCed now) is working on removing the failure paths because he believes they do not really matter for debugfs and they make code more ugly. More importantly a check for NULL is not correct because you get ERR_PTR after recent changes IIRC.

Quoted 22 lines

Signed-off-by Yue Hu <[email protected]>

---
 mm/cma_debug.c | 2 ++
 1 file changed, 2 insertions(+)

mm/cma_debug.c +2 -0

index 2c2c869..3e9d984 100644
@@ -169,6 +169,8 @@ static void cma_debugfs_add_one(struct cma *cma, struct dentry *root_dentry)
 	scnprintf(name, sizeof(name), "cma-%s", cma->name);
 
 	tmp = debugfs_create_dir(name, root_dentry);
+	if (!tmp)
+		return;
 
 	debugfs_create_file("alloc", 0200, tmp, cma, &cma_alloc_fops);
 	debugfs_create_file("free", 0200, tmp, cma, &cma_free_fops);

Signature · 1 line

1.9.1

Signature · 2 lines

Michal Hocko
SUSE Labs

Greg KH

2019-02-21 08:36 UTC #3 lore raw

Re: [PATCH] mm/cma_debug: Check for null tmp in cma_debugfs_add_one()

Quoted 26 lines · Michal Hocko

Quoted 5 lines · Yue Hu
From: Yue Hu <[email protected]>
If debugfs_create_dir() failed, the following debugfs_create_file() will be meanless since it depends on non-NULL tmp dentry and it will only waste CPU resource.
The file will be created in the debugfs root. But, more importantly. Greg (CCed now) is working on removing the failure paths because he believes they do not really matter for debugfs and they make code more ugly. More importantly a check for NULL is not correct because you get ERR_PTR after recent changes IIRC.
Quoted 13 lines
Signed-off-by Yue Hu <[email protected]>
---
 mm/cma_debug.c | 2 ++
 1 file changed, 2 insertions(+)
mm/cma_debug.c +0 -0
index 2c2c869..3e9d984 100644
@@ -169,6 +169,8 @@ static void cma_debugfs_add_one(struct cma *cma, struct dentry *root_dentry)
 	scnprintf(name, sizeof(name), "cma-%s", cma->name);
 

Quoted 3 lines

mm/cma_debug.c static void cma_debugfs_add_one(struct cma *cma, struct dentry *root_dentry) · line 173
Quoted 3 lines
mm/cma_debug.c +2 -0
 	tmp = debugfs_create_dir(name, root_dentry);
+	if (!tmp)
+		return;

Ick, yes, this patch isn't ok, I've been doing lots of work to rip these checks out :)

Thanks for catching this Michal.

greg k-h

Michal Hocko

2019-02-21 08:45 UTC #4 lore raw

Re: [PATCH] mm/cma_debug: Check for null tmp in cma_debugfs_add_one()

Quoted 27 lines · Greg KH

Quoted 26 lines · Michal Hocko
Quoted 5 lines · Yue Hu
From: Yue Hu <[email protected]>
If debugfs_create_dir() failed, the following debugfs_create_file() will be meanless since it depends on non-NULL tmp dentry and it will only waste CPU resource.
The file will be created in the debugfs root. But, more importantly. Greg (CCed now) is working on removing the failure paths because he believes they do not really matter for debugfs and they make code more ugly. More importantly a check for NULL is not correct because you get ERR_PTR after recent changes IIRC.
Quoted 13 lines
Signed-off-by Yue Hu <[email protected]>
---
 mm/cma_debug.c | 2 ++
 1 file changed, 2 insertions(+)
mm/cma_debug.c +0 -0
index 2c2c869..3e9d984 100644
@@ -169,6 +169,8 @@ static void cma_debugfs_add_one(struct cma *cma, struct dentry *root_dentry)
 	scnprintf(name, sizeof(name), "cma-%s", cma->name);
 

Quoted 6 lines

Quoted 3 lines
mm/cma_debug.c static void cma_debugfs_add_one(struct cma *cma, struct dentry *root_dentry) · line 173
Quoted 3 lines
mm/cma_debug.c +2 -0
 	tmp = debugfs_create_dir(name, root_dentry);
+	if (!tmp)
+		return;
Ick, yes, this patch isn't ok, I've been doing lots of work to rip these checks out :)

Btw. I believe that it would help to clarify this stance in the kerneldoc otherwise these checks will be returning back because the general kernel development attitude is to check for errors. As I've said previously debugfs being different is ugly but decision is yours.

Signature · 2 lines

Michal Hocko
SUSE Labs

Greg KH

2019-02-21 09:10 UTC #5 lore raw

Re: [PATCH] mm/cma_debug: Check for null tmp in cma_debugfs_add_one()

Quoted 35 lines · Michal Hocko

Quoted 27 lines · Greg KH
Quoted 26 lines · Michal Hocko
Quoted 5 lines · Yue Hu
From: Yue Hu <[email protected]>
If debugfs_create_dir() failed, the following debugfs_create_file() will be meanless since it depends on non-NULL tmp dentry and it will only waste CPU resource.
The file will be created in the debugfs root. But, more importantly. Greg (CCed now) is working on removing the failure paths because he believes they do not really matter for debugfs and they make code more ugly. More importantly a check for NULL is not correct because you get ERR_PTR after recent changes IIRC.
Quoted 13 lines
Signed-off-by Yue Hu <[email protected]>
---
 mm/cma_debug.c | 2 ++
 1 file changed, 2 insertions(+)
mm/cma_debug.c +0 -0
index 2c2c869..3e9d984 100644
@@ -169,6 +169,8 @@ static void cma_debugfs_add_one(struct cma *cma, struct dentry *root_dentry)
 	scnprintf(name, sizeof(name), "cma-%s", cma->name);
 
Quoted 6 lines
Quoted 3 lines
mm/cma_debug.c static void cma_debugfs_add_one(struct cma *cma, struct dentry *root_dentry) · line 173
Quoted 3 lines
mm/cma_debug.c +2 -0
 	tmp = debugfs_create_dir(name, root_dentry);
+	if (!tmp)
+		return;
Ick, yes, this patch isn't ok, I've been doing lots of work to rip these checks out :)

Quoted 4 lines

Btw. I believe that it would help to clarify this stance in the kerneldoc otherwise these checks will be returning back because the general kernel development attitude is to check for errors. As I've said previously debugfs being different is ugly but decision is yours.

Yes, I'll be doing that, thanks for the reminder.

greg k-h

Yue Hu

author

2019-02-21 08:56 UTC #6 lore raw

Re: [PATCH] mm/cma_debug: Check for null tmp in cma_debugfs_add_one()

In reply to Michal Hocko

Quoted 7 lines · On Thu, 21 Feb 2019 09:23:09 +0100 Michal Hocko <[email protected]> wrote:

Quoted 5 lines · Yue Hu
From: Yue Hu <[email protected]>
If debugfs_create_dir() failed, the following debugfs_create_file() will be meanless since it depends on non-NULL tmp dentry and it will only waste CPU resource.

Quoted 5 lines

The file will be created in the debugfs root. But, more importantly. Greg (CCed now) is working on removing the failure paths because he believes they do not really matter for debugfs and they make code more ugly. More importantly a check for NULL is not correct because you get ERR_PTR after recent changes IIRC.

Same check logic in cma_debugfs_init(), i'm just finding they do not stay the same.

Quoted 24 lines

Quoted 23 lines

Signed-off-by Yue Hu <[email protected]>

---
 mm/cma_debug.c | 2 ++
 1 file changed, 2 insertions(+)

mm/cma_debug.c +2 -0

index 2c2c869..3e9d984 100644
@@ -169,6 +169,8 @@ static void cma_debugfs_add_one(struct cma *cma, struct dentry *root_dentry)
 	scnprintf(name, sizeof(name), "cma-%s", cma->name);
 
 	tmp = debugfs_create_dir(name, root_dentry);
+	if (!tmp)
+		return;
 
 	debugfs_create_file("alloc", 0200, tmp, cma, &cma_alloc_fops);
 	debugfs_create_file("free", 0200, tmp, cma, &cma_free_fops);

Signature · 1 line

1.9.1

Greg KH

2019-02-21 09:10 UTC #7 lore raw

Re: [PATCH] mm/cma_debug: Check for null tmp in cma_debugfs_add_one()

Quoted 10 lines · Yue Hu

Quoted 7 lines · On Thu, 21 Feb 2019 09:23:09 +0100 Michal Hocko <[email protected]> wrote:
Quoted 5 lines · Yue Hu
From: Yue Hu <[email protected]>
If debugfs_create_dir() failed, the following debugfs_create_file() will be meanless since it depends on non-NULL tmp dentry and it will only waste CPU resource.

Quoted 8 lines

Quoted 5 lines
The file will be created in the debugfs root. But, more importantly. Greg (CCed now) is working on removing the failure paths because he believes they do not really matter for debugfs and they make code more ugly. More importantly a check for NULL is not correct because you get ERR_PTR after recent changes IIRC.
Same check logic in cma_debugfs_init(), i'm just finding they do not stay the same.

I have patches to fix that up as well :)

thanks,

greg k-h