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

[Axel Rasmussen <axelrasmussen@google.com>]

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.