Message-ID: <CACJspmLiYAG6Z7Zzn+J_DQ=QVs=zXp8ttBihvO8LHDsassapXg@mail.gmail.com>
Date:   Mon, 1 Jan 2018 23:43:36 -0800
From:   Steve Ibanez <sibanez@...nford.edu>
To:     Neal Cardwell <ncardwell@...gle.com>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        Yuchung Cheng <ycheng@...gle.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Netdev <netdev@...r.kernel.org>, Florian Westphal <fw@...len.de>,
        Mohammad Alizadeh <alizadeh@...il.mit.edu>,
        Lawrence Brakmo <brakmo@...com>
Subject: Re: Linux ECN Handling

Hi Neal,

Apologies for the delay, and happy new year!

To answer your question, data is only transferred in one direction
(from the client to the server). The SeqNo in the pkts from the server
to the client is not incremented. So I don't think that a data pkt is
attempted to be sent in the tcp_data_snd_check() call clearing the
ICSK_ACK_SCHED bit. Although I think it would be helpful to include
your new debugging field in the tcp_sock (tp->processing_cwr) so that
I can check this field in the tcp_transmit_skb() and tcp_send_ack()
functions. I added the new field and tried to set it at the top of the
tcp_rcv_established(), but then when I try to check the field in the
tcp_send_ack() function it never appears to be set. Below I'm showing
how I set the tp->processing_cwr field in the tcp_rcv_established
function and how I check it in the tcp_send_ack function. Is this how
you were imagining the processing_cwr field to be used?

void tcp_rcv_established(struct sock *sk, struct sk_buff *skb,
                         const struct tcphdr *th, unsigned int len)
 {
        struct tcp_sock *tp = tcp_sk(sk);
+        struct inet_sock *inet = inet_sk(sk);
+
+        // SI: Debugging TCP ECN handeling
+        if (tcp_hdr(skb)->cwr) {
+               tp->processing_cwr = 1;
+        } else {
+               tp->processing_cwr = 0;
+       }

        tcp_mstamp_refresh(tp);
        if (unlikely(!sk->sk_rx_dst))


 void tcp_send_ack(struct sock *sk)
 {
        struct sk_buff *buff;
+        // SI: Debugging TCP ECN haneling
+        const struct tcp_sock *tp = tcp_sk(sk);
+        struct inet_sock *inet = inet_sk(sk);
+
+        // SI: Debugging TCP ECN handeling
+        if ((sk->sk_family == AF_INET) && tp->processing_cwr) {
+                printk("tcp_debug: tcp_send_ack: %pI4/%u  - CWR set
and rcv_nxt=%u, snd_una=%u\n",
+                         &inet->inet_daddr, ntohs(inet->inet_sport),
tp->rcv_nxt, tp->snd_una);
+        }

        /* If we have been reset, we may not send again. */
        if (sk->sk_state == TCP_CLOSE)

Thanks!
-Steve


On Wed, Dec 20, 2017 at 12:17 PM, Neal Cardwell <ncardwell@...gle.com> wrote:
> On Wed, Dec 20, 2017 at 2:20 PM, Steve Ibanez <sibanez@...nford.edu> wrote:
>>
>> Hi Neal,
>>
>> I added in some more printk statements and it does indeed look like
>> all of these calls you listed are being invoked successfully. I guess
>> this isn't too surprising given what the inet_csk_schedule_ack() and
>> inet_csk_ack_scheduled() functions are doing:
>>
>> static inline void inet_csk_schedule_ack(struct sock *sk)
>> {
>>         inet_csk(sk)->icsk_ack.pending |= ICSK_ACK_SCHED;
>> }
>>
>> static inline int inet_csk_ack_scheduled(const struct sock *sk)
>> {
>>         return inet_csk(sk)->icsk_ack.pending & ICSK_ACK_SCHED;
>> }
>>
>> So through the code path that you listed, the inet_csk_schedule_ack()
>> function sets the ICSK_ACK_SCHED bit and then the tcp_ack_snd_check()
>> function just checks that the ICSK_ACK_SCHED bit is indeed set.
>>inet_csk_schedule_ack
>> Do you know how I can verify that setting the ICSK_ACK_SCHED bit
>> actually results in an ACK being sent?
>
> Hmm. I don't think in this case we can verify that setting the
> ICSK_ACK_SCHED bit actually results in an ACK being sent. Because
> AFAICT in this case it seems like an ACK is not sent. :-) This is
> based on both the tcpdumps on Dec 5 and your detective work yesterday
> ("The tcp_rcv_established() function calls tcp_ack_snd_check() at the
> end of step5 and then the return statement indicated below is invoked,
> which prevents the __tcp_ack_snd_check() function from running.")
>
> So AFAICT the puzzle is: how is the icsk_ack.pending  ICSK_ACK_SCHED
> bit being cleared between the inet_csk_schedule_ack() call and the
> tcp_ack_snd_check() call, without (apparently) an actual ACK being
> sent on the wire?
>
> AFAICT the ICSK_ACK_SCHED bit is not supposed to be cleared unless we
> get to this sequence:
>
> tcp_transmit_skb()
>   if (likely(tcb->tcp_flags & TCPHDR_ACK))
>     tcp_event_ack_sent(sk, tcp_skb_pcount(skb));
>      -> inet_csk_clear_xmit_timer(sk, ICSK_TIME_DACK);
>             icsk->icsk_ack.blocked = icsk->icsk_ack.pending = 0;
>
> I don't have a theory that fits all of those data points, unless this
> is a bi-directional transfer (is it?) and between the
> inet_csk_schedule_ack() call and the tcp_ack_snd_check() call the TCP
> connection sends a data packet (in tcp_data_snd_check()) and then it
> is dropped for some reason before the packet make it to the tcpdump
> sniffing point. Perhaps because of a qdisc limit or something?
>
> I guess a possible next step would be, while processing an incoming
> skb with the cwr bit set, the code could set a new debugging field in
> the tcp_sock (tp->processing_cwr), and then you could check this field
> in tcp_transmit_skb() and printk if (1) there is an attempted
> queue_xmit() cal and (2) if the queue_xmit() fails (the err > 0 case).
>
> That's a long shot, but the only idea I have at this point.
>
> thanks,
> neal

Powered by blists - more mailing lists