slub: return correct error on slab_sysfs_init

PATCH RESEND In review Jeff Liu 4 participants 9 messages Jun 18, 2014 – Jun 23, 2014

Subsystems (2)

Memory management Slab allocator

lore

Share & tools

lore

Jeff Liu

author

2014-06-18 01:29 UTC #0 lore raw

PATCH RESEND 1/1 slub: return correct error on slab_sysfs_init

From: Jie Liu <[email protected]>

Return -ENOMEM than -ENOSYS if kset_create_and_add() failed

Signed-off-by Jie Liu <[email protected]>

---
 mm/slub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/slub.c +1 -1

index b2b0473..e10f60f 100644
@@ -5301,7 +5301,7 @@ static int __init slab_sysfs_init(void)
 	if (!slab_kset) {
 		mutex_unlock(&slab_mutex);
 		pr_err("Cannot register slab subsystem.\n");
-		return -ENOSYS;
+		return -ENOMEM;
 	}
 
 	slab_state = FULL;

Signature · 7 lines

1.8.3.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

David Rientjes

reviewer

2014-06-18 20:16 UTC #1 lore raw

Quoted 4 lines · Jeff Liu

From: Jie Liu <[email protected]>
Return -ENOMEM than -ENOSYS if kset_create_and_add() failed

Why? kset_create_and_add() can fail for a few other reasons other than memory constraints and given that this is only done at bootstrap, it actually seems like a duplicate name would be a bigger concern than low on memory if another init call actually registered it.

Quoted 10 lines

Signed-off-by Jie Liu <[email protected]>
---
 mm/slub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/slub.c +0 -0
index b2b0473..e10f60f 100644

mm/slub.c static int __init slab_sysfs_init(void) · line 5307

Quoted 8 lines

mm/slub.c +1 -1

@@ -5301,7 +5301,7 @@ static int __init slab_sysfs_init(void)
 	if (!slab_kset) {
 		mutex_unlock(&slab_mutex);
 		pr_err("Cannot register slab subsystem.\n");
-		return -ENOSYS;
+		return -ENOMEM;
 	}
 
 	slab_state = FULL;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

reviewer Acked-by

2014-06-19 14:39 UTC #2 lore raw

Quoted 4 lines · David Rientjes

Why? kset_create_and_add() can fail for a few other reasons other than memory constraints and given that this is only done at bootstrap, it actually seems like a duplicate name would be a bigger concern than low on memory if another init call actually registered it.

Greg said that the only reason for failure would be out of memory.

Acked-by Christoph Lameter <[email protected]>

Andrew Morton

maintainer

2014-06-19 20:32 UTC #3 lore raw

Quoted 8 lines · On Thu, 19 Jun 2014 09:39:54 -0500 (CDT) Christoph Lameter <[email protected]>…

Quoted 4 lines · David Rientjes
Why? kset_create_and_add() can fail for a few other reasons other than memory constraints and given that this is only done at bootstrap, it actually seems like a duplicate name would be a bigger concern than low on memory if another init call actually registered it.
Greg said that the only reason for failure would be out of memory.

The kset_create_and_add interface is busted - it should return an ERR_PTR on error, not NULL. This seems to be a common gregkh failing :(

It's plausible that out-of-memory is the most common reason for kset_create_and_add() failure, dunno.

Jeff, the changelog wasn't a good one - it failed to describe the reasons for the change. What was wrong with ENOSYS and why is ENOMEM more appropriate? If Greg told us that out-of-memory is the only possible reason for the failure then it would be useful to capture the reasoning behind this within this changelog.

Also let's describe the effects of this patch. It looks like it's just cosmetic - if kset_create_and_add() fails, the kernel behavior will be the same either way.

David Rientjes

reviewer

2014-06-19 21:34 UTC #4 lore raw

Quoted 5 lines · Andrew Morton

Quoted 5 lines
Quoted 4 lines
Why? kset_create_and_add() can fail for a few other reasons other than memory constraints and given that this is only done at bootstrap, it actually seems like a duplicate name would be a bigger concern than low on memory if another init call actually registered it.

Quoted 8 lines

Quoted 1 line
Greg said that the only reason for failure would be out of memory.
The kset_create_and_add interface is busted - it should return an ERR_PTR on error, not NULL. This seems to be a common gregkh failing :(
It's plausible that out-of-memory is the most common reason for kset_create_and_add() failure, dunno.

I seriously doubt out of memory issues are the most common reason for failure since this is only done at init, it seems much more likely that someone accidently added an object of the same name, "slab", erroneous and then -ENOMEM wouldn't make any sense. kset_create_and_add() can most certainly return other errors rather than just -ENOMEM.

Jeff Liu

author

2014-06-20 13:51 UTC #5 lore raw

In reply to Andrew Morton

Quoted 23 lines · Andrew Morton

Quoted 8 lines · On Thu, 19 Jun 2014 09:39:54 -0500 (CDT) Christoph Lameter <[email protected]>…
Quoted 4 lines · David Rientjes
Why? kset_create_and_add() can fail for a few other reasons other than memory constraints and given that this is only done at bootstrap, it actually seems like a duplicate name would be a bigger concern than low on memory if another init call actually registered it.
Greg said that the only reason for failure would be out of memory.
The kset_create_and_add interface is busted - it should return an ERR_PTR on error, not NULL. This seems to be a common gregkh failing :(
It's plausible that out-of-memory is the most common reason for kset_create_and_add() failure, dunno.
Jeff, the changelog wasn't a good one - it failed to describe the reasons for the change. What was wrong with ENOSYS and why is ENOMEM more appropriate? If Greg told us that out-of-memory is the only possible reason for the failure then it would be useful to capture the reasoning behind this within this changelog.

Quoted 3 lines

Also let's describe the effects of this patch. It looks like it's just cosmetic - if kset_create_and_add() fails, the kernel behavior will be the same either way.

I admit that the current changelog is indistinct :)

At that time, I thought it would be ENOMEM because I was review another patch for adding sysfs support to XFS where we return ENOMEM in this case:
http://www.spinics.net/lists/xfs/msg28343.html

This drives to me to think why it should be ENOMEM rather than ERR_PTR since it seems most likely kset_create_and_add() would fails due to other reasons. Hence I looked through kernel sources and figured out most subsystems are return ENOMEM, maybe those subsystems are refers to the kset example code at:
samples/kobject/kset-example.c

So my original motivation is just to make the slub sysfs init error handling in accordance to other subsystems(nitpick) and it does not affect the kernel behaviour.

Combine with Greg's comments, as such, maybe the changelog would looks like the following?

GregKH: the only reason for failure would be out of memory on kset_create_and_add(). return -ENOMEM than -ENOSYS if the call is failed which is consistent with other subsystems in this situation.

Cheers,
-Jeff

David Rientjes

reviewer

2014-06-20 22:30 UTC #6 lore raw

Quoted 10 lines · Jeff Liu

At that time, I thought it would be ENOMEM because I was review another patch for adding sysfs support to XFS where we return ENOMEM in this case:
http://www.spinics.net/lists/xfs/msg28343.html
This drives to me to think why it should be ENOMEM rather than ERR_PTR since it seems most likely kset_create_and_add() would fails due to other reasons. Hence I looked through kernel sources and figured out most subsystems are return ENOMEM, maybe those subsystems are refers to the kset example code at:
samples/kobject/kset-example.c

If you're going to ignore other emails in this thread, then you're not going to make a very strong argument.

kset_create_and_add() can return NULL for reasons OTHER than just ENOMEM. It can also be returned for EEXIST because something registered the kobject with the same name. During init, which is what you're modifying here, the liklihood is higher that it will return EEXIST rather than ENOMEM otherwise there are much bigger problems than return value.

Quoted 3 lines

So my original motivation is just to make the slub sysfs init error handling in accordance to other subsystems(nitpick) and it does not affect the kernel behaviour.

Why should slub match the other incorrect behavior?

What you're never addressing is WHY you are even making this change or even care about the return value. Show userspace breakage that depends on this.

Quoted 7 lines

Combine with Greg's comments, as such, maybe the changelog would looks like the following?
GregKH: the only reason for failure would be out of memory on kset_create_and_add(). return -ENOMEM than -ENOSYS if the call is failed which is consistent with other subsystems in this situation.

Bullshit. Read the above.

If you want to return PTR_ERR() when this fails and fixup all the callers, then propose that patch. Until then, it's a pretty simple rule: if you don't have an errno, don't assume the reason for failure.

Jeff Liu

author

2014-06-21 08:49 UTC #7 lore raw

Quoted 13 lines · David Rientjes

Quoted 10 lines · Jeff Liu
At that time, I thought it would be ENOMEM because I was review another patch for adding sysfs support to XFS where we return ENOMEM in this case:
http://www.spinics.net/lists/xfs/msg28343.html
This drives to me to think why it should be ENOMEM rather than ERR_PTR since it seems most likely kset_create_and_add() would fails due to other reasons. Hence I looked through kernel sources and figured out most subsystems are return ENOMEM, maybe those subsystems are refers to the kset example code at:
samples/kobject/kset-example.c

Quoted 2 lines

If you're going to ignore other emails in this thread, then you're not going to make a very strong argument.

No, I was not intended to ignore your comments, instead, I appreciate your review since it took up your time and time is valuable to everybody. But the time zone is too late to me yesterday, and I need to refresh my head to read through the whole call chains in kset_create_and_add().

Quoted 6 lines

kset_create_and_add() can return NULL for reasons OTHER than just ENOMEM. It can also be returned for EEXIST because something registered the kobject with the same name. During init, which is what you're modifying here, the liklihood is higher that it will return EEXIST rather than ENOMEM otherwise there are much bigger problems than return value.

Agree, now it's clear EEXIST is returned if we trying to create a new kobject with the same name and with the same parent_kobj, i.e,

kset_create_and_add()
kset_register()

    kset_register()
      kobject_add_internal()
        create_dir()
          sysfs_create_dir_ns()->sysfs_warn_dup()...

And also, the likelihood is higher to some extent, indeed.

Quoted 10 lines

Quoted 4 lines
So my original motivation is just to make the slub sysfs init error handling in accordance to other subsystems(nitpick) and it does not affect the kernel behaviour.
Why should slub match the other incorrect behavior?
What you're never addressing is WHY you are even making this change or even care about the return value. Show userspace breakage that depends on this.

For the consistency with other subsystems, but now I don't think it's right due to above reason.

Quoted 9 lines

Quoted 7 lines
Combine with Greg's comments, as such, maybe the changelog would looks like the following?
GregKH: the only reason for failure would be out of memory on kset_create_and_add(). return -ENOMEM than -ENOSYS if the call is failed which is consistent with other subsystems in this situation.
Bullshit. Read the above.

^^^^^^^
I assume that you spoke like that because I have not reply to you in time, I can understand if so. Otherwise, don't talk to me like that no matter who you are!

Quoted 4 lines

If you want to return PTR_ERR() when this fails and fixup all the callers, then propose that patch. Until then, it's a pretty simple rule: if you don't have an errno, don't assume the reason for failure.

As I mentioned previously, Greg don't like to fixup kobjects API via PTR_ERR(). For me, I neither want to propose PTR_ERR to kobject nor try to push the current slub fix, because it's make no sense to slub with either errno.

Cheers,
-Jeff

Christoph Lameter

reviewer

2014-06-23 13:59 UTC #8 lore raw

Quoted 6 lines · Jeff Liu

Quoted 3 lines
Quoted 1 line
Bullshit. Read the above.
^^^^^^^
I assume that you spoke like that because I have not reply to you in time, I can understand if so. Otherwise, don't talk to me like that no matter who you are!

Ignore his BS. You already posted the patches that did what he asked for. Lets go back to that one. I am reversing my reversal. Sorry about that.

Quoted 7 lines

Quoted 3 lines
If you want to return PTR_ERR() when this fails and fixup all the callers, then propose that patch. Until then, it's a pretty simple rule: if you don't have an errno, don't assume the reason for failure.
As I mentioned previously, Greg don't like to fixup kobjects API via PTR_ERR(). For me, I neither want to propose PTR_ERR to kobject nor try to push the current slub fix, because it's make no sense to slub with either errno.

Lets ignore Greg's incorrect assessment and do what Andrew suggested. I prefer to have clean error code passing.