Re: TSecr != 0 check in inet_lro.c

[Eric Dumazet <eric.dumazet@gmail.com>]

Octavian Purdila a écrit :
> Hi,
> 
> We are seeing a performance issue with TSO/LRO which we tracked down to the 
> TSecr !=0 check in lro_tcp_ip_check.

ouch...

> 
> It happens when the LRO side's TSval wraps around and gets to 0. That triggers 
> the TSO side to send packets with TSecr set to 0, which means that such 
> packets won't be aggregated - and that will put a lot of burden on the stack 
> which will result in lots of drops.

Probability of such event is 1 / 2^32 or so ?

> 
> I'm failing to understand the purpose of this check. Any hints? :)
> 

rfc1323 badly interpreted ?

I remember tsecr=0 was forbidden by Linux, while apparently rfc is not
so clear.

rfc1323 : 3.2
         The Timestamp Echo Reply field (TSecr) is only valid if the ACK
         bit is set in the TCP header; if it is valid, it echos a times-
         tamp value that was sent by the remote TCP in the TSval field
         of a Timestamps option.  When TSecr is not valid, its value
         must be zero.  The TSecr value will generally be from the most
         recent Timestamp option that was received; however, there are
         exceptions that are explained below.

Note how this is not saying "a zero Tsecr value is not valid"

I could not find why : "When TSecr is not valid, its value
must be zero", and why we consider a zero value to be not meaningfull...

static inline void tcp_ack_update_rtt(struct sock *sk, const int flag,
                                      const s32 seq_rtt)
{
        const struct tcp_sock *tp = tcp_sk(sk);
        /* Note that peer MAY send zero echo. In this case it is ignored. (rfc1323) */
        if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr)
                tcp_ack_saw_tstamp(sk, flag);
        else if (seq_rtt >= 0)
                tcp_ack_no_tstamp(sk, seq_rtt, flag);
}

static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
                                         struct tcphdr *th, unsigned len)
{
        struct tcp_sock *tp = tcp_sk(sk);
        struct inet_connection_sock *icsk = inet_csk(sk);
        int saved_clamp = tp->rx_opt.mss_clamp;

        tcp_parse_options(skb, &tp->rx_opt, 0);

        if (th->ack) {
...
                if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr &&
                    !between(tp->rx_opt.rcv_tsecr, tp->retrans_stamp,
                             tcp_time_stamp)) {
                        NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_PAWSACTIVEREJECTED);
                        goto reset_and_undo;
                }
...

static inline void tcp_rcv_rtt_measure_ts(struct sock *sk,
                                          const struct sk_buff *skb)
{
        struct tcp_sock *tp = tcp_sk(sk);
        if (tp->rx_opt.rcv_tsecr &&
            (TCP_SKB_CB(skb)->end_seq -
             TCP_SKB_CB(skb)->seq >= inet_csk(sk)->icsk_ack.rcv_mss))
                tcp_rcv_rtt_update(tp, tcp_time_stamp - tp->rx_opt.rcv_tsecr, 0);
}
...
static inline int tcp_packet_delayed(struct tcp_sock *tp)
{
        return !tp->retrans_stamp ||
                (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr &&
                 before(tp->rx_opt.rcv_tsecr, tp->retrans_stamp));
}
...

So we dont have a bit saying we received a tsecr, we use the 
'if saw_tstamp AND tsecr is not null' convention...