On Thu, Dec 16, 2021 at 10:14 AM Martin KaFai Lau <kafai@xxxxxx> wrote:
On Thu, Dec 16, 2021 at 01:21:26PM +0000, Pavel Begunkov wrote:
On 12/15/21 22:07, Stanislav Fomichev wrote:I also suggested to try to stay with one way for fullsock context in v2
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.
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!
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.