On 08/23, Alexei Starovoitov wrote:

On Mon, Aug 23, 2021 at 05:52:50PM -0400, Hans Montero wrote:

quoted

From: Hans Montero <redacted>

This patch set adds a BPF local storage optimization. The first patch  

adds the

quoted

feature, and the second patch extends the bpf selftests so that the  

feature is

quoted

tested.

We are running BPF programs for each egress packet and noticed that
bpf_sk_storage_get incurs a significant amount of cpu time. By inlining  

the

quoted

storage into struct sock and accessing that instead of performing a map  

lookup,

quoted

we expect to reduce overhead for our specific use-case.

Looks like a hack to me. Please share the perf numbers and setup details.
I think there should be a different way to address performance concerns
without going into such hacks.

What kind of perf numbers would you like to see? What we see here is
that bpf_sk_storage_get() cycles are somewhere on par with hashtable
lookups (we've moved off of 5-tuple ht lookup to sk_storage). Looking
at the code, it seems it's mostly coming from the following:

   sk_storage = rcu_dereference(sk->sk_bpf_storage);
   sdata = rcu_dereference(local_storage->cache[smap->cache_idx]);
   return sdata->data

We do 3 cold-cache references :-( This is where the idea of inlining
something in the socket itself came from. The RFC is just to present
the case and discuss. I was thinking about doing some kind of
inlining at runtime (and fallback to non-inlined case) but wanted
to start with discussing this compile-time option first.

We can also try to save sdata somewhere in the socket to avoid two
lookups for the cached case, this can potentially save us two  
rcu_dereference's.
Is that something that looks acceptable? I was wondering whether you've
considered any socket storage optimizations on your side?

I can try to set up some office hours to discuss in person if that's
preferred.

quoted

This also has a
side-effect of persisting the socket storage, which can be beneficial.

Without explicit opt-in such sharing will cause multiple bpf progs to  
corrupt
each other data.

New BPF_F_SHARED_LOCAL_STORAGE flag is here to provide this opt-in.