Re: [PATCH v2] cgroup/bpf: fast path for not loaded skb BPF filtering

On 12/14/21 19:14, Martin KaFai Lau wrote:

On Tue, Dec 14, 2021 at 11:40:26AM +0000, Pavel Begunkov wrote:

quoted

On 12/14/21 07:27, Martin KaFai Lau wrote:

quoted

On Sat, Dec 11, 2021 at 07:17:49PM +0000, Pavel Begunkov wrote:

quoted

cgroup_bpf_enabled_key static key guards from overhead in cases where
no cgroup bpf program of a specific type is loaded in any cgroup. Turn
out that's not always good enough, e.g. when there are many cgroups but
ones that we're interesting in are without bpf. It's seen in server
environments, but the problem seems to be even wider as apparently
systemd loads some BPF affecting my laptop.

Profiles for small packet or zerocopy transmissions over fast network
show __cgroup_bpf_run_filter_skb() taking 2-3%, 1% of which is from
migrate_disable/enable(), and similarly on the receiving side. Also
got +4-5% of t-put for local testing.

What is t-put?  throughput?

yes

quoted

Local testing means sending to lo/dummy?

yes, it was dummy specifically

Thanks for confirming.

Please also put these details in the commit log.
I was slow.  With only '%' as a unit, it took me a min to guess
what t-put may mean ;)

I guess requests/s is a more natural metric for net. I anyway going
to resend, will reword it a bit.

quoted

quoted

quoted

+#define CGROUP_BPF_TYPE_ENABLED(sk, atype)				       \

and change cgroup.c to directly use this instead, so
everywhere holding a fullsock sk will use this instead
of having two helpers for empty check.

Why?

As mentioned earlier, prefer to have one way to do the same thing
for checking with a fullsock.

quoted

CGROUP_BPF_TYPE_ENABLED can't be a function atm because of header
dependency hell, and so it'd kill some of typization, which doesn't add
clarity.

I didn't mean to change it to a function.  I actually think,
for the sk context, it should eventually be folded with the existing
cgroup_bpf_enabled() macro because those are the tests to ensure
there is bpf prog to run before proceeding.
Need to audit about the non fullsock case. not sure yet.

btw, would be nice to rewrite helpers as inline functions, but
sock, cgroup, etc. are not defined in bpf-cgroup.h are can't be
included. May make sense e.g. not include bpf-cgroup.h in bpf.h
but to move some definitions like struct cgroup_bpf into
include/linux/cgroup-defs.h.
Though I'd rather leave it to someone with a better grasp on
BPF code base.

quoted

And also it imposes some extra overhead to *sockopt using
the first helper directly.

I think it is unimportant unless it is measurable in normal
use case.

I hope so

quoted

I think it's better with two of them.

Ok. I won't insist.  There are atype that may not have sk, so
a separate inline function for checking emptiness may eventually
be useful there.

-- 
Pavel Begunkov