Re: [PATCH -mm] mm: percpu: fix incorrect size in pcpu_obj_full_size()

[Dennis Zhou <dennis@kernel.org>]

Hello,

On Fri, Feb 10, 2023 at 02:05:08PM -0800, Andrew Morton wrote:
> On Fri, 10 Feb 2023 15:49:47 +0000 Yafang Shao <laoar.shao@gmail.com> wrote:
> 
> > The extra space which is used to store the obj_cgroup membership is only
> > valid when kmemcg is enabled. The kmemcg can be disabled via the kernel
> > parameter "cgroup.memory=nokmem" at runtime.
> > This helper is also used in non-memcg code, for example the tracepoint,
> > so we should fix it.
> > 
> > It was found by code review when I was implementing bpf memory usage[1].
> > No real issue happens in production environment.
> > 
> > ...
> >
> > --- a/mm/percpu-internal.h
> > +++ b/mm/percpu-internal.h
> > @@ -4,6 +4,7 @@
> >  
> >  #include <linux/types.h>
> >  #include <linux/percpu.h>
> > +#include <linux/memcontrol.h>
> >  
> >  /*
> >   * pcpu_block_md is the metadata block struct.
> > @@ -125,7 +126,8 @@ static inline size_t pcpu_obj_full_size(size_t size)
> >  	size_t extra_size = 0;
> >  
> >  #ifdef CONFIG_MEMCG_KMEM
> > -	extra_size += size / PCPU_MIN_ALLOC_SIZE * sizeof(struct obj_cgroup *);
> > +	if (!mem_cgroup_kmem_disabled())
> > +		extra_size += size / PCPU_MIN_ALLOC_SIZE * sizeof(struct obj_cgroup *);
> >  #endif
> >  
> >  	return size * num_possible_cpus() + extra_size;
> 

Sorry I've been a bit mia...

> Seems risky at the first look - enabling kmemcg at runtime will make
> prior calculations based on pcpu_obj_full_size) incorrect.  But as long
> as this is only used for accounting I guess that's OK.
> 
> What happens if we do a bunch of allocations with kmemcg enabled, then
> disable kmemcg then free those allocations, or some such thing.  Does
> the accounting end up wrong?
> 

For now it works correctly because of 2 things. 1 - the function is only
called by accounting. 2 - the free path doesn't consult
mem_cgroup_kmem_disabled() but consults if a memcg exists for a percpu
allocation. If accounting is enabled, we'd always account the additional
memory for the memcg accounting. If it's not enabled, then percpu is
well unaccounted for.

This function probably needs to be renamed a bit more carefully so it
doesn't bleed outside of mm/percpu.c.

In short, I don't think this change is correct.

> The final sentence in the pcpu_obj_full_size() kerneldoc could do with
> an update - it still implies that the extra_size accounting is
> unconditional.
> 

Thanks,
Dennis