Re: [PATCH bpf-next v5 1/3] bpf: remove extra lock_sock for TCP_ZEROCOPY_RECEIVE

On Fri, Jan 8, 2021 at 8:08 PM Stanislav Fomichev [off-list ref] wrote:

On Fri, Jan 8, 2021 at 10:41 AM Eric Dumazet [off-list ref] wrote:

quoted

On Fri, Jan 8, 2021 at 7:26 PM Stanislav Fomichev [off-list ref] wrote:

quoted

On Fri, Jan 8, 2021 at 10:10 AM Eric Dumazet [off-list ref] wrote:

quoted

On Fri, Jan 8, 2021 at 7:03 PM Stanislav Fomichev [off-list ref] wrote:

quoted

Add custom implementation of getsockopt hook for TCP_ZEROCOPY_RECEIVE.
We skip generic hooks for TCP_ZEROCOPY_RECEIVE and have a custom
call in do_tcp_getsockopt using the on-stack data. This removes
3% overhead for locking/unlocking the socket.

Without this patch:
     3.38%     0.07%  tcp_mmap  [kernel.kallsyms]  [k] __cgroup_bpf_run_filter_getsockopt
            |
             --3.30%--__cgroup_bpf_run_filter_getsockopt
                       |
                        --0.81%--__kmalloc

With the patch applied:
     0.52%     0.12%  tcp_mmap  [kernel.kallsyms]  [k] __cgroup_bpf_run_filter_getsockopt_kern

OK but we are adding yet another indirect call.

Can you add a patch on top of it adding INDIRECT_CALL_INET() avoidance ?

Sure, but do you think it will bring any benefit?

Sure, avoiding an indirect call might be the same gain than the
lock_sock() avoidance :)

quoted

We don't have any indirect avoidance in __sys_getsockopt for the
sock->ops->getsockopt() call.
If we add it for this new bpf_bypass_getsockopt, we might as well add
it for sock->ops->getsockopt?

Well, that is orthogonal to this patch.
As you may know, Google kernels do have a mitigation there already and
Brian may upstream it.

I guess my point here was that if I send it out only for bpf_bypass_getsockopt
it might look a bit strange because the rest of the getsockopt still
suffers the indirect costs.

Each new indirect call adds a cost. If you focus on optimizing
TCP_ZEROCOPY_RECEIVE,
it is counter intuitive adding an expensive indirect call.

 If Brian has plans to upstream the rest, maybe

it's better to upstream everything together with some numbers?
CC'ing him for his opinion.

I am just saying your point about the other indirect call is already taken care.

I'm happy to follow up in whatever form is best. I can also resend
with INDIRECT_CALL_INET2 if there are no objections in including
this version from the start.

INDIRECT_CALL_INET2 seems a strange name to me.