Re: [PATCH bpf-next v2 1/2] bpf: include verifier memory allocations in memcg statistics

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2025-06-12 at 17:53 -0700, Alexei Starovoitov wrote:
> On Thu, Jun 12, 2025 at 5:18 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > 
> > On Thu, Jun 12, 2025 at 5:15 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > > 
> > > On Thu, 2025-06-12 at 17:05 -0700, Andrii Nakryiko wrote:
> > > 
> > > [...]
> > > 
> > > > We have a bunch of GFP_USER allocs as well, e.g. for instruction
> > > > history and state hashmap. At least the former is very much
> > > > interesting, so should we add __GFP_ACCOUNT to those as well?
> > > 
> > > Thank you for pointing this out.
> > > GFP_USER allocations are in 4 places in verifier.c:
> > > 1. copy of state->jmp_history in copy_verifier_state
> > > 2. realloc of state->jmp_history in push_jmp_history
> > > 3. allocation of struct bpf_prog for every subprogram in jit_subprograms
> > > 4. env->explored_states fixed size array of list heads in bpf_check
> > > 
> > > GFP_USER is not used in btf.c and log.c.
> > > 
> > > Is there any reason to keep 1-4 as GFP_USER?
> > > From gfp_types.h:
> > > 
> > >   * %GFP_USER is for userspace allocations that also need to be directly
> > >   * accessibly by the kernel or hardware. It is typically used by hardware
> > >   * for buffers that are mapped to userspace (e.g. graphics) that hardware
> > >   * still must DMA to. cpuset limits are enforced for these allocations. a
> > > 
> > > I assume for (3) this might be used for programs offloading (?),
> > > but 1,2,4 are internal to verifier.
> > > 
> > > Wdyt?
> > 
> > Alexei might remember more details, but I think the thinking was that
> > all these allocations are user-induced based on specific BPF program
> > code, so at some point we were marking them as GFP_USER. But clearly
> > this is inconsistent, so perhaps just unifying to GFP_KERNEL_ACCOUNT
> > is a better way forward?
> 
> Beetlejuice.
> 1,2,4 can be converted to GFP_KERNEL_ACCOUNT,
> since it's a temp memory for the purpose of verification.
> 3 should probably stay as GFP_USER, since it's a long term memory.
> GFP_USER is more restrictive than GFP_KERNEL, since
> it requires memory to be within cpuset limits set for the current task.
> The pages allocated for user space needs should be GFP_USER.
> One can argue that bpf prog is not accessed by any user task
> and prog itself is more like kernel module, so GFP_KERNEL is fine,
> but it will require a bigger code audit.

Thank you, I've converted 1,2,4.
For 3 I'd avoid accounting same way I avoided it in jit, assuming that
this memory does not belong to cgroup, but to some common kernel pool.

Looks maximal memory consumption did not change by much compared to
measurements for SCC series, top consumers for sched_ext:

Program                          Peak memory (MiB)
-------------------------------  -----------------
lavd_select_cpu                                 42
lavd_enqueue                                    41
lavd_dispatch                                   28
layered_dispatch                                28
layered_enqueue                                 12

Top consumers for selftests:

File                     Program                         Peak memory (MiB)
-----------------------  ------------------------------  -----------------
pyperf600.bpf.o          on_event                                      267
pyperf180.bpf.o          on_event                                      213
strobemeta.bpf.o         on_event                                      199
verifier_loops1.bpf.o    loop_after_a_conditional_jump                 128
pyperf100.bpf.o          on_event                                      118