On Wed, Jan 06, 2021 at 02:45:56PM -0800, sdf@xxxxxxxxxx wrote:
> On 01/06, Martin KaFai Lau wrote:
> > On Tue, Jan 05, 2021 at 01:43:50PM -0800, Stanislav Fomichev wrote:
> > > 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.
> > >
> > > Also:
> > > - Removed BUILD_BUG_ON (zerocopy doesn't depend on the buf size anymore)
> > > - Separated on-stack buffer into bpf_sockopt_buf and downsized to 32
> > bytes
> > > (let's keep it to help with the other options)
> > >
> > > (I can probably split this patch into two: add new features and rework
> > > bpf_sockopt_buf; can follow up if the approach in general sounds
> > > good).
> > >
> > > Without this patch:
> > > 3.29% 0.07% tcp_mmap [kernel.kallsyms] [k]
> > __cgroup_bpf_run_filter_getsockopt
> > > |
> > > --3.22%--__cgroup_bpf_run_filter_getsockopt
> > > |
> > > |--0.66%--lock_sock_nested
> > A general question for sockopt prog, why the BPF_CGROUP_(GET|SET)SOCKOPT
> > prog
> > has to run under lock_sock()?
> I don't think there is a strong reason. We expose sk to the BPF program,
> but mainly for the socket storage map (which, afaik, doesn't require
> socket to be locked). OTOH, it seems that providing a consistent view
> of the sk to the BPF is a good idea.
hmm... most of the bpf prog also does not require a locked sock. For
example, the __sk_buff->sk. If a bpf prog needs a locked view of sk,
a more generic solution is desired. Anyhow, I guess the train has sort
of sailed for sockopt bpf.
>
> Eric has suggested to try to use fast socket lock. It helps a bit,
> but it doesn't remove the issue completely because
> we do a bunch of copy_{to,from}_user in the generic
> __cgroup_bpf_run_filter_getsockopt as well :-(
>
> > > |
> > > |--0.57%--__might_fault
Is it a debug kernel?
> > > |
> > > --0.56%--release_sock
> > >
> > > With the patch applied:
> > > 0.42% 0.10% tcp_mmap [kernel.kallsyms] [k]
> > __cgroup_bpf_run_filter_getsockopt_kern
> > > 0.02% 0.02% tcp_mmap [kernel.kallsyms] [k]
> > __cgroup_bpf_run_filter_getsockopt
> > >
> > [ ... ]
>
> > > @@ -1445,15 +1442,29 @@ int __cgroup_bpf_run_filter_getsockopt(struct
> > sock *sk, int level,
> > > int __user *optlen, int max_optlen,
> > > int retval)
> > > {
> > > - struct cgroup *cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
> > > - struct bpf_sockopt_kern ctx = {
> > > - .sk = sk,
> > > - .level = level,
> > > - .optname = optname,
> > > - .retval = retval,
> > > - };
> > > + struct bpf_sockopt_kern ctx;
> > > + struct bpf_sockopt_buf buf;
> > > + struct cgroup *cgrp;
> > > int ret;
> > >
> > > +#ifdef CONFIG_INET
> > > + /* TCP do_tcp_getsockopt has optimized getsockopt implementation
> > > + * to avoid extra socket lock for TCP_ZEROCOPY_RECEIVE.
> > > + */
> > > + if (sk->sk_prot->getsockopt == tcp_getsockopt &&
> > > + level == SOL_TCP && optname == TCP_ZEROCOPY_RECEIVE)
> > > + return retval;
> > > +#endif
> > That seems too much protocol details and not very scalable.
> > It is not very related to kernel/bpf/cgroup.c which has very little idea
> > whether a specific protocol has optimized things in some ways (e.g. by
> > directly calling cgroup's bpf prog at some strategic places in this
> > patch).
> > Lets see if it can be done better.
>
> > At least, these protocol checks belong to the net's socket.c
> > more than the bpf's cgroup.c here. If it also looks like layering
> > breakage in socket.c, may be adding a signal in sk_prot (for example)
> > to tell if the sk_prot->getsockopt has already called the cgroup's bpf
> > prog? (e.g. tcp_getsockopt() can directly call the cgroup's bpf for all
> > optname instead of only TCP_ZEROCOPY_RECEIVE).
>
> > For example:
>
> > int __sys_getsockopt(...)
> > {
> > /* ... */
>
> > if (!sk_prot->bpf_getsockopt_handled)
> > BPF_CGROUP_RUN_PROG_GETSOCKOPT(...);
> > }
>
> > Thoughts?
>
> Sounds good. I didn't go that far because I don't expect there to be
> a lot of special cases like that. But it might be worth supporting
> it in a generic way from the beginning.
>
> I was thinking about something simpler:
>
> int __cgroup_bpf_run_filter_getsockopt(sk, ...)
> {
> if (sk->sk_prot->bypass_bpf_getsockopt(level, optlen)) {
I think it meant s/optlen/optname/ which is not __user.
Yeah, I think that can provide a more generic solution
and also abstract things away.
Please add a details comment in this function.
> return retval;
> }
>
> // ...
> }
>
> Not sure it's worth exposing it to the __sys_getsockopt. WDYT?
or call that in BPF_CGROUP_RUN_PROG_GETSOCKOPT(). then the
changes in __cgroup_bpf_run_filter_getsockopt() in this
patch should go away?
