| Message ID | 20220502182345.306970-2-xiyou.wangcong@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | sockmap: some performance optimizations | expand |
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. > 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. > + 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() ? > + ++seq; > + break; > + } > + kfree_skb(skb); [2] > + if (!desc->count) > + break; > + WRITE_ONCE(tp->copied_seq, seq); > + } > + WRITE_ONCE(tp->copied_seq, seq); > + > + tcp_rcv_space_adjust(sk); > + > + /* Clean up data we have read: This will do ACK frames. */ > + if (copied > 0) > + tcp_cleanup_rbuf(sk, copied); > + > + return copied; > +} > +EXPORT_SYMBOL(tcp_read_skb); > + > int tcp_peek_len(struct socket *sock) > { > return tcp_inq(sock->sk); > @@ -1890,7 +1937,7 @@ static int receive_fallback_to_copy(struct sock *sk, > struct sk_buff *skb; > u32 offset; > > - skb = tcp_recv_skb(sk, tcp_sk(sk)->copied_seq, &offset); > + skb = tcp_recv_skb(sk, tcp_sk(sk)->copied_seq, &offset, false); > if (skb) > tcp_zerocopy_set_hint_for_skb(sk, zc, skb, offset); > } > @@ -1937,7 +1984,7 @@ static int tcp_zc_handle_leftover(struct tcp_zerocopy_receive *zc, > if (skb) { > offset = *seq - TCP_SKB_CB(skb)->seq; > } else { > - skb = tcp_recv_skb(sk, *seq, &offset); > + skb = tcp_recv_skb(sk, *seq, &offset, false); > if (TCP_SKB_CB(skb)->has_rxtstamp) { > tcp_update_recv_tstamps(skb, tss); > zc->msg_flags |= TCP_CMSG_TS; > @@ -2130,7 +2177,7 @@ static int tcp_zerocopy_receive(struct sock *sk, > skb = skb->next; > offset = seq - TCP_SKB_CB(skb)->seq; > } else { > - skb = tcp_recv_skb(sk, seq, &offset); > + skb = tcp_recv_skb(sk, seq, &offset, false); > } > > if (TCP_SKB_CB(skb)->has_rxtstamp) { > @@ -2186,7 +2233,7 @@ static int tcp_zerocopy_receive(struct sock *sk, > tcp_rcv_space_adjust(sk); > > /* Clean up data we have read: This will do ACK frames. */ > - tcp_recv_skb(sk, seq, &offset); > + tcp_recv_skb(sk, seq, &offset, false); > tcp_cleanup_rbuf(sk, length + copylen); > ret = 0; > if (length == zc->length) > -- > 2.32.0 >
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.
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); 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) { + 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); + ++seq; + break; + } + kfree_skb(skb); + if (!desc->count) + break; + WRITE_ONCE(tp->copied_seq, seq); + } + WRITE_ONCE(tp->copied_seq, seq); + + tcp_rcv_space_adjust(sk); + + /* Clean up data we have read: This will do ACK frames. */ + if (copied > 0) + tcp_cleanup_rbuf(sk, copied); + + return copied; +} +EXPORT_SYMBOL(tcp_read_skb); + int tcp_peek_len(struct socket *sock) { return tcp_inq(sock->sk); @@ -1890,7 +1937,7 @@ static int receive_fallback_to_copy(struct sock *sk, struct sk_buff *skb; u32 offset; - skb = tcp_recv_skb(sk, tcp_sk(sk)->copied_seq, &offset); + skb = tcp_recv_skb(sk, tcp_sk(sk)->copied_seq, &offset, false); if (skb) tcp_zerocopy_set_hint_for_skb(sk, zc, skb, offset); } @@ -1937,7 +1984,7 @@ static int tcp_zc_handle_leftover(struct tcp_zerocopy_receive *zc, if (skb) { offset = *seq - TCP_SKB_CB(skb)->seq; } else { - skb = tcp_recv_skb(sk, *seq, &offset); + skb = tcp_recv_skb(sk, *seq, &offset, false); if (TCP_SKB_CB(skb)->has_rxtstamp) { tcp_update_recv_tstamps(skb, tss); zc->msg_flags |= TCP_CMSG_TS; @@ -2130,7 +2177,7 @@ static int tcp_zerocopy_receive(struct sock *sk, skb = skb->next; offset = seq - TCP_SKB_CB(skb)->seq; } else { - skb = tcp_recv_skb(sk, seq, &offset); + skb = tcp_recv_skb(sk, seq, &offset, false); } if (TCP_SKB_CB(skb)->has_rxtstamp) { @@ -2186,7 +2233,7 @@ static int tcp_zerocopy_receive(struct sock *sk, tcp_rcv_space_adjust(sk); /* Clean up data we have read: This will do ACK frames. */ - tcp_recv_skb(sk, seq, &offset); + tcp_recv_skb(sk, seq, &offset, false); tcp_cleanup_rbuf(sk, length + copylen); ret = 0; if (length == zc->length)