On Mon, May 2, 2022 at 5:02 PM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
>
> On Mon, May 2, 2022 at 11:24 AM Cong Wang <xiyou.wangcong@xxxxxxxxx> wrote:
> >
> > From: Cong Wang <cong.wang@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Cc: John Fastabend <john.fastabend@xxxxxxxxx>
> > Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> > Cc: Jakub Sitnicki <jakub@xxxxxxxxxxxxxx>
> > Signed-off-by: Cong Wang <cong.wang@xxxxxxxxxxxxx>
> > ---
> > 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.
