Re: [quicwg/base-drafts] Add details of marking packets lost on PTO (#2624)

ianswett <notifications@github.com> Tue, 16 April 2019 15:57 UTC

Return-Path: <noreply@github.com>
X-Original-To: quic-issues@ietfa.amsl.com
Delivered-To: quic-issues@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id EE73812098D for <quic-issues@ietfa.amsl.com>; Tue, 16 Apr 2019 08:57:02 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -8.001
X-Spam-Level:
X-Spam-Status: No, score=-8.001 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=0.001, MAILING_LIST_MULTI=-1, RCVD_IN_DNSWL_HI=-5, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=github.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id PJ2nwvPMlSeI for <quic-issues@ietfa.amsl.com>; Tue, 16 Apr 2019 08:57:00 -0700 (PDT)
Received: from out-6.smtp.github.com (out-6.smtp.github.com [192.30.252.197]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 60731120C52 for <quic-issues@ietf.org>; Tue, 16 Apr 2019 07:57:15 -0700 (PDT)
Date: Tue, 16 Apr 2019 07:57:12 -0700
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1555426632; bh=eSNmxR7c2349aUVa20LhmQnZZew6zfkPIKrUcbKpYYU=; h=Date:From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=t8SyWUtQHoAEBcHzteNe+yAYxBxe6oVoZPwlXtzeCU6V5wkCcywtq7EgBb3xI4l7g iTlChDg/QClVE4v8ZESt91D2Jl0Bu92ctpwYqKpKRt2V0ntfSkidVAOjK75+qJLZWn f2YxMe2llT+zXalzQjXN2WF8XpkoISxA98z/0Cxo=
From: ianswett <notifications@github.com>
Reply-To: quicwg/base-drafts <reply+0166e4ab203f0e008ba5137ba53e0f2514440d1674dde8df92cebac31fc892a169ce19d6a975@reply.github.com>
To: quicwg/base-drafts <base-drafts@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <quicwg/base-drafts/pull/2624/review/227249612@github.com>
In-Reply-To: <quicwg/base-drafts/pull/2624@github.com>
References: <quicwg/base-drafts/pull/2624@github.com>
Subject: Re: [quicwg/base-drafts] Add details of marking packets lost on PTO (#2624)
Mime-Version: 1.0
Content-Type: multipart/alternative; boundary="--==_mimepart_5cb5ed489db2e_5c903fbbee6d45b8258143"; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: list
X-GitHub-Sender: ianswett
X-GitHub-Recipient: quic-issues
X-GitHub-Reason: subscribed
X-Auto-Response-Suppress: All
X-GitHub-Recipient-Address: quic-issues@ietf.org
Archived-At: <https://mailarchive.ietf.org/arch/msg/quic-issues/3qzp0aOnZMM4rQhyNZA4_Xe79fE>
X-BeenThere: quic-issues@ietf.org
X-Mailman-Version: 2.1.29
List-Id: Notification list for GitHub issues related to the QUIC WG <quic-issues.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/quic-issues/>
List-Post: <mailto:quic-issues@ietf.org>
List-Help: <mailto:quic-issues-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 16 Apr 2019 15:57:07 -0000

ianswett requested changes on this pull request.

LG overall, but I think the example needs to be fixed, unfortunately.

> @@ -623,10 +623,21 @@ time.
 
 ### Sending Probe Packets
 
-When a PTO timer expires, the sender MUST send one ack-eliciting packet as a
-probe, unless there is nothing to send. A sender MAY send up to two
-ack-eliciting packets, to avoid an expensive consecutive PTO expiration due
-to a single packet loss.
+When a PTO timer expires, a sender MUST send at least one ack-eliciting packet
+as a probe, unless there is no data available to send.  An endpoint MAY send up
+to two ack-eliciting packets, to avoid an expensive consecutive PTO expiration
+due to a single packet loss.
+
+It is possible that the sender has no new or previously-sent data to send.  For
+example, this can happen when there is no new application data to send and all
+streams with data in flight are reset (see Section 13.2 of {{QUIC-TRANSPORT}}).
+Under these conditions, the sender SHOULD send PING or other ack-eliciting
+frames in up to two packets, re-arming the PTO timer.

Why send two packets in the case when you're just trying to establish loss?  I'd specifically say 1 packet for this case.

> -When a PTO timer expires, the sender MUST send one ack-eliciting packet as a
-probe, unless there is nothing to send. A sender MAY send up to two
-ack-eliciting packets, to avoid an expensive consecutive PTO expiration due
-to a single packet loss.
+When a PTO timer expires, a sender MUST send at least one ack-eliciting packet
+as a probe, unless there is no data available to send.  An endpoint MAY send up
+to two ack-eliciting packets, to avoid an expensive consecutive PTO expiration
+due to a single packet loss.
+
+It is possible that the sender has no new or previously-sent data to send.  For
+example, this can happen when there is no new application data to send and all
+streams with data in flight are reset (see Section 13.2 of {{QUIC-TRANSPORT}}).
+Under these conditions, the sender SHOULD send PING or other ack-eliciting
+frames in up to two packets, re-arming the PTO timer.
+
+When there is no data to send, the sender MAY mark any packets still in flight

I'd like to be clear that this is an alternative, so maybe say "Alternatively, when there is..."?

> @@ -1242,7 +1246,8 @@ OnLossDetectionTimeout():
        SendOnePaddedInitialPacket()
     crypto_count++
   else:
-    // PTO
+    // PTO. Send new data if available, else retransmit old data.
+    // If neither is available, send PING frames.

```suggestion
    // If neither is available, send a PING frame.
```

> @@ -1455,8 +1460,7 @@ Invoked when an ACK frame with an ECN section is received from the peer.
 
 ## On Packets Lost
 
-Invoked by loss detection from DetectLostPackets when new packets
-are detected lost.
+Invoked when packets are deemed lost.

Is it worth specifying "from DetectLostPackets" still?

> @@ -623,10 +623,21 @@ time.
 
 ### Sending Probe Packets
 
-When a PTO timer expires, the sender MUST send one ack-eliciting packet as a
-probe, unless there is nothing to send. A sender MAY send up to two
-ack-eliciting packets, to avoid an expensive consecutive PTO expiration due
-to a single packet loss.
+When a PTO timer expires, a sender MUST send at least one ack-eliciting packet
+as a probe, unless there is no data available to send.  An endpoint MAY send up
+to two ack-eliciting packets, to avoid an expensive consecutive PTO expiration
+due to a single packet loss.
+
+It is possible that the sender has no new or previously-sent data to send.  For
+example, this can happen when there is no new application data to send and all
+streams with data in flight are reset (see Section 13.2 of {{QUIC-TRANSPORT}}).

Because of the reset issue, I think another example, such as Nick's, would be best.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2624#pullrequestreview-227249612