Re: [PATCH] tcp: Add TCP_INFO counter for packets received out-of-order

Re: [PATCH] tcp: Add TCP_INFO counter for packets received out-of-order

[Eric Dumazet]

On Mon, Sep 9, 2019 at 4:30 PM Thomas Higdon <tph@fb.com> wrote:
>
> For receive-heavy cases on the server-side, we want to track the
> connection quality for individual client IPs. This counter, similar to
> the existing system-wide TCPOFOQueue counter in /proc/net/netstat,
> tracks out-of-order packet reception. By providing this counter in
> TCP_INFO, it will allow understanding to what degree receive-heavy
> sockets are experiencing out-of-order delivery and packet drops
> indicating congestion.
>
> Please note that this is similar to the counter in NetBSD TCP_INFO, and
> has the same name.
>
> Signed-off-by: Thomas Higdon <tph@fb.com>
> ---
>  include/linux/tcp.h      | 2 ++
>  include/uapi/linux/tcp.h | 2 ++
>  net/ipv4/tcp.c           | 1 +
>  net/ipv4/tcp_input.c     | 1 +
>  4 files changed, 6 insertions(+)
>
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index f3a85a7fb4b1..a01dc78218f1 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -393,6 +393,8 @@ struct tcp_sock {
>          */
>         struct request_sock *fastopen_rsk;
>         u32     *saved_syn;
> +
> +       u32 rcv_ooopack; /* Received out-of-order packets, for tcpinfo */
>  };
>
>  enum tsq_enum {
> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> index b3564f85a762..20237987ccc8 100644
> --- a/include/uapi/linux/tcp.h
> +++ b/include/uapi/linux/tcp.h
> @@ -270,6 +270,8 @@ struct tcp_info {
>         __u64   tcpi_bytes_retrans;  /* RFC4898 tcpEStatsPerfOctetsRetrans */
>         __u32   tcpi_dsack_dups;     /* RFC4898 tcpEStatsStackDSACKDups */
>         __u32   tcpi_reord_seen;     /* reordering events seen */
> +
> +       __u32   tcpi_rcv_ooopack;    /* Out-of-order packets received */

This is problematic : you create a 32bit hole in this structure that
we will never be able to fill.

We need to add another metric here so that the whole 64bit space is used.

>  };
>
>  /* netlink attributes types for SCM_TIMESTAMPING_OPT_STATS */
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 94df48bcecc2..d4386f054f18 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -3295,6 +3295,7 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
>         info->tcpi_bytes_retrans = tp->bytes_retrans;
>         info->tcpi_dsack_dups = tp->dsack_dups;
>         info->tcpi_reord_seen = tp->reord_seen;
> +       info->tcpi_rcv_ooopack = tp->rcv_ooopack;
>         unlock_sock_fast(sk, slow);
>  }
>  EXPORT_SYMBOL_GPL(tcp_get_info);
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 706cbb3b2986..2774680c5d05 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4555,6 +4555,7 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
>         tp->pred_flags = 0;
>         inet_csk_schedule_ack(sk);
>
> +       tp->rcv_ooopack++;

We count skbs or we count segments ?

(GRO might have aggregated multiple segments)


>         NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPOFOQUEUE);
>         seq = TCP_SKB_CB(skb)->seq;
>         end_seq = TCP_SKB_CB(skb)->end_seq;


You forgot to clear the field in tcp_disconnect()

Re: [PATCH] tcp: Add TCP_INFO counter for packets received out-of-order

[Thomas Higdon]

On Mon, Sep 09, 2019 at 05:01:46PM +0200, Eric Dumazet wrote:
> On Mon, Sep 9, 2019 at 4:30 PM Thomas Higdon <tph@fb.com> wrote:
> > diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> > index b3564f85a762..20237987ccc8 100644
> > --- a/include/uapi/linux/tcp.h
> > +++ b/include/uapi/linux/tcp.h
> > @@ -270,6 +270,8 @@ struct tcp_info {
> >         __u64   tcpi_bytes_retrans;  /* RFC4898 tcpEStatsPerfOctetsRetrans */
> >         __u32   tcpi_dsack_dups;     /* RFC4898 tcpEStatsStackDSACKDups */
> >         __u32   tcpi_reord_seen;     /* reordering events seen */
> > +
> > +       __u32   tcpi_rcv_ooopack;    /* Out-of-order packets received */
> 
> This is problematic : you create a 32bit hole in this structure that
> we will never be able to fill.
> 
> We need to add another metric here so that the whole 64bit space is used.

I don't have another metric to add currently. Perhaps I could first place
a '__u32 _reserved' member so that someone else may replace it with
a 32-bit member in the future. Unless there is a canonical way to do
this? I couldn't find any prior examples.

> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 706cbb3b2986..2774680c5d05 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -4555,6 +4555,7 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
> >         tp->pred_flags = 0;
> >         inet_csk_schedule_ack(sk);
> >
> > +       tp->rcv_ooopack++;
> 
> We count skbs or we count segments ?
> 
> (GRO might have aggregated multiple segments)

Let's count segments -- I will copy the technique of tcp_segs_in(), which
checks the maximum of 1 and gso_segs from the lower layer. Interestingly,
on my development machine, which uses the virtio-net driver, when LRO is
enabled, gso_segs is always zero, even when an aggregated segment is
passed up the stack. I guess this may be a problem with virtio-net? It
still seems that using gso_segs here is the correct solution.