Re: [Patch bpf-next v2 1/4] tcp: introduce tcp_read_skb()

[Cong Wang <xiyou.wangcong@gmail.com>]

On Mon, May 2, 2022 at 5:02 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, May 2, 2022 at 11:24 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > From: Cong Wang <cong.wang@bytedance.com>
> >
> > This patch inroduces tcp_read_skb() based on tcp_read_sock(),
> > a preparation for the next patch which actually introduces
> > a new sock ops.
> >
> > TCP is special here, because it has tcp_read_sock() which is
> > mainly used by splice(). tcp_read_sock() supports partial read
> > and arbitrary offset, neither of them is needed for sockmap.
> >
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: John Fastabend <john.fastabend@gmail.com>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Jakub Sitnicki <jakub@cloudflare.com>
> > Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> > ---
> >  include/net/tcp.h |  2 ++
> >  net/ipv4/tcp.c    | 63 +++++++++++++++++++++++++++++++++++++++++------
> >  2 files changed, 57 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index 94a52ad1101c..ab7516e5cc56 100644
> > --- a/include/net/tcp.h
> > +++ b/include/net/tcp.h
> > @@ -667,6 +667,8 @@ void tcp_get_info(struct sock *, struct tcp_info *);
> >  /* Read 'sendfile()'-style from a TCP socket */
> >  int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> >                   sk_read_actor_t recv_actor);
> > +int tcp_read_skb(struct sock *sk, read_descriptor_t *desc,
> > +                sk_read_actor_t recv_actor);
> >
> >  void tcp_initialize_rcv_mss(struct sock *sk);
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index db55af9eb37b..8d48126e3694 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -1600,7 +1600,7 @@ static void tcp_eat_recv_skb(struct sock *sk, struct sk_buff *skb)
> >         __kfree_skb(skb);
> >  }
> >
> > -static struct sk_buff *tcp_recv_skb(struct sock *sk, u32 seq, u32 *off)
> > +static struct sk_buff *tcp_recv_skb(struct sock *sk, u32 seq, u32 *off, bool unlink)
> >  {
> >         struct sk_buff *skb;
> >         u32 offset;
> > @@ -1613,6 +1613,8 @@ static struct sk_buff *tcp_recv_skb(struct sock *sk, u32 seq, u32 *off)
> >                 }
> >                 if (offset < skb->len || (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN)) {
> >                         *off = offset;
> > +                       if (unlink)
> > +                               __skb_unlink(skb, &sk->sk_receive_queue);
>
> Why adding this @unlink parameter ?
> This makes your patch more invasive than needed.
> Can not this unlink happen from your new helper instead ? See [3] later.

Good point, I was trying to reuse the code there, but it is just one
__skb_unlink().

>
> >                         return skb;
> >                 }
> >                 /* This looks weird, but this can happen if TCP collapsing
> > @@ -1646,7 +1648,7 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> >
> >         if (sk->sk_state == TCP_LISTEN)
> >                 return -ENOTCONN;
> > -       while ((skb = tcp_recv_skb(sk, seq, &offset)) != NULL) {
> > +       while ((skb = tcp_recv_skb(sk, seq, &offset, false)) != NULL) {
> >                 if (offset < skb->len) {
> >                         int used;
> >                         size_t len;
> > @@ -1677,7 +1679,7 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> >                          * getting here: tcp_collapse might have deleted it
> >                          * while aggregating skbs from the socket queue.
> >                          */
> > -                       skb = tcp_recv_skb(sk, seq - 1, &offset);
> > +                       skb = tcp_recv_skb(sk, seq - 1, &offset, false);
> >                         if (!skb)
> >                                 break;
> >                         /* TCP coalescing might have appended data to the skb.
> > @@ -1702,13 +1704,58 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> >
> >         /* Clean up data we have read: This will do ACK frames. */
> >         if (copied > 0) {
> > -               tcp_recv_skb(sk, seq, &offset);
> > +               tcp_recv_skb(sk, seq, &offset, false);
> >                 tcp_cleanup_rbuf(sk, copied);
> >         }
> >         return copied;
> >  }
> >  EXPORT_SYMBOL(tcp_read_sock);
> >
> > +int tcp_read_skb(struct sock *sk, read_descriptor_t *desc,
> > +                sk_read_actor_t recv_actor)
> > +{
> > +       struct tcp_sock *tp = tcp_sk(sk);
> > +       u32 seq = tp->copied_seq;
> > +       struct sk_buff *skb;
> > +       int copied = 0;
> > +       u32 offset;
> > +
> > +       if (sk->sk_state == TCP_LISTEN)
> > +               return -ENOTCONN;
> > +
> > +       while ((skb = tcp_recv_skb(sk, seq, &offset, true)) != NULL) {
>
> [3]
>             The unlink from sk->sk_receive_queue could happen here.

Right.

>
> > +               int used = recv_actor(desc, skb, 0, skb->len);
> > +
> > +               if (used <= 0) {
> > +                       if (!copied)
> > +                               copied = used;
> > +                       break;
> > +               }
> > +               seq += used;
> > +               copied += used;
> > +
> > +               if (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN) {
> > +                       kfree_skb(skb);
>
> [1]
>
> The two kfree_skb() ([1] & [2]) should be a consume_skb() ?

Hm, it is tricky here, we use the skb refcount after this patchset, so
it could be a real drop from another kfree_skb() in net/core/skmsg.c
which initiates the drop.

Thanks.