Re: [PATCH bpf-next v2] bpf: Allow compiler to inline most of bpf_local_storage_lookup()

[Marco Elver]

On Thu, 8 Feb 2024 at 00:58, Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> On 2/7/24 4:26 AM, Marco Elver wrote:
> > In various performance profiles of kernels with BPF programs attached,
> > bpf_local_storage_lookup() appears as a significant portion of CPU
> > cycles spent. To enable the compiler generate more optimal code, turn
> > bpf_local_storage_lookup() into a static inline function, where only the
> > cache insertion code path is outlined
> >
> > Notably, outlining cache insertion helps avoid bloating callers by
> > duplicating setting up calls to raw_spin_{lock,unlock}_irqsave() (on
> > architectures which do not inline spin_lock/unlock, such as x86), which
> > would cause the compiler produce worse code by deciding to outline
> > otherwise inlinable functions. The call overhead is neutral, because we
> > make 2 calls either way: either calling raw_spin_lock_irqsave() and
> > raw_spin_unlock_irqsave(); or call __bpf_local_storage_insert_cache(),
> > which calls raw_spin_lock_irqsave(), followed by a tail-call to
> > raw_spin_unlock_irqsave() where the compiler can perform TCO and (in
> > optimized uninstrumented builds) turns it into a plain jump. The call to
> > __bpf_local_storage_insert_cache() can be elided entirely if
> > cacheit_lockit is a false constant expression.
> >
> > Based on results from './benchs/run_bench_local_storage.sh' (21 trials,
> > reboot between each trial; x86 defconfig + BPF, clang 16) this produces
> > improvements in throughput and latency in the majority of cases, with an
> > average (geomean) improvement of 8%:
[...]
> >   include/linux/bpf_local_storage.h             | 30 ++++++++++-
> >   kernel/bpf/bpf_local_storage.c                | 52 +++++--------------
> >   .../bpf/prog_tests/task_local_storage.c       |  6 ---
> >   .../selftests/bpf/progs/cgrp_ls_recursion.c   | 26 ----------
> >   .../selftests/bpf/progs/task_ls_recursion.c   | 17 ------
> >   5 files changed, 41 insertions(+), 90 deletions(-)
> >
> > diff --git a/include/linux/bpf_local_storage.h b/include/linux/bpf_local_storage.h
> > index 173ec7f43ed1..dcddb0aef7d8 100644
> > --- a/include/linux/bpf_local_storage.h
> > +++ b/include/linux/bpf_local_storage.h
> > @@ -129,10 +129,36 @@ bpf_local_storage_map_alloc(union bpf_attr *attr,
> >                           struct bpf_local_storage_cache *cache,
> >                           bool bpf_ma);
> >
> > -struct bpf_local_storage_data *
> > +void __bpf_local_storage_insert_cache(struct bpf_local_storage *local_storage,
> > +                                   struct bpf_local_storage_map *smap,
> > +                                   struct bpf_local_storage_elem *selem);
> > +/* If cacheit_lockit is false, this lookup function is lockless */
> > +static inline struct bpf_local_storage_data *
> >   bpf_local_storage_lookup(struct bpf_local_storage *local_storage,
> >                        struct bpf_local_storage_map *smap,
> > -                      bool cacheit_lockit);
> > +                      bool cacheit_lockit)
> > +{
> > +     struct bpf_local_storage_data *sdata;
> > +     struct bpf_local_storage_elem *selem;
> > +
> > +     /* Fast path (cache hit) */
> > +     sdata = rcu_dereference_check(local_storage->cache[smap->cache_idx],
> > +                                   bpf_rcu_lock_held());
> > +     if (sdata && rcu_access_pointer(sdata->smap) == smap)
> > +             return sdata;
>
> I think we should focus on fast path (your v1 patch)
> and I suppose most production environments
> want to hit fast path in most times. In your production environment did
> you see more than 16 local storage maps per entity (task/sk/inode)?

I think having more than 16 local storage maps isn't entirely unlikely
as eBPF usage grows. But at the moment, it should be rare.

> In the fast path, the memory accesses are
>    two from local_storage->cache[smap->cache_idx] and
>    one from sdata->smap
>
>
> > +
> > +     /* Slow path (cache miss) */
> > +     hlist_for_each_entry_rcu(selem, &local_storage->list, snode,
> > +                               rcu_read_lock_trace_held())
> > +             if (rcu_access_pointer(SDATA(selem)->smap) == smap)
> > +                     break;
>
> But if we reach slow path here which means we have more than 16 local
> storage maps, then traversing the list and getting SDATA(selem)->smap
> will be very expensive, in addition to memory accesses in fast path.
>
> I suppose here we mostly care about socket local storage since it is
> totally possible for a production workload to have millions of sockets.
> To improve performance, fast path should hit in most cases.
> If there are too many sk local storage maps, some kind of sharing
> can be done so multiple applications might be using a single sk
> local storage.
>
> Your above inlining/outlining analysis also show how tricky it is
> for compilation optimization. Without profiling, it is totally
> possible that compiler might do optimization differently in
> the future.

Sure, but it's usually the case that we have to help the compiler a
little to produce more optimal code - if the compiler becomes stupid
in future, we need either fix the compiler or help it some more.

> So here is my suggestion, let us do inlining
> for fast path and focus on performance of fast path.

The slow-path (iterate list w/o cache insertion) is still relatively
small (it's a pointer-chasing loop and a compare), and I decided that
it can be justified inlining it. Martin asked in v1 why there were
slowdowns above 16 local maps, and I analyzed, and concluded that
inlining most is needed to fix and does not hurt performance: in fact,
the current version is better than v1 in all cases (even for 16 maps
or below).

Let me know which version you prefer, and I'll change it. However,
based on the results, I would prefer the current version.

Thanks,
-- Marco