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

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

[Eric Dumazet]

On Tue, Sep 10, 2019 at 10:11 PM Thomas Higdon <tph@fb.com> wrote:
>
>
...
> Because an additional 32-bit member in struct tcp_info would cause
> a hole on 64-bit systems, we reserve a struct member '_reserved'.
...
> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> index b3564f85a762..990a5bae3ac1 100644
> --- a/include/uapi/linux/tcp.h
> +++ b/include/uapi/linux/tcp.h
> @@ -270,6 +270,9 @@ struct tcp_info {
>         __u64   tcpi_bytes_retrans;  /* RFC4898 tcpEStatsPerfOctetsRetrans */
>         __u32   tcpi_dsack_dups;     /* RFC4898 tcpEStatsStackDSACKDups */
>         __u32   tcpi_reord_seen;     /* reordering events seen */
> +
> +       __u32   _reserved;           /* Reserved for future 32-bit member. */
> +       __u32   tcpi_rcv_ooopack;    /* Out-of-order packets received */
>  };
>

Unfortunately we won't be able to use this hole, because the way the
TCP_INFO works,

The kernel will report the same size after the reserved field is
renamed to something else.

User space code is able to detect which fields are there or not based
on what the kernel
returns for the size of the structure.

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

[Neal Cardwell]

On Tue, Sep 10, 2019 at 4:39 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 10, 2019 at 10:11 PM Thomas Higdon <tph@fb.com> wrote:
> >
> >
> ...
> > Because an additional 32-bit member in struct tcp_info would cause
> > a hole on 64-bit systems, we reserve a struct member '_reserved'.
> ...
> > diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> > index b3564f85a762..990a5bae3ac1 100644
> > --- a/include/uapi/linux/tcp.h
> > +++ b/include/uapi/linux/tcp.h
> > @@ -270,6 +270,9 @@ struct tcp_info {
> >         __u64   tcpi_bytes_retrans;  /* RFC4898 tcpEStatsPerfOctetsRetrans */
> >         __u32   tcpi_dsack_dups;     /* RFC4898 tcpEStatsStackDSACKDups */
> >         __u32   tcpi_reord_seen;     /* reordering events seen */
> > +
> > +       __u32   _reserved;           /* Reserved for future 32-bit member. */
> > +       __u32   tcpi_rcv_ooopack;    /* Out-of-order packets received */
> >  };
> >
>
> Unfortunately we won't be able to use this hole, because the way the
> TCP_INFO works,
>
> The kernel will report the same size after the reserved field is
> renamed to something else.
>
> User space code is able to detect which fields are there or not based
> on what the kernel
> returns for the size of the structure.

If we are looking for something else useful to use for a __u32 to pair
up with this new field, I would suggest  we export tp->snd_wnd
(send-side receive window) and tp->rcv_wnd (receive-side receive
window) in tcp_info. We could export one of them now, and the other
the next time we need to add a field and need some useful "padding".
These fields could help folks diagnose whether a flow is
receive-window-limited at a given instant, using "ss", etc. I think
there was some interest in this internally in our team a while ago.

neal

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

[Jason Baron]

On 9/10/19 4:38 PM, Eric Dumazet wrote:
> On Tue, Sep 10, 2019 at 10:11 PM Thomas Higdon <tph@fb.com> wrote:
>>
>>
> ...
>> Because an additional 32-bit member in struct tcp_info would cause
>> a hole on 64-bit systems, we reserve a struct member '_reserved'.
> ...
>> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
>> index b3564f85a762..990a5bae3ac1 100644
>> --- a/include/uapi/linux/tcp.h
>> +++ b/include/uapi/linux/tcp.h
>> @@ -270,6 +270,9 @@ struct tcp_info {
>>         __u64   tcpi_bytes_retrans;  /* RFC4898 tcpEStatsPerfOctetsRetrans */
>>         __u32   tcpi_dsack_dups;     /* RFC4898 tcpEStatsStackDSACKDups */
>>         __u32   tcpi_reord_seen;     /* reordering events seen */
>> +
>> +       __u32   _reserved;           /* Reserved for future 32-bit member. */
>> +       __u32   tcpi_rcv_ooopack;    /* Out-of-order packets received */
>>  };
>>
> 
> Unfortunately we won't be able to use this hole, because the way the
> TCP_INFO works,
> 
> The kernel will report the same size after the reserved field is
> renamed to something else.
> 
> User space code is able to detect which fields are there or not based
> on what the kernel
> returns for the size of the structure.
> 

Hi,

I was interested in adding a field to tcp_info around the TFO state of a
socket. So for the server side it would indicate if TFO was used to
create the socket and on the client side it would report whether TFO
worked and if not that it failed with maybe some additional states
around why it failed. I'm thinking it would be maybe 3 bits.

My question is whether its reasonable to use the unused bits of
__u8    tcpi_delivery_rate_app_limited:1;. Or is this not good because
the size hasn't changed? What if I avoided using 0 for the new field to
avoid the possibility of not knowing if 0 because its the old kernel or
0 because that's now its a TFO state? IE the new field could always be >
0 for the new kernel.

Thanks,

-Jason

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

[Eric Dumazet]

 Tue, Sep 17, 2019 at 10:13 AM Jason Baron <jbaron@akamai.com> wrote:
>
>
> Hi,
>
> I was interested in adding a field to tcp_info around the TFO state of a
> socket. So for the server side it would indicate if TFO was used to
> create the socket and on the client side it would report whether TFO
> worked and if not that it failed with maybe some additional states
> around why it failed. I'm thinking it would be maybe 3 bits.
>
> My question is whether its reasonable to use the unused bits of
> __u8    tcpi_delivery_rate_app_limited:1;. Or is this not good because
> the size hasn't changed? What if I avoided using 0 for the new field to
> avoid the possibility of not knowing if 0 because its the old kernel or
> 0 because that's now its a TFO state? IE the new field could always be >
> 0 for the new kernel.
>

I guess that storing the 'why it has failed' would need more bits.

I suggest maybe using an event for this, instead of TCP_INFO ?

As of using the bits, maybe the monitoring application does not really care
if running on an old kernel where the bits would be zero.

Commit eb8329e0a04db0061f714f033b4454326ba147f4 reserved a single
bit and did not bother about making sure the monitoring would detect if this
runs on an old kernel.

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

[Neal Cardwell]

On Tue, Sep 17, 2019 at 1:22 PM Eric Dumazet <edumazet@google.com> wrote:
>
>  Tue, Sep 17, 2019 at 10:13 AM Jason Baron <jbaron@akamai.com> wrote:
> >
> >
> > Hi,
> >
> > I was interested in adding a field to tcp_info around the TFO state of a
> > socket. So for the server side it would indicate if TFO was used to
> > create the socket and on the client side it would report whether TFO
> > worked and if not that it failed with maybe some additional states
> > around why it failed. I'm thinking it would be maybe 3 bits.

BTW, one aspect of that "did TFO work" info is available already in
tcp_info in the tcpi_options field.

Kernel side is:
  if (tp->syn_data_acked)
        info->tcpi_options |= TCPI_OPT_SYN_DATA;

We use this bit in packetdrill tests on client and server side to
check that the TFO data-in-SYN succeeded:
   +0 %{ assert (tcpi_options & TCPI_OPT_SYN_DATA) != 0, tcpi_options }%

These TFO bits were added much later than the other bits, so IMHO it
would be OK to add more bits somewhere unused in tcp_info to indicate
reasons for TFO failure. Especially if, as you suggest, "0" as a code
point could indicate that the code point is undefined, and all
meaningful code points were non-zero.

neal

> > My question is whether its reasonable to use the unused bits of
> > __u8    tcpi_delivery_rate_app_limited:1;. Or is this not good because
> > the size hasn't changed? What if I avoided using 0 for the new field to
> > avoid the possibility of not knowing if 0 because its the old kernel or
> > 0 because that's now its a TFO state? IE the new field could always be >
> > 0 for the new kernel.
> >
>
> I guess that storing the 'why it has failed' would need more bits.
>
> I suggest maybe using an event for this, instead of TCP_INFO ?
>
> As of using the bits, maybe the monitoring application does not really care
> if running on an old kernel where the bits would be zero.
>
> Commit eb8329e0a04db0061f714f033b4454326ba147f4 reserved a single
> bit and did not bother about making sure the monitoring would detect if this
> runs on an old kernel.