* [PATCH] slub: Don't panic for memcg kmem cache creation failure
@ 2019-06-19 23:25 Shakeel Butt
2019-06-20 5:50 ` Michal Hocko
2019-06-21 20:52 ` David Rientjes
0 siblings, 2 replies; 6+ messages in thread
From: Shakeel Butt @ 2019-06-19 23:25 UTC (permalink / raw)
To: Johannes Weiner, Christoph Lameter, Michal Hocko, Andrew Morton,
Roman Gushchin, Pekka Enberg, David Rientjes, Joonsoo Kim
Cc: cgroups, linux-mm, linux-kernel, Shakeel Butt, Dave Hansen
Currently for CONFIG_SLUB, if a memcg kmem cache creation is failed and
the corresponding root kmem cache has SLAB_PANIC flag, the kernel will
be crashed. This is unnecessary as the kernel can handle the creation
failures of memcg kmem caches. Additionally CONFIG_SLAB does not
implement this behavior. So, to keep the behavior consistent between
SLAB and SLUB, removing the panic for memcg kmem cache creation
failures. The root kmem cache creation failure for SLAB_PANIC correctly
panics for both SLAB and SLUB.
Reported-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Shakeel Butt <shakeelb@google.com>
---
mm/slub.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/mm/slub.c b/mm/slub.c
index 6a5174b51cd6..84c6508e360d 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3640,10 +3640,6 @@ static int kmem_cache_open(struct kmem_cache *s, slab_flags_t flags)
free_kmem_cache_nodes(s);
error:
- if (flags & SLAB_PANIC)
- panic("Cannot create slab %s size=%u realsize=%u order=%u offset=%u flags=%lx\n",
- s->name, s->size, s->size,
- oo_order(s->oo), s->offset, (unsigned long)flags);
return -EINVAL;
}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] slub: Don't panic for memcg kmem cache creation failure
2019-06-19 23:25 [PATCH] slub: Don't panic for memcg kmem cache creation failure Shakeel Butt
@ 2019-06-20 5:50 ` Michal Hocko
2019-06-20 14:44 ` Shakeel Butt
2019-06-21 20:52 ` David Rientjes
1 sibling, 1 reply; 6+ messages in thread
From: Michal Hocko @ 2019-06-20 5:50 UTC (permalink / raw)
To: Shakeel Butt
Cc: Johannes Weiner, Christoph Lameter, Andrew Morton,
Roman Gushchin, Pekka Enberg, David Rientjes, Joonsoo Kim,
cgroups, linux-mm, linux-kernel, Dave Hansen
On Wed 19-06-19 16:25:14, Shakeel Butt wrote:
> Currently for CONFIG_SLUB, if a memcg kmem cache creation is failed and
> the corresponding root kmem cache has SLAB_PANIC flag, the kernel will
> be crashed. This is unnecessary as the kernel can handle the creation
> failures of memcg kmem caches.
AFAICS it will handle those by simply not accounting those objects
right?
> Additionally CONFIG_SLAB does not
> implement this behavior. So, to keep the behavior consistent between
> SLAB and SLUB, removing the panic for memcg kmem cache creation
> failures. The root kmem cache creation failure for SLAB_PANIC correctly
> panics for both SLAB and SLUB.
I do agree that panicing is really dubious especially because it opens
doors to shut the system down from a restricted environment. So the
patch makes sesne to me.
I am wondering whether SLAB_PANIC makes sense in general though. Why is
it any different from any other essential early allocations? We tend to
not care about allocation failures for those on bases that the system
must be in a broken state to fail that early already. Do you think it is
time to remove SLAB_PANIC altogether?
> Reported-by: Dave Hansen <dave.hansen@intel.com>
> Signed-off-by: Shakeel Butt <shakeelb@google.com>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/slub.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 6a5174b51cd6..84c6508e360d 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3640,10 +3640,6 @@ static int kmem_cache_open(struct kmem_cache *s, slab_flags_t flags)
>
> free_kmem_cache_nodes(s);
> error:
> - if (flags & SLAB_PANIC)
> - panic("Cannot create slab %s size=%u realsize=%u order=%u offset=%u flags=%lx\n",
> - s->name, s->size, s->size,
> - oo_order(s->oo), s->offset, (unsigned long)flags);
> return -EINVAL;
> }
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] slub: Don't panic for memcg kmem cache creation failure
2019-06-20 5:50 ` Michal Hocko
@ 2019-06-20 14:44 ` Shakeel Butt
2019-06-20 14:51 ` Dave Hansen
2019-06-20 15:35 ` Michal Hocko
0 siblings, 2 replies; 6+ messages in thread
From: Shakeel Butt @ 2019-06-20 14:44 UTC (permalink / raw)
To: Michal Hocko
Cc: Johannes Weiner, Christoph Lameter, Andrew Morton,
Roman Gushchin, Pekka Enberg, David Rientjes, Joonsoo Kim,
Cgroups, Linux MM, LKML, Dave Hansen
On Wed, Jun 19, 2019 at 10:50 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Wed 19-06-19 16:25:14, Shakeel Butt wrote:
> > Currently for CONFIG_SLUB, if a memcg kmem cache creation is failed and
> > the corresponding root kmem cache has SLAB_PANIC flag, the kernel will
> > be crashed. This is unnecessary as the kernel can handle the creation
> > failures of memcg kmem caches.
>
> AFAICS it will handle those by simply not accounting those objects
> right?
>
The memcg kmem cache creation is async. The allocation has already
been decided not to be accounted on creation trigger. If memcg kmem
cache creation is failed, it will fail silently and the next
allocation will trigger the creation process again.
> > Additionally CONFIG_SLAB does not
> > implement this behavior. So, to keep the behavior consistent between
> > SLAB and SLUB, removing the panic for memcg kmem cache creation
> > failures. The root kmem cache creation failure for SLAB_PANIC correctly
> > panics for both SLAB and SLUB.
>
> I do agree that panicing is really dubious especially because it opens
> doors to shut the system down from a restricted environment. So the
> patch makes sesne to me.
>
> I am wondering whether SLAB_PANIC makes sense in general though. Why is
> it any different from any other essential early allocations? We tend to
> not care about allocation failures for those on bases that the system
> must be in a broken state to fail that early already. Do you think it is
> time to remove SLAB_PANIC altogether?
>
That would need some investigation into the history of SLAB_PANIC. I
will look into it.
> > Reported-by: Dave Hansen <dave.hansen@intel.com>
> > Signed-off-by: Shakeel Butt <shakeelb@google.com>
>
> Acked-by: Michal Hocko <mhocko@suse.com>
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] slub: Don't panic for memcg kmem cache creation failure
2019-06-20 14:44 ` Shakeel Butt
@ 2019-06-20 14:51 ` Dave Hansen
2019-06-20 15:35 ` Michal Hocko
1 sibling, 0 replies; 6+ messages in thread
From: Dave Hansen @ 2019-06-20 14:51 UTC (permalink / raw)
To: Shakeel Butt, Michal Hocko
Cc: Johannes Weiner, Christoph Lameter, Andrew Morton,
Roman Gushchin, Pekka Enberg, David Rientjes, Joonsoo Kim,
Cgroups, Linux MM, LKML
On 6/20/19 7:44 AM, Shakeel Butt wrote:
>> I am wondering whether SLAB_PANIC makes sense in general though. Why is
>> it any different from any other essential early allocations? We tend to
>> not care about allocation failures for those on bases that the system
>> must be in a broken state to fail that early already. Do you think it is
>> time to remove SLAB_PANIC altogether?
>>
> That would need some investigation into the history of SLAB_PANIC. I
> will look into it.
I think it still makes sense for things like the vma, filp, dentry
caches. If we don't
have those, we can't even execve("/sbin/init") so we shouldn't even bother
continuing to boot.
Maybe we should turn off SLAB_PANIC behavior after boot. We don't want
a silly driver or filesystem module that's creating slabs to be causing
panic()s.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] slub: Don't panic for memcg kmem cache creation failure
2019-06-20 14:44 ` Shakeel Butt
2019-06-20 14:51 ` Dave Hansen
@ 2019-06-20 15:35 ` Michal Hocko
1 sibling, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2019-06-20 15:35 UTC (permalink / raw)
To: Shakeel Butt
Cc: Johannes Weiner, Christoph Lameter, Andrew Morton,
Roman Gushchin, Pekka Enberg, David Rientjes, Joonsoo Kim,
Cgroups, Linux MM, LKML, Dave Hansen
On Thu 20-06-19 07:44:27, Shakeel Butt wrote:
> On Wed, Jun 19, 2019 at 10:50 PM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Wed 19-06-19 16:25:14, Shakeel Butt wrote:
> > > Currently for CONFIG_SLUB, if a memcg kmem cache creation is failed and
> > > the corresponding root kmem cache has SLAB_PANIC flag, the kernel will
> > > be crashed. This is unnecessary as the kernel can handle the creation
> > > failures of memcg kmem caches.
> >
> > AFAICS it will handle those by simply not accounting those objects
> > right?
> >
>
> The memcg kmem cache creation is async. The allocation has already
> been decided not to be accounted on creation trigger. If memcg kmem
> cache creation is failed, it will fail silently and the next
> allocation will trigger the creation process again.
Ohh, right I forgot that it will get retried. This would be useful to
mention in the changelog as it is not straightforward from reading just
the particular function.
> > > Additionally CONFIG_SLAB does not
> > > implement this behavior. So, to keep the behavior consistent between
> > > SLAB and SLUB, removing the panic for memcg kmem cache creation
> > > failures. The root kmem cache creation failure for SLAB_PANIC correctly
> > > panics for both SLAB and SLUB.
> >
> > I do agree that panicing is really dubious especially because it opens
> > doors to shut the system down from a restricted environment. So the
> > patch makes sesne to me.
> >
> > I am wondering whether SLAB_PANIC makes sense in general though. Why is
> > it any different from any other essential early allocations? We tend to
> > not care about allocation failures for those on bases that the system
> > must be in a broken state to fail that early already. Do you think it is
> > time to remove SLAB_PANIC altogether?
> >
>
> That would need some investigation into the history of SLAB_PANIC. I
> will look into it.
Well, I strongly suspect this is a relict from the past. I have hard
time to believe that the system would get to a usable state if many of
those caches would fail to allocate. And as Dave said in his reply it is
quite silly to give this weapon to a random driver hands. Everybody just
thinks his toy is the most important one...
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] slub: Don't panic for memcg kmem cache creation failure
2019-06-19 23:25 [PATCH] slub: Don't panic for memcg kmem cache creation failure Shakeel Butt
2019-06-20 5:50 ` Michal Hocko
@ 2019-06-21 20:52 ` David Rientjes
1 sibling, 0 replies; 6+ messages in thread
From: David Rientjes @ 2019-06-21 20:52 UTC (permalink / raw)
To: Shakeel Butt
Cc: Johannes Weiner, Christoph Lameter, Michal Hocko, Andrew Morton,
Roman Gushchin, Pekka Enberg, Joonsoo Kim, cgroups, linux-mm,
linux-kernel, Dave Hansen
On Wed, 19 Jun 2019, Shakeel Butt wrote:
> Currently for CONFIG_SLUB, if a memcg kmem cache creation is failed and
> the corresponding root kmem cache has SLAB_PANIC flag, the kernel will
> be crashed. This is unnecessary as the kernel can handle the creation
> failures of memcg kmem caches. Additionally CONFIG_SLAB does not
> implement this behavior. So, to keep the behavior consistent between
> SLAB and SLUB, removing the panic for memcg kmem cache creation
> failures. The root kmem cache creation failure for SLAB_PANIC correctly
> panics for both SLAB and SLUB.
>
> Reported-by: Dave Hansen <dave.hansen@intel.com>
> Signed-off-by: Shakeel Butt <shakeelb@google.com>
Acked-by: David Rientjes <rientjes@google.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-21 20:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-19 23:25 [PATCH] slub: Don't panic for memcg kmem cache creation failure Shakeel Butt
2019-06-20 5:50 ` Michal Hocko
2019-06-20 14:44 ` Shakeel Butt
2019-06-20 14:51 ` Dave Hansen
2019-06-20 15:35 ` Michal Hocko
2019-06-21 20:52 ` David Rientjes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).