On Tue, Aug 24, 2021 at 09:03:20AM -0700, sdf@google.com wrote:

On 08/23, Alexei Starovoitov wrote:

quoted

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.

quoted

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 :-( 

Only if the prog doesn't read anything at all through 'sk' pointer,
but sounds like the bpf logic is accessing it, so for a system with millions
of sockets the first access to 'sk' will pay that penalty.
I suspect if you rearrange bpf prog to do sk->foo first the cache
miss will move and bpf_sk_storage_get() won't be hot anymore.
That's why optimizing loads like this without considering the full
picture might not achieve the desired result at the end.

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? 

Not until there is clear 'perf report' where issue is obvious.

I was wondering whether you've
considered any socket storage optimizations on your side?

Quite the opposite. We've refactored several bpf progs to use
local storage instead of hash maps and achieved nice perf wins.

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

Indeed, it's probably the best do discuss it on a call.