From 3a45d32d4e6982fbfb3927e43cf607af2d826a4b Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Wed, 19 May 2021 13:40:55 +0200 Subject: [PATCH] dcsctp: Report duplicate TSNs Reporting the duplicate TSNs is a SHOULD in the RFC, and using the duplicate TNSs is a MAY, and in reality I haven't seen an implementation use it yet. However, it's good for debugging and for stats generation. Bug: webrtc:12614 Change-Id: I1cc3f86961a8d289708cbf50d98dedfd25077955 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219462 Reviewed-by: Florent Castelli Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/master@{#34053} --- net/dcsctp/packet/chunk/sack_chunk.cc | 9 +++---- net/dcsctp/packet/chunk/sack_chunk.h | 7 ++--- net/dcsctp/packet/chunk_validators.cc | 7 ++--- net/dcsctp/rx/data_tracker.cc | 18 ++++++------- net/dcsctp/rx/data_tracker.h | 2 +- net/dcsctp/rx/data_tracker_test.cc | 39 +++++++++++++++++++++++++++ 6 files changed, 59 insertions(+), 23 deletions(-) diff --git a/net/dcsctp/packet/chunk/sack_chunk.cc b/net/dcsctp/packet/chunk/sack_chunk.cc index a9f17d79dd..d80e430082 100644 --- a/net/dcsctp/packet/chunk/sack_chunk.cc +++ b/net/dcsctp/packet/chunk/sack_chunk.cc @@ -88,13 +88,12 @@ absl::optional SackChunk::Parse(rtc::ArrayView data) { offset += kGapAckBlockSize; } - std::vector duplicate_tsns; - duplicate_tsns.reserve(nbr_of_gap_blocks); + std::set duplicate_tsns; for (int i = 0; i < nbr_of_dup_tsns; ++i) { BoundedByteReader sub_reader = reader->sub_reader(offset); - duplicate_tsns.push_back(TSN(sub_reader.Load32<0>())); + duplicate_tsns.insert(TSN(sub_reader.Load32<0>())); offset += kDupTsnBlockSize; } RTC_DCHECK(offset == reader->variable_data_size()); @@ -124,11 +123,11 @@ void SackChunk::SerializeTo(std::vector& out) const { offset += kGapAckBlockSize; } - for (int i = 0; i < nbr_of_dup_tsns; ++i) { + for (TSN tsn : duplicate_tsns_) { BoundedByteWriter sub_writer = writer.sub_writer(offset); - sub_writer.Store32<0>(*duplicate_tsns_[i]); + sub_writer.Store32<0>(*tsn); offset += kDupTsnBlockSize; } diff --git a/net/dcsctp/packet/chunk/sack_chunk.h b/net/dcsctp/packet/chunk/sack_chunk.h index 0b464fb359..e6758fa332 100644 --- a/net/dcsctp/packet/chunk/sack_chunk.h +++ b/net/dcsctp/packet/chunk/sack_chunk.h @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -48,7 +49,7 @@ class SackChunk : public Chunk, public TLVTrait { SackChunk(TSN cumulative_tsn_ack, uint32_t a_rwnd, std::vector gap_ack_blocks, - std::vector duplicate_tsns) + std::set duplicate_tsns) : cumulative_tsn_ack_(cumulative_tsn_ack), a_rwnd_(a_rwnd), gap_ack_blocks_(std::move(gap_ack_blocks)), @@ -63,7 +64,7 @@ class SackChunk : public Chunk, public TLVTrait { rtc::ArrayView gap_ack_blocks() const { return gap_ack_blocks_; } - rtc::ArrayView duplicate_tsns() const { return duplicate_tsns_; } + const std::set& duplicate_tsns() const { return duplicate_tsns_; } private: static constexpr size_t kGapAckBlockSize = 4; @@ -72,7 +73,7 @@ class SackChunk : public Chunk, public TLVTrait { const TSN cumulative_tsn_ack_; const uint32_t a_rwnd_; std::vector gap_ack_blocks_; - std::vector duplicate_tsns_; + std::set duplicate_tsns_; }; } // namespace dcsctp diff --git a/net/dcsctp/packet/chunk_validators.cc b/net/dcsctp/packet/chunk_validators.cc index b3467037c7..48d351827e 100644 --- a/net/dcsctp/packet/chunk_validators.cc +++ b/net/dcsctp/packet/chunk_validators.cc @@ -38,9 +38,7 @@ SackChunk ChunkValidators::Clean(SackChunk&& sack) { // Not more than at most one remaining? Exit early. if (gap_ack_blocks.size() <= 1) { return SackChunk(sack.cumulative_tsn_ack(), sack.a_rwnd(), - std::move(gap_ack_blocks), - std::vector(sack.duplicate_tsns().begin(), - sack.duplicate_tsns().end())); + std::move(gap_ack_blocks), sack.duplicate_tsns()); } // Sort the intervals by their start value, to aid in the merging below. @@ -63,8 +61,7 @@ SackChunk ChunkValidators::Clean(SackChunk&& sack) { } return SackChunk(sack.cumulative_tsn_ack(), sack.a_rwnd(), std::move(merged), - std::vector(sack.duplicate_tsns().begin(), - sack.duplicate_tsns().end())); + sack.duplicate_tsns()); } bool ChunkValidators::Validate(const SackChunk& sack) { diff --git a/net/dcsctp/rx/data_tracker.cc b/net/dcsctp/rx/data_tracker.cc index 3e03dfece2..68a4895ec8 100644 --- a/net/dcsctp/rx/data_tracker.cc +++ b/net/dcsctp/rx/data_tracker.cc @@ -52,7 +52,7 @@ void DataTracker::Observe(TSN tsn, // Old chunk already seen before? if (unwrapped_tsn <= last_cumulative_acked_tsn_) { - // TODO(boivie) Set duplicate TSN, even if it's not used in SCTP yet. + duplicate_tsns_.insert(unwrapped_tsn.Wrap()); return; } @@ -66,7 +66,11 @@ void DataTracker::Observe(TSN tsn, additional_tsns_.erase(additional_tsns_.begin()); } } else { - additional_tsns_.insert(unwrapped_tsn); + bool inserted = additional_tsns_.insert(unwrapped_tsn).second; + if (!inserted) { + // Already seen before. + duplicate_tsns_.insert(unwrapped_tsn.Wrap()); + } } // https://tools.ietf.org/html/rfc4960#section-6.7 @@ -178,15 +182,11 @@ SackChunk DataTracker::CreateSelectiveAck(size_t a_rwnd) { // that. So this SACK produced is more like a NR-SACK as explained in // https://ieeexplore.ieee.org/document/4697037 and which there is an RFC // draft at https://tools.ietf.org/html/draft-tuexen-tsvwg-sctp-multipath-17. - std::vector duplicate_tsns; - duplicate_tsns.reserve(duplicates_.size()); - for (UnwrappedTSN tsn : duplicates_) { - duplicate_tsns.push_back(tsn.Wrap()); - } - duplicates_.clear(); + std::set duplicate_tsns; + duplicate_tsns_.swap(duplicate_tsns); return SackChunk(last_cumulative_acked_tsn_.Wrap(), a_rwnd, - CreateGapAckBlocks(), duplicate_tsns); + CreateGapAckBlocks(), std::move(duplicate_tsns)); } std::vector DataTracker::CreateGapAckBlocks() const { diff --git a/net/dcsctp/rx/data_tracker.h b/net/dcsctp/rx/data_tracker.h index 6146d2a839..f5deaf147d 100644 --- a/net/dcsctp/rx/data_tracker.h +++ b/net/dcsctp/rx/data_tracker.h @@ -126,7 +126,7 @@ class DataTracker { UnwrappedTSN last_cumulative_acked_tsn_; // Received TSNs that are not directly following `last_cumulative_acked_tsn_`. std::set additional_tsns_; - std::set duplicates_; + std::set duplicate_tsns_; }; } // namespace dcsctp diff --git a/net/dcsctp/rx/data_tracker_test.cc b/net/dcsctp/rx/data_tracker_test.cc index d714b0ba9e..7518d6dc91 100644 --- a/net/dcsctp/rx/data_tracker_test.cc +++ b/net/dcsctp/rx/data_tracker_test.cc @@ -25,6 +25,7 @@ namespace dcsctp { namespace { using ::testing::ElementsAre; using ::testing::IsEmpty; +using ::testing::UnorderedElementsAre; constexpr size_t kArwnd = 10000; constexpr TSN kInitialTSN(11); @@ -230,5 +231,43 @@ TEST_F(DataTrackerTest, WillNotAcceptInvalidTSNs) { EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn - 0x8000000))); } +TEST_F(DataTrackerTest, ReportSingleDuplicateTsns) { + Observer({11, 12, 11}); + SackChunk sack = buf_.CreateSelectiveAck(kArwnd); + EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(12)); + EXPECT_THAT(sack.gap_ack_blocks(), IsEmpty()); + EXPECT_THAT(sack.duplicate_tsns(), UnorderedElementsAre(TSN(11))); +} + +TEST_F(DataTrackerTest, ReportMultipleDuplicateTsns) { + Observer({11, 12, 13, 14, 12, 13, 12, 13, 15, 16}); + SackChunk sack = buf_.CreateSelectiveAck(kArwnd); + EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(16)); + EXPECT_THAT(sack.gap_ack_blocks(), IsEmpty()); + EXPECT_THAT(sack.duplicate_tsns(), UnorderedElementsAre(TSN(12), TSN(13))); +} + +TEST_F(DataTrackerTest, ReportDuplicateTsnsInGapAckBlocks) { + Observer({11, /*12,*/ 13, 14, 13, 14, 15, 16}); + SackChunk sack = buf_.CreateSelectiveAck(kArwnd); + EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(11)); + EXPECT_THAT(sack.gap_ack_blocks(), ElementsAre(SackChunk::GapAckBlock(2, 5))); + EXPECT_THAT(sack.duplicate_tsns(), UnorderedElementsAre(TSN(13), TSN(14))); +} + +TEST_F(DataTrackerTest, ClearsDuplicateTsnsAfterCreatingSack) { + Observer({11, 12, 13, 14, 12, 13, 12, 13, 15, 16}); + SackChunk sack1 = buf_.CreateSelectiveAck(kArwnd); + EXPECT_EQ(sack1.cumulative_tsn_ack(), TSN(16)); + EXPECT_THAT(sack1.gap_ack_blocks(), IsEmpty()); + EXPECT_THAT(sack1.duplicate_tsns(), UnorderedElementsAre(TSN(12), TSN(13))); + + Observer({17}); + SackChunk sack2 = buf_.CreateSelectiveAck(kArwnd); + EXPECT_EQ(sack2.cumulative_tsn_ack(), TSN(17)); + EXPECT_THAT(sack2.gap_ack_blocks(), IsEmpty()); + EXPECT_THAT(sack2.duplicate_tsns(), IsEmpty()); +} + } // namespace } // namespace dcsctp