[PATCH RESEND] slub: return correct error on slab_sysfs_init

[PATCH RESEND] slub: return correct error on slab_sysfs_init

[Jeff Liu]

From: Jie Liu <jeff.liu@oracle.com>

Return -ENOMEM than -ENOSYS if kset_create_and_add() failed

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
 mm/slub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/slub.c b/mm/slub.c
index b2b0473..e10f60f 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -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;
-- 
1.8.3.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 RESEND] slub: return correct error on slab_sysfs_init

[David Rientjes]

On Wed, 18 Jun 2014, Jeff Liu wrote:

> From: Jie Liu <jeff.liu@oracle.com>
> 
> 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.

> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> ---
>  mm/slub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index b2b0473..e10f60f 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -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 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 RESEND] slub: return correct error on slab_sysfs_init

[Christoph Lameter]

On Wed, 18 Jun 2014, David Rientjes wrote:

> 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 <cl@linux.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 RESEND] slub: return correct error on slab_sysfs_init

[Andrew Morton]

On Thu, 19 Jun 2014 09:39:54 -0500 (CDT) Christoph Lameter <cl@gentwo.org> wrote:

> On Wed, 18 Jun 2014, David Rientjes wrote:
> 
> > 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.





--
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 RESEND] slub: return correct error on slab_sysfs_init

[David Rientjes]

On Thu, 19 Jun 2014, Andrew Morton wrote:

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

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.

--
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 RESEND] slub: return correct error on slab_sysfs_init

[Jeff Liu]

On 06/20/2014 04:32 AM, Andrew Morton wrote:
> On Thu, 19 Jun 2014 09:39:54 -0500 (CDT) Christoph Lameter <cl@gentwo.org> wrote:
> 
>> On Wed, 18 Jun 2014, David Rientjes wrote:
>>
>>> 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.

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





  

--
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 RESEND] slub: return correct error on slab_sysfs_init

[David Rientjes]

On Fri, 20 Jun 2014, Jeff Liu wrote:

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

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

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

--
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 RESEND] slub: return correct error on slab_sysfs_init

[Jeff Liu]

On 06/21/2014 06:30 AM, David Rientjes wrote:
> On Fri, 20 Jun 2014, Jeff Liu wrote:
> 
>> 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.

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().

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

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

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

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

--
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 RESEND] slub: return correct error on slab_sysfs_init

[Christoph Lameter]

On Sat, 21 Jun 2014, Jeff Liu wrote:

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

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


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