Re: [PATCH] mm, slub: print CPU id on slab OOM

Re: [PATCH] mm, slub: print CPU id on slab OOM

[Vlastimil Babka]

On 8/7/24 01:26, Axel Rasmussen wrote:
> Depending on how remote_node_defrag_ratio is configured, allocations can
> end up in this path as a result of the local node being OOM, despite the
> allocation overall being unconstrained (node == -1).
> 
> When we print a warning, printing the current CPU makes that situation
> more clear (i.e., you can immediately see which node's OOM status
> matters for the allocation at hand).
> 
> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>

Added to slab/for-next, thanks.

> ---
>  mm/slub.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index c9d8a2497fd6..7148047998de 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3422,7 +3422,8 @@ slab_out_of_memory(struct kmem_cache *s, gfp_t gfpflags, int nid)
>  	if ((gfpflags & __GFP_NOWARN) || !__ratelimit(&slub_oom_rs))
>  		return;
>  
> -	pr_warn("SLUB: Unable to allocate memory on node %d, gfp=%#x(%pGg)\n",
> +	pr_warn("SLUB: Unable to allocate memory for CPU %u on node %d, gfp=%#x(%pGg)\n",
> +		preemptible() ? raw_smp_processor_id() : smp_processor_id(),
>  		nid, gfpflags, &gfpflags);
>  	pr_warn("  cache: %s, object size: %u, buffer size: %u, default order: %u, min order: %u\n",
>  		s->name, s->object_size, s->size, oo_order(s->oo),

Re: [PATCH] mm, slub: print CPU id on slab OOM

[David Rientjes]

On Tue, 6 Aug 2024, Axel Rasmussen wrote:

> Depending on how remote_node_defrag_ratio is configured, allocations can
> end up in this path as a result of the local node being OOM, despite the
> allocation overall being unconstrained (node == -1).
> 
> When we print a warning, printing the current CPU makes that situation
> more clear (i.e., you can immediately see which node's OOM status
> matters for the allocation at hand).
> 
> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH] mm, slub: print CPU id on slab OOM

[Vlastimil Babka]

On 8/7/24 1:26 AM, Axel Rasmussen wrote:
> Depending on how remote_node_defrag_ratio is configured, allocations can
> end up in this path as a result of the local node being OOM, despite the
> allocation overall being unconstrained (node == -1).
> 
> When we print a warning, printing the current CPU makes that situation
> more clear (i.e., you can immediately see which node's OOM status
> matters for the allocation at hand).
> 
> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
> ---
>  mm/slub.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index c9d8a2497fd6..7148047998de 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3422,7 +3422,8 @@ slab_out_of_memory(struct kmem_cache *s, gfp_t gfpflags, int nid)
>  	if ((gfpflags & __GFP_NOWARN) || !__ratelimit(&slub_oom_rs))
>  		return;
>  
> -	pr_warn("SLUB: Unable to allocate memory on node %d, gfp=%#x(%pGg)\n",
> +	pr_warn("SLUB: Unable to allocate memory for CPU %u on node %d, gfp=%#x(%pGg)\n",

BTW, wouldn't "on CPU" be more correct, as "for CPU" might be misleading
that we are somehow constrained to that CPU?

> +		preemptible() ? raw_smp_processor_id() : smp_processor_id(),

Also could we just use raw_smp_processor_id() always here? I don't see
this has any advantage or am I missing something?

>  		nid, gfpflags, &gfpflags);
>  	pr_warn("  cache: %s, object size: %u, buffer size: %u, default order: %u, min order: %u\n",
>  		s->name, s->object_size, s->size, oo_order(s->oo),

Re: [PATCH] mm, slub: print CPU id on slab OOM

[David Rientjes]

On Sun, 11 Aug 2024, Vlastimil Babka wrote:

> > diff --git a/mm/slub.c b/mm/slub.c
> > index c9d8a2497fd6..7148047998de 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -3422,7 +3422,8 @@ slab_out_of_memory(struct kmem_cache *s, gfp_t gfpflags, int nid)
> >  	if ((gfpflags & __GFP_NOWARN) || !__ratelimit(&slub_oom_rs))
> >  		return;
> >  
> > -	pr_warn("SLUB: Unable to allocate memory on node %d, gfp=%#x(%pGg)\n",
> > +	pr_warn("SLUB: Unable to allocate memory for CPU %u on node %d, gfp=%#x(%pGg)\n",
> 
> BTW, wouldn't "on CPU" be more correct, as "for CPU" might be misleading
> that we are somehow constrained to that CPU?
> 

Agreed.

When I suggested this patch, I was trying to ascertain whether something 
was really wonky based on some logs that we were seeing.

  node 0: slabs: 223, objs: 11819, free: 0
  node 1: slabs: 951, objs: 50262, free: 218

This is for a NUMA_NO_NODE allocation, so I wanted to know if the cpu was 
on node 0 or node 1.

Even with the patch, that requires knowing the cpu-to-node mapping.  If we 
add the CPU output here, we likely also want to print out cpu_to_node().

> > +		preemptible() ? raw_smp_processor_id() : smp_processor_id(),
> 
> Also could we just use raw_smp_processor_id() always here? I don't see
> this has any advantage or am I missing something?
> 

This matches my understanding as well.

Re: [PATCH] mm, slub: print CPU id on slab OOM

[Axel Rasmussen]

On Sun, Aug 11, 2024 at 1:21 PM David Rientjes <rientjes@google.com> wrote:
>
> On Sun, 11 Aug 2024, Vlastimil Babka wrote:
>
> > > diff --git a/mm/slub.c b/mm/slub.c
> > > index c9d8a2497fd6..7148047998de 100644
> > > --- a/mm/slub.c
> > > +++ b/mm/slub.c
> > > @@ -3422,7 +3422,8 @@ slab_out_of_memory(struct kmem_cache *s, gfp_t gfpflags, int nid)
> > >     if ((gfpflags & __GFP_NOWARN) || !__ratelimit(&slub_oom_rs))
> > >             return;
> > >
> > > -   pr_warn("SLUB: Unable to allocate memory on node %d, gfp=%#x(%pGg)\n",
> > > +   pr_warn("SLUB: Unable to allocate memory for CPU %u on node %d, gfp=%#x(%pGg)\n",
> >
> > BTW, wouldn't "on CPU" be more correct, as "for CPU" might be misleading
> > that we are somehow constrained to that CPU?
> >
>
> Agreed.

No objection to the rewording.

>
> When I suggested this patch, I was trying to ascertain whether something
> was really wonky based on some logs that we were seeing.
>
>   node 0: slabs: 223, objs: 11819, free: 0
>   node 1: slabs: 951, objs: 50262, free: 218
>
> This is for a NUMA_NO_NODE allocation, so I wanted to know if the cpu was
> on node 0 or node 1.
>
> Even with the patch, that requires knowing the cpu-to-node mapping.  If we
> add the CPU output here, we likely also want to print out cpu_to_node().

Seems reasonable. Of course we could always look it up separately, but
it would be convenient to just print it directly. I can send a v2 to
add this.

>
> > > +           preemptible() ? raw_smp_processor_id() : smp_processor_id(),
> >
> > Also could we just use raw_smp_processor_id() always here? I don't see
> > this has any advantage or am I missing something?
> >
>
> This matches my understanding as well.

That's fair, in any case it seems to matter very little for this use
case whether the read is "stable" or not. Better to keep it simple. I
can send a v2 with this tweak too.