Re: [bpf-next PATCH v3 2/6] bpf, sockmap: On receive programs try to fast track SK_PASS ingress

[John Fastabend <john.fastabend@gmail.com>]

Jakub Sitnicki wrote:
> Hey John,
> 
> Exiting to see this work :-)
> 
> 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

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.

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.

> 
> Do we need a similar check here?
> 

Don't think so for above reason. Thanks for asking though and let me
know if you see something.

I think to make the workqueue path symmetric I'll move the check there
into the egress branch.

> > +		}
> > +		if (err < 0) {
> > +			skb_queue_tail(&psock->ingress_skb, skb);
> > +			schedule_work(&psock->work);
> > +		}
> >  		break;
> >  	case __SK_REDIRECT:
> >  		sk_psock_skb_redirect(skb);