Message-ID: <CACJspmK89b2sMzeBhK1Xs-hkN3aZyr4f24FwN8qSh9i=59faYQ@mail.gmail.com>
Date:   Tue, 2 Jan 2018 15:57:41 -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,

Sorry, my last email was incorrect. It turns out the default tcp
congestion control alg that was being used on my client machines was
cubic instead of dctcp. That is why tp->processing_cwr field was never
set in the tcp_rcv_established function. I've changed the default back
to dctcp on all of my machines.

I am now logging the value of tp->rcv_nxt at the top of the
tcp_transmit_skb() function for all CWR segments. I see that during
normal operation, the value of tp->rcv_nxt is equal to the SeqNo in
the CWR segment  + length of the CWR segment. However, for the unACKed
CWR segment, the value of tp->rcv_nxt is just equal to the SeqNo in
the CWR segment (i.e. not incremented by the length). And I see that
by the time the tcp_ack_snd_check() function is executed, tp->rcv_nxt
has been incremented by the length of the unACKed CWR segment.

The tcp_transmit_skb() function sets the outgoing segment's ack_seq to
be tp->rcv_next:

th->ack_seq             = htonl(tp->rcv_nxt);

So I think the rcv_nxt field is supposed to be incremented before
reaching tcp_transmit_skb(). Can you see any reason as to why this
field would not be incremented for CWR segments sometimes?

Thanks,
-Steve


On Tue, Jan 2, 2018 at 8:27 AM, Neal Cardwell <ncardwell@...gle.com> wrote:
> On Tue, Jan 2, 2018 at 2:43 AM, Steve Ibanez <sibanez@...nford.edu> wrote:
>> 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?
>
> Happy new year to you as well, and thank you, Steve, for running this
> experiment! Yes, this is basically the kind of thing I had in mind.
>
> The connection will run the "fast path" tcp_rcv_established() code if
> the connection is in the ESTABLISHED state.  From the symptoms it
> sounds like what's happening is that in this test the connection is
> not in the ESTABLISHED state when the CWR arrives, so it's probably
> running the more general tcp_rcv_state_process() function. I would
> suggest adding your tp->processing_cwr instrumentation at the top of
> tcp_rcv_state_process() as well, and then re-running the test. (In
> tcp_v4_do_rcv() and tcp_v6_do_rcv(), for each incoming skb one of
> those two functions is called).
>
> It is interesting that the connection does not seem to be in the
> ESTABLISHED state. Maybe that is an ingredient of the unexpected
> behavior in this case...
>
> Thanks!
> neal

Powered by blists - more mailing lists