Message-ID: <CADVnQy=Dw__BrTHwoUrn0Wxu8=Xb7NqZ9bAuEeM8SrNhzd_2SA@mail.gmail.com>
Date:   Fri, 28 Jul 2017 18:48:24 -0400
From:   Neal Cardwell <ncardwell@...gle.com>
To:     Mao Wenan <maowenan@...wei.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Yuchung Cheng <ycheng@...gle.com>,
        Nandita Dukkipati <nanditad@...gle.com>,
        weiyongjun1@...wei.com, Weilong Chen <chenweilong@...wei.com>,
        wangkefeng.wang@...wei.com
Subject: Re: [PATCH V3 net-next] TLP: Don't reschedule PTO when there's one
 outstanding TLP retransmission

On Thu, Jul 27, 2017 at 8:08 AM, Mao Wenan <maowenan@...wei.com> wrote:
> If there is one TLP probe went out(TLP use the write_queue_tail
> packet as TLP probe, we assume this first TLP probe named A), and
> this TLP probe was not acked by receive side.
>
> Then the transmit side sent the next two packetes out(named B,C),
> but unfortunately these two packets are also not acked by receive side.
>
> And then there is one data packet with ack_seq A arrive at transmit
> side, in tcp_ack() will call tcp_schedule_loss_probe() to rearm PTO,
> the handler tcp_send_loss_probe() is to check
> if(tp->tlp_high_seq) then go to rearm_timer(because there is one
> outstanding TLP named A), so the new TLP probe can't be sent out and
> it needs to rearm the RTO timer(timeout is relative to the transmit
> time of the write queue head).
>
> After that, there is another data packet with ack_seq A is received,
> if the tlp_time_stamp is greater than rto_time_stamp, it will reset
> the TLP timeout, which is before previous RTO timeout, so PTO is
> rearm and previous RTO is cleared. Because there is no
> retransmission packet was sent or no TLP sack receive,
> tp->tlp_high_seq can't be reset to zero and the next TLP probe also
> can't be sent out, so there is no way(or very long time)
> to retransmit the lost packet.
>
> This fix is to check(tp->tlp_high_seq) in tcp_schedule_loss_probe()
> when TLP PTO is after RTO, It is not needed to reschedule PTO when
> there is one outstanding TLP retransmission, so if the TLP A is lost
> RTO can retransmit lost packet, then tp->tlp_high_seq will be set to
> 0, and TLP will go to the normal work process.
>
> v1->v2
>         refine some words of code and patch comments.
> v2->v3
>         delete senseless "{" and "}" in if clause.
>
> Signed-off-by: Mao Wenan <maowenan@...wei.com>

Thanks for posting this patch with a detailed problem description, as
well as a trace in the thread for v1 of the patch. This was very
helpful!

Thinking about the problem you describe, and looking at the trace,
AFAICT I don't think this is the patch we want.

We can still have this problem of improperly/repeatedly rescheduling a
PTO even when the TLPs are new data. When the TLPs are new data
tp->tlp_high_seq is not set, and so the patch above will not help.

I think the broader problem is hinted at in this part of your commit
description:

> After that, there is another data packet with ack_seq A is received,
> if the tlp_time_stamp is greater than rto_time_stamp, it will reset
> the TLP timeout

The broader problem here is that an incoming data packet (with no new
ACK/SACK info) affected the TLP for our outbound data. That is a
problem because such incoming data can cause us to delay the TLP when
there is no reason to.

I think this is basically the same as the TLP issue from the "TCP fast
retransmit issues" thread on netdev from July 26. Our TCP team at
Google has a proposed fix for this more general issue that we have
tested and reviewed. I will post a quick summary of the proposed patch
in the "TCP fast retransmit issues" thread. Once the patch has
undergone a little more testing we will send it to the list, hopefully
next week.

Thanks!
neal

Powered by blists - more mailing lists