| Message ID | 160226839426.5692.13107801574043388675.stgit@john-Precision-5820-Tower |
|---|---|
| Headers | show |
| Series | sockmap/sk_skb program memory acct fixes | expand |
Hello: This series was applied to bpf/bpf-next.git (refs/heads/master): On Fri, 09 Oct 2020 11:35:53 -0700 you wrote: > Users of sockmap and skmsg trying to build proxys and other tools > have pointed out to me the error handling can be problematic. If > the proxy is under-provisioned and/or the BPF admin does not have > the ability to update/modify memory provisions on the sockets > its possible data may be dropped. For some things we have retries > so everything works out OK, but for most things this is likely > not great. And things go bad. > > [...] Here is the summary with links: - [bpf-next,v3,1/6] bpf, sockmap: skb verdict SK_PASS to self already checked rmem limits https://git.kernel.org/bpf/bpf-next/c/cfea28f890cf - [bpf-next,v3,2/6] bpf, sockmap: On receive programs try to fast track SK_PASS ingress https://git.kernel.org/bpf/bpf-next/c/9ecbfb06a078 - [bpf-next,v3,3/6] bpf, sockmap: remove skb_set_owner_w wmem will be taken later from sendpage https://git.kernel.org/bpf/bpf-next/c/29545f4977cf - [bpf-next,v3,4/6] bpf, sockmap: remove dropped data on errors in redirect case https://git.kernel.org/bpf/bpf-next/c/9047f19e7ccb - [bpf-next,v3,5/6] bpf, sockmap: Remove skb_orphan and let normal skb_kfree do cleanup https://git.kernel.org/bpf/bpf-next/c/10d58d006356 - [bpf-next,v3,6/6] bpf, sockmap: Add memory accounting so skbs on ingress lists are visible https://git.kernel.org/bpf/bpf-next/c/0b17ad25d8d1 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Users of sockmap and skmsg trying to build proxys and other tools have pointed out to me the error handling can be problematic. If the proxy is under-provisioned and/or the BPF admin does not have the ability to update/modify memory provisions on the sockets its possible data may be dropped. For some things we have retries so everything works out OK, but for most things this is likely not great. And things go bad. The original design dropped memory accounting on the receive socket as early as possible. We did this early in sk_skb handling and then charged it to the redirect socket immediately after running the BPF program. But, this design caused a fundamental problem. Namely, what should we do if we redirect to a socket that has already reached its socket memory limits. For proxy use cases the network admin can tune memory limits. But, in general we punted on this problem and told folks to simply make your memory limits high enough to handle your workload. This is not a really good answer. When deploying into environments where we expect this to be transparent its no longer the case because we need to tune params. In fact its really only viable in cases where we have fine grained control over the application. For example a proxy redirecting from an ingress socket to an egress socket. The result is I get bug reports because its surprising for one, but more importantly also breaks some use cases. So lets fix it. This series cleans up the different cases so that in many common modes, such as passing packet up to receive socket, we can simply use the underlying assumption that the TCP stack already has done memory accounting. Next instead of trying to do memory accounting against the socket we plan to redirect into we keep memory accounting on the receive socket until the skb can be put on the redirect socket. This means if we do an egress redirect to a socket and sock_writable() returns EAGAIN we can requeue the skb on the workqueue and try again. The same scenario plays out for ingress. If the skb can not be put on the receive queue of the redirect socket than we simply requeue and retry. In both cases memory is still accounted for against the receiving socket. This also handles head of line blocking. With the above scheme the skb is on a queue associated with the socket it will be sent/recv'd on, but the memory accounting is against the received socket. This means the receive socket can advance to the next skb and avoid head of line blocking. At least until its receive memory on the socket runs out. This will put some maximum size on the amount of data any socket can enqueue giving us bounds on the skb lists so they can't grow indefinitely. Overall I think this is a win. Tested with test_sockmap. These are fixes, but I tagged it for bpf-next considering we are at -rc8. v1->v2: Fix uninitialized/unused variables (kernel test robot) v2->v3: fix typo in patch2 err=0 needs to be <0 so use err=-EIO --- John Fastabend (6): bpf, sockmap: skb verdict SK_PASS to self already checked rmem limits bpf, sockmap: On receive programs try to fast track SK_PASS ingress bpf, sockmap: remove skb_set_owner_w wmem will be taken later from sendpage bpf, sockmap: remove dropped data on errors in redirect case bpf, sockmap: Remove skb_orphan and let normal skb_kfree do cleanup bpf, sockmap: Add memory accounting so skbs on ingress lists are visible net/core/skmsg.c | 83 +++++++++++++++++++++++++++++------------------------- 1 file changed, 45 insertions(+), 38 deletions(-) -- Signature