From: Jakub Sitnicki <jakub@cloudflare.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: alexei.starovoitov@gmail.com, daniel@iogearbox.net,
netdev@vger.kernel.org, bpf@vger.kernel.org, lmb@cloudflare.com
Subject: Re: [bpf-next PATCH v3 2/6] bpf, sockmap: On receive programs try to fast track SK_PASS ingress
Date: Tue, 13 Oct 2020 21:43:03 +0200 [thread overview]
Message-ID: <87blh5rjy0.fsf@cloudflare.com> (raw)
In-Reply-To: <5f8477448f66e_370c208e4@john-XPS-13-9370.notmuch>
On Mon, Oct 12, 2020 at 05:33 PM CEST, John Fastabend wrote:
> Jakub Sitnicki wrote:
[...]
>> On Fri, Oct 09, 2020 at 08:36 PM CEST, John Fastabend wrote:
>> > When we receive an skb and the ingress skb verdict program returns
>> > SK_PASS we currently set the ingress flag and put it on the workqueue
>> > so it can be turned into a sk_msg and put on the sk_msg ingress queue.
>> > Then finally telling userspace with data_ready hook.
>> >
>> > Here we observe that if the workqueue is empty then we can try to
>> > convert into a sk_msg type and call data_ready directly without
>> > bouncing through a workqueue. Its a common pattern to have a recv
>> > verdict program for visibility that always returns SK_PASS. In this
>> > case unless there is an ENOMEM error or we overrun the socket we
>> > can avoid the workqueue completely only using it when we fall back
>> > to error cases caused by memory pressure.
>> >
>> > By doing this we eliminate another case where data may be dropped
>> > if errors occur on memory limits in workqueue.
>> >
>> > Fixes: 51199405f9672 ("bpf: skb_verdict, support SK_PASS on RX BPF path")
>> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>> > ---
>> > net/core/skmsg.c | 17 +++++++++++++++--
>> > 1 file changed, 15 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/net/core/skmsg.c b/net/core/skmsg.c
>> > index 040ae1d75b65..4b160d97b7f9 100644
>> > --- a/net/core/skmsg.c
>> > +++ b/net/core/skmsg.c
>> > @@ -773,6 +773,7 @@ static void sk_psock_verdict_apply(struct sk_psock *psock,
>> > {
>> > struct tcp_skb_cb *tcp;
>> > struct sock *sk_other;
>> > + int err = -EIO;
>> >
>> > switch (verdict) {
>> > case __SK_PASS:
>> > @@ -784,8 +785,20 @@ static void sk_psock_verdict_apply(struct sk_psock *psock,
>> >
>> > tcp = TCP_SKB_CB(skb);
>> > tcp->bpf.flags |= BPF_F_INGRESS;
>> > - skb_queue_tail(&psock->ingress_skb, skb);
>> > - schedule_work(&psock->work);
>> > +
>> > + /* If the queue is empty then we can submit directly
>> > + * into the msg queue. If its not empty we have to
>> > + * queue work otherwise we may get OOO data. Otherwise,
>> > + * if sk_psock_skb_ingress errors will be handled by
>> > + * retrying later from workqueue.
>> > + */
>> > + if (skb_queue_empty(&psock->ingress_skb)) {
>> > + err = sk_psock_skb_ingress(psock, skb);
>>
>> When going through the workqueue (sk_psock_backlog), we will also check
>> if socket didn't get detached from the process, that is if
>> psock->sk->sk_socket != NULL, before queueing into msg queue.
>
> The sk_socket check is only for the egress path,
>
> sk_psock_handle_skb -> skb_send_sock_locked -> kernel_sendmsg_locked
Oh, okay. I thought it was because we want to forwarding into the socket
as soon as there is no process to read from the queue.
> Then the do_tcp_sendpages() uses sk_socket and I don't see any checks for
> sk_socket being set. Although I think its worth looking through to see
> if the psock/sk state is always such that we have sk_socket there I
> don't recall off-hand where that is null'd.
It's in sock_orphan().
> But, to answer your question this is ingress only and here we don't
> use sk_socket for anything so I don't see any reason the check is
> needed. All that is done here is converting to skmsg and posting
> onto ingress queue.
Queued skb won't be read out, but I don't see a problem with it.
[...]
next prev parent reply other threads:[~2020-10-13 19:43 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-09 18:35 [bpf-next PATCH v2 0/6] sockmap/sk_skb program memory acct fixes John Fastabend
2020-10-09 18:36 ` [bpf-next PATCH v3 1/6] bpf, sockmap: skb verdict SK_PASS to self already checked rmem limits John Fastabend
2020-10-09 18:36 ` [bpf-next PATCH v3 2/6] bpf, sockmap: On receive programs try to fast track SK_PASS ingress John Fastabend
2020-10-12 9:03 ` Jakub Sitnicki
2020-10-12 15:33 ` John Fastabend
2020-10-13 19:43 ` Jakub Sitnicki [this message]
2020-10-09 18:36 ` [bpf-next PATCH v3 3/6] bpf, sockmap: remove skb_set_owner_w wmem will be taken later from sendpage John Fastabend
2020-10-09 18:37 ` [bpf-next PATCH v3 4/6] bpf, sockmap: remove dropped data on errors in redirect case John Fastabend
2020-10-12 12:04 ` Jakub Sitnicki
2020-10-12 17:19 ` John Fastabend
2020-10-13 10:27 ` Jakub Sitnicki
2020-10-09 18:37 ` [bpf-next PATCH v3 5/6] bpf, sockmap: Remove skb_orphan and let normal skb_kfree do cleanup John Fastabend
2020-10-09 18:37 ` [bpf-next PATCH v3 6/6] bpf, sockmap: Add memory accounting so skbs on ingress lists are visible John Fastabend
2020-10-12 1:10 ` [bpf-next PATCH v2 0/6] sockmap/sk_skb program memory acct fixes patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87blh5rjy0.fsf@cloudflare.com \
--to=jakub@cloudflare.com \
--cc=alexei.starovoitov@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.com \
--cc=lmb@cloudflare.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).