Re: [PATCH v3] cgroup/bpf: fast path skb BPF filtering

On 1/24/22 18:25, Stanislav Fomichev wrote:

On Mon, Jan 24, 2022 at 7:49 AM Pavel Begunkov [off-list ref] wrote:

quoted

On 12/16/21 18:24, Stanislav Fomichev wrote:

quoted

On Thu, Dec 16, 2021 at 10:14 AM Martin KaFai Lau [off-list ref] wrote:

quoted

On Thu, Dec 16, 2021 at 01:21:26PM +0000, Pavel Begunkov wrote:

quoted

On 12/15/21 22:07, Stanislav Fomichev wrote:

quoted

quoted

I'm skeptical I'll be able to measure inlining one function,
variability between boots/runs is usually greater and would hide it.

Right, that's why I suggested to mirror what we do in set/getsockopt
instead of the new extra CGROUP_BPF_TYPE_ENABLED. But I'll leave it up
to you, Martin and the rest.

I also suggested to try to stay with one way for fullsock context in v2
but it is for code readability reason.

How about calling CGROUP_BPF_TYPE_ENABLED() just next to cgroup_bpf_enabled()
in BPF_CGROUP_RUN_PROG_*SOCKOPT_*() instead ?

SG!

quoted

It is because both cgroup_bpf_enabled() and CGROUP_BPF_TYPE_ENABLED()
want to check if there is bpf to run before proceeding everything else
and then I don't need to jump to the non-inline function itself to see
if there is other prog array empty check.

Stan, do you have concern on an extra inlined sock_cgroup_ptr()
when there is bpf prog to run for set/getsockopt()?  I think
it should be mostly noise from looking at
__cgroup_bpf_run_filter_*sockopt()?

Yeah, my concern is also mostly about readability/consistency. Either
__cgroup_bpf_prog_array_is_empty everywhere or this new
CGROUP_BPF_TYPE_ENABLED everywhere. I'm slightly leaning towards
__cgroup_bpf_prog_array_is_empty because I don't believe direct
function calls add any visible overhead and macros are ugly :-) But
either way is fine as long as it looks consistent.

Martin, Stanislav, do you think it's good to go? Any other concerns?
It feels it might end with bikeshedding and would be great to finally
get it done, especially since I find the issue to be pretty simple.

I'll leave it up to the bpf maintainers/reviewers. Personally, I'd
still prefer a respin with a consistent
__cgroup_bpf_prog_array_is_empty or CGROUP_BPF_TYPE_ENABLED everywhere
(shouldn't be a lot of effort?)

I can make CGROUP_BPF_TYPE_ENABLED() used everywhere, np.

I'll leave out unification with cgroup_bpf_enabled() as don't
really understand the fullsock dancing in
BPF_CGROUP_RUN_PROG_INET_EGRESS(). Any idea whether it's needed
and/or how to shove it out of inlined checks?

-- 
Pavel Begunkov