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

[Andrew Morton <akpm@linux-foundation.org>]

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;

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?

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.