Return-Path: 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 95079130E25 for ; Wed, 28 Nov 2018 16:43:37 -0800 (PST) X-Virus-Scanned: amavisd-new at amsl.com X-Spam-Flag: NO X-Spam-Score: -9.46 X-Spam-Level: X-Spam-Status: No, score=-9.46 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_HIGH=-1.46, 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 9qZEks2zN4IV for ; Wed, 28 Nov 2018 16:43:35 -0800 (PST) Received: from out-7.smtp.github.com (out-7.smtp.github.com [192.30.252.198]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 96442130DD7 for ; Wed, 28 Nov 2018 16:43:35 -0800 (PST) Date: Wed, 28 Nov 2018 16:43:34 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1543452214; bh=RRafCNyWoJ+4HXd64iHFmvCGpcVBr5tqKlqflBJuBmw=; h=Date:From:Reply-To:To:Cc:Subject:List-ID:List-Archive:List-Post: List-Unsubscribe:From; b=FgSsxyXrmnwYmjm8gnZbczGUKohJTvNO/5+Xpjm+Q8aeqxB8Zfx8kXMb8rbc3Btfp P+zrqvEOUmK8wkadpmNXxsNJH448mLYM90lK0JCrJ/j4rAp0fY4KUxzG+Sztdet05O V565Ssj7aFfCFOE2QfbHb7alL70wjUGhMrn+vhWo= From: Martin Thomson Reply-To: quicwg/base-drafts To: quicwg/base-drafts Cc: Subscribed Message-ID: Subject: [quicwg/base-drafts] Refactor DetectLostPackets (#2066) Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="--==_mimepart_5bff3636815fe_66f53fb08b0d45b42261e2"; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: list X-GitHub-Sender: martinthomson X-GitHub-Recipient: quic-issues X-GitHub-Reason: subscribed X-Auto-Response-Suppress: All X-GitHub-Recipient-Address: quic-issues@ietf.org Archived-At: X-BeenThere: quic-issues@ietf.org X-Mailman-Version: 2.1.29 List-Id: Notification list for GitHub issues related to the QUIC WG List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Nov 2018 00:43:37 -0000 ----==_mimepart_5bff3636815fe_66f53fb08b0d45b42261e2 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit This had a couple of problems that I think this addresses, and some that it doesn't. The first is that it isn't clear what is being iterated over and in what order. I think that the point here is to iterate over sent_packets, starting with the oldest. Correct me if that is wrong. In any case, this change doesn't assume an iteration order. The only reason the iteration order is important is in setting loss_time, which needs to be the earliest time (assuming that I'm right). This simplifies the function, by setting thresholds at the top and doing a simple comparison. I've added a note about loss_time potentially being in the past. The problem that remains is that this appeared to iterate only over packets that have a packet number less than the largest acknowledged. I've added that condition to the loop, but I don't think that it's right. I think that it's just redundant - and while an implementation might stop its iteration at the largest acknowledged to save cycles, this function will operate the same without the extra check. It's also not clear whether the greater than comparisons here were correct. If you assume that firing of the timer cannot take zero time, this is never an issue, but with discrete intervals on time values, that's not always going to happen. As setup, this code could be called at exactly loss_time for a packet, in which case that packet will not be declared lost. I think that this wants >= in the time comparison for that reason. Finally, should the early retransmit timer be configurable? Should it be set based on kTimeReorderingThreshold? You can view, comment on, or merge this pull request online at: https://github.com/quicwg/base-drafts/pull/2066 -- Commit Summary -- * Refactor DetectLostPackets -- File Changes -- M draft-ietf-quic-recovery.md (35) -- Patch Links -- https://github.com/quicwg/base-drafts/pull/2066.patch https://github.com/quicwg/base-drafts/pull/2066.diff -- 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/2066 ----==_mimepart_5bff3636815fe_66f53fb08b0d45b42261e2 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

This had a couple of problems that I think this addresses, and some that
it doesn't.

The first is that it isn't clear what is being iterated over and in what
order. I think that the point here is to iterate over sent_packets,
starting with the oldest. Correct me if that is wrong. In any case,
this change doesn't assume an iteration order. The only reason the
iteration order is important is in setting loss_time, which needs to be
the earliest time (assuming that I'm right).

This simplifies the function, by setting thresholds at the top and doing
a simple comparison.

I've added a note about loss_time potentially being in the past.

The problem that remains is that this appeared to iterate only over
packets that have a packet number less than the largest acknowledged.
I've added that condition to the loop, but I don't think that it's
right. I think that it's just redundant - and while an implementation
might stop its iteration at the largest acknowledged to save cycles,
this function will operate the same without the extra check.

It's also not clear whether the greater than comparisons here were
correct. If you assume that firing of the timer cannot take zero time,
this is never an issue, but with discrete intervals on time values,
that's not always going to happen. As setup, this code could be called
at exactly loss_time for a packet, in which case that packet will not be
declared lost. I think that this wants >= in the time comparison for
that reason.

Finally, should the early retransmit timer be configurable? Should it
be set based on kTimeReorderingThreshold?

You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/2066

Commit Summary

  • Refactor DetectLostPackets

File Changes

Patch Links:

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

----==_mimepart_5bff3636815fe_66f53fb08b0d45b42261e2--