Re: [PATCH bpf-next v5 3/3] bpf: Cache build IDs in sleepable stackmap path

[Andrii Nakryiko]

On Thu, May 14, 2026 at 5:53 PM Ihor Solodrai <ihor.solodrai@xxxxxxxxx> wrote:
>
> Stack traces often contain adjacent IPs from the same VMA or from
> different VMAs backed by the same ELF file. Cache the last successfully
> parsed build ID together with the resolved VMA range and backing file
> so the sleepable build-ID path can avoid repeated VMA locking and file
> parsing in common cases.
>
> Suggested-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> Acked-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> Signed-off-by: Ihor Solodrai <ihor.solodrai@xxxxxxxxx>
> ---
>  kernel/bpf/stackmap.c | 56 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 08f7659505d1..7336fd55c856 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -230,13 +230,33 @@ static void stack_map_get_build_id_offset_sleepable(struct bpf_stack_build_id *i
>                 .vma = NULL,
>                 .mm = mm,
>         };
> -       unsigned long vm_pgoff, vm_start;
> +       struct {
> +               struct file *file;
> +               const unsigned char *build_id;
> +               unsigned long vm_start;
> +               unsigned long vm_end;
> +               unsigned long vm_pgoff;
> +       } cache = {};
> +       unsigned long vm_pgoff, vm_start, vm_end;
>         struct vm_area_struct *vma;
>         struct file *file;
>         u64 ip;
>
>         for (u32 i = 0; i < trace_nr; i++) {
>                 ip = READ_ONCE(id_offs[i].ip);
> +
> +               /*
> +                * Range cache fast path: if ip falls within the previously
> +                * resolved VMA range, reuse the cache build_id without
> +                * re-acquiring the VMA lock.
> +                */
> +               if (cache.build_id && ip >= cache.vm_start && ip < cache.vm_end) {
> +                       vm_start = cache.vm_start;
> +                       vm_end = cache.vm_end;
> +                       vm_pgoff = cache.vm_pgoff;

isn't this unnecessarily convoluted way of doing things? If we are
here, we know for sure a) how to calculate offset and b) we need to
memcpy. So just do that here and just then maybe jump all the way to
id_offs[i].offset setting. Or just fill offset, status and memcpy
build id here and continue.

> +                       goto build_id_valid;
> +               }
> +
>                 vma = stack_map_lock_vma(&lock, ip);
>                 if (!vma || !vma->vm_file) {
>                         stack_map_build_id_set_ip(&id_offs[i]);
> @@ -244,9 +264,21 @@ static void stack_map_get_build_id_offset_sleepable(struct bpf_stack_build_id *i
>                         continue;
>                 }
>
> -               file = get_file(vma->vm_file);
> +               file = vma->vm_file;
>                 vm_pgoff = vma->vm_pgoff;
>                 vm_start = vma->vm_start;
> +               vm_end = vma->vm_end;
> +
> +               if (file == cache.file) {
> +                       /*
> +                        * Same backing file as previous (e.g. different VMAs
> +                        * of the same ELF binary). Reuse the cache build_id.
> +                        */
> +                       stack_map_unlock_vma(&lock);
> +                       goto build_id_valid;
> +               }

same thing here, file matched, so memcpy(build_id), calculate correct
offset, set offset and status, unlock and continue. This logic is
pretty straightforward, IMO it's cleaner to copy-paste these 4 lines
than dealing with the goto maze...


> +
> +               file = get_file(file);
>                 stack_map_unlock_vma(&lock);
>
>                 /* build_id_parse_file() may block on filesystem reads */
> @@ -255,11 +287,29 @@ static void stack_map_get_build_id_offset_sleepable(struct bpf_stack_build_id *i
>                         fput(file);
>                         continue;
>                 }
> -               fput(file);
>
> +               if (cache.file)
> +                       fput(cache.file);
> +               cache.file = file;
> +               cache.build_id = id_offs[i].build_id;
> +
> +build_id_valid:
> +               /*
> +                * In the slow path cache.build_id points to id_offs[i].build_id.
> +                * Cache hits leave cache.build_id pointing at a prior slot,
> +                * triggering the memcpy here.
> +                */
> +               if (cache.build_id != id_offs[i].build_id)
> +                       memcpy(id_offs[i].build_id, cache.build_id, BUILD_ID_SIZE_MAX);
> +               cache.vm_start = vm_start;
> +               cache.vm_end = vm_end;
> +               cache.vm_pgoff = vm_pgoff;
>                 id_offs[i].offset = (vm_pgoff << PAGE_SHIFT) + ip - vm_start;
>                 id_offs[i].status = BPF_STACK_BUILD_ID_VALID;
>         }
> +
> +       if (cache.file)
> +               fput(cache.file);
>  }
>
>  /*
> --
> 2.54.0
>