From d838d2791979bb50f464a61c557d55c6a324621e Mon Sep 17 00:00:00 2001 From: Lally Singh Date: Wed, 26 Aug 2015 13:15:20 -0400 Subject: [PATCH] Added send-thresholding and fixed text packet dumping. Also a little squelch for the over-max-MTU log spam we see in there. BUG=https://code.google.com/p/webrtc/issues/detail?id=4468 R=pthatcher@chromium.org, pthatcher@webrtc.org Review URL: https://codereview.webrtc.org/1266033005 . Cr-Commit-Position: refs/heads/master@{#9788} --- talk/media/sctp/sctpdataengine.cc | 81 ++++++++++++++++++++-- talk/media/sctp/sctpdataengine.h | 13 +++- talk/media/sctp/sctpdataengine_unittest.cc | 23 ++++++ 3 files changed, 109 insertions(+), 8 deletions(-) diff --git a/talk/media/sctp/sctpdataengine.cc b/talk/media/sctp/sctpdataengine.cc index 8c8a6a192f..19cdddc426 100644 --- a/talk/media/sctp/sctpdataengine.cc +++ b/talk/media/sctp/sctpdataengine.cc @@ -109,6 +109,8 @@ typedef rtc::ScopedMessageData OutboundPacketMessage; // take off 80 bytes for DTLS/TURN/TCP/IP overhead. static const size_t kSctpMtu = 1200; +// The size of the SCTP association send buffer. 256kB, the usrsctp default. +static const int kSendBufferSize = 262144; enum { MSG_SCTPINBOUNDPACKET = 1, // MessageData is SctpInboundPacket MSG_SCTPOUTBOUNDPACKET = 2, // MessageData is rtc:Buffer @@ -177,11 +179,11 @@ static bool GetDataMediaType( } // Log the packet in text2pcap format, if log level is at LS_VERBOSE. -static void VerboseLogPacket(void *addr, size_t length, int direction) { +static void VerboseLogPacket(void *data, size_t length, int direction) { if (LOG_CHECK_LEVEL(LS_VERBOSE) && length > 0) { char *dump_buf; if ((dump_buf = usrsctp_dumppacket( - addr, length, direction)) != NULL) { + data, length, direction)) != NULL) { LOG(LS_VERBOSE) << dump_buf; usrsctp_freedumpbuffer(dump_buf); } @@ -244,6 +246,10 @@ static int OnSctpInboundPacket(struct socket* sock, union sctp_sockstore addr, // Set the initial value of the static SCTP Data Engines reference count. int SctpDataEngine::usrsctp_engines_count = 0; +// All the channels created by this engine, used for callbacks from +// usrsctplib that only contain socket pointers. Channels are removed when +// SignalDestroyed is fired. +std::vector SctpDataEngine::channels_; SctpDataEngine::SctpDataEngine() { if (usrsctp_engines_count == 0) { @@ -258,6 +264,11 @@ SctpDataEngine::SctpDataEngine() { // TODO(ldixon): Consider turning this on/off. usrsctp_sysctl_set_sctp_ecn_enable(0); + int send_size = usrsctp_sysctl_get_sctp_sendspace(); + if (send_size != kSendBufferSize) { + LOG(LS_ERROR) << "Got different send size than expected: " << send_size; + } + // TODO(ldixon): Consider turning this on/off. // This is not needed right now (we don't do dynamic address changes): // If SCTP Auto-ASCONF is enabled, the peer is informed automatically @@ -312,9 +323,48 @@ DataMediaChannel* SctpDataEngine::CreateChannel( if (data_channel_type != DCT_SCTP) { return NULL; } - return new SctpDataMediaChannel(rtc::Thread::Current()); + SctpDataMediaChannel *channel = new SctpDataMediaChannel( + rtc::Thread::Current()); + channels_.push_back(channel); + channel->SignalDestroyed.connect(this, &SctpDataEngine::OnChannelDestroyed); + return channel; } +// static +SctpDataMediaChannel* SctpDataEngine::GetChannelFromSocket( + struct socket* sock) { + for (auto p:channels_) { + if (p->socket() == sock) { + return p; + } + } + return 0; +} + + +void SctpDataEngine::OnChannelDestroyed(SctpDataMediaChannel* channel) { + auto it = std::find(channels_.begin(), channels_.end(), channel); + if (it == channels_.end()) { + LOG(LS_ERROR) << "OnChannelDestroyed: the channel wasn't registered."; + return; + } + channels_.erase(it); +} + +// static +int SctpDataEngine::SendThresholdCallback(struct socket* sock, + uint32_t sb_free) { + SctpDataMediaChannel *channel = GetChannelFromSocket(sock); + if (!channel) { + LOG(LS_ERROR) << "SendThresholdCallback: Failed to get channel for socket " + << sock; + return 0; + } + channel->SignalReadyToSend(true); + return 0; +} + + SctpDataMediaChannel::SctpDataMediaChannel(rtc::Thread* thread) : worker_thread_(thread), local_port_(kSctpDefaultPort), @@ -327,6 +377,7 @@ SctpDataMediaChannel::SctpDataMediaChannel(rtc::Thread* thread) SctpDataMediaChannel::~SctpDataMediaChannel() { CloseSctpSocket(); + SignalDestroyed(this); } sockaddr_conn SctpDataMediaChannel::GetSctpSockAddr(int port) { @@ -347,8 +398,16 @@ bool SctpDataMediaChannel::OpenSctpSocket() { << "->Ignoring attempt to re-create existing socket."; return false; } + + // If kSendBufferSize isn't reflective of reality, we log an error, but we + // still have to do something reasonable here. Look up what the buffer's + // real size is and set our threshold to something reasonable. + const static int send_threshold = usrsctp_sysctl_get_sctp_sendspace() / 2; + sock_ = usrsctp_socket(AF_CONN, SOCK_STREAM, IPPROTO_SCTP, - cricket::OnSctpInboundPacket, NULL, 0, this); + cricket::OnSctpInboundPacket, + &SctpDataEngine::SendThresholdCallback, + send_threshold, this); if (!sock_) { LOG_ERRNO(LS_ERROR) << debug_name_ << "Failed to create SCTP socket."; return false; @@ -393,7 +452,8 @@ bool SctpDataMediaChannel::OpenSctpSocket() { } // Disable MTU discovery - struct sctp_paddrparams params = {{0}}; + struct sctp_paddrparams params; + memset(¶ms, 0, sizeof(params)); params.spp_assoc_id = 0; params.spp_flags = SPP_PMTUD_DISABLE; params.spp_pathmtu = kSctpMtu; @@ -904,10 +964,17 @@ bool SctpDataMediaChannel::SetRecvCodecs(const std::vector& codecs) { void SctpDataMediaChannel::OnPacketFromSctpToNetwork( rtc::Buffer* buffer) { - if (buffer->size() > kSctpMtu) { + // usrsctp seems to interpret the MTU we give it strangely -- it seems to + // give us back packets bigger than that MTU, if only by a fixed amount. + // This is that amount that we've observed. + const int kSctpOverhead = 76; + if (buffer->size() > (kSctpOverhead + kSctpMtu)) { LOG(LS_ERROR) << debug_name_ << "->OnPacketFromSctpToNetwork(...): " << "SCTP seems to have made a packet that is bigger " - "than its official MTU."; + << "than its official MTU: " << buffer->size() + << " vs max of " << kSctpMtu + << " even after adding " << kSctpOverhead + << " extra SCTP overhead"; } MediaChannel::SendPacket(buffer); } diff --git a/talk/media/sctp/sctpdataengine.h b/talk/media/sctp/sctpdataengine.h index 86bfa37a48..a591542bd6 100644 --- a/talk/media/sctp/sctpdataengine.h +++ b/talk/media/sctp/sctpdataengine.h @@ -64,6 +64,8 @@ const uint32 kMaxSctpSid = 1023; // usrsctp.h) const int kSctpDefaultPort = 5000; +class SctpDataMediaChannel; + // A DataEngine that interacts with usrsctp. // // From channel calls, data flows like this: @@ -88,7 +90,7 @@ const int kSctpDefaultPort = 5000; // 14. SctpDataMediaChannel::SignalDataReceived(data) // [from the same thread, methods registered/connected to // SctpDataMediaChannel are called with the recieved data] -class SctpDataEngine : public DataEngineInterface { +class SctpDataEngine : public DataEngineInterface, public sigslot::has_slots<> { public: SctpDataEngine(); virtual ~SctpDataEngine(); @@ -97,9 +99,15 @@ class SctpDataEngine : public DataEngineInterface { virtual const std::vector& data_codecs() { return codecs_; } + static int SendThresholdCallback(struct socket* sock, uint32_t sb_free); + private: + static std::vector channels_; static int usrsctp_engines_count; std::vector codecs_; + + static SctpDataMediaChannel* GetChannelFromSocket(struct socket* sock); + void OnChannelDestroyed(SctpDataMediaChannel *channel); }; // TODO(ldixon): Make into a special type of TypedMessageData. @@ -188,6 +196,9 @@ class SctpDataMediaChannel : public DataMediaChannel, debug_name_ = debug_name; } const std::string& debug_name() const { return debug_name_; } + const struct socket* socket() { return sock_; } + + sigslot::signal1 SignalDestroyed; private: sockaddr_conn GetSctpSockAddr(int port); diff --git a/talk/media/sctp/sctpdataengine_unittest.cc b/talk/media/sctp/sctpdataengine_unittest.cc index 5b4c09e6a7..437253087d 100644 --- a/talk/media/sctp/sctpdataengine_unittest.cc +++ b/talk/media/sctp/sctpdataengine_unittest.cc @@ -240,10 +240,16 @@ class SctpDataMediaChannelTest : public testing::Test, net2_.reset(new SctpFakeNetworkInterface(rtc::Thread::Current())); recv1_.reset(new SctpFakeDataReceiver()); recv2_.reset(new SctpFakeDataReceiver()); + chan1_ready_to_send_count_ = 0; + chan2_ready_to_send_count_ = 0; chan1_.reset(CreateChannel(net1_.get(), recv1_.get())); chan1_->set_debug_name("chan1/connector"); + chan1_->SignalReadyToSend.connect( + this, &SctpDataMediaChannelTest::OnChan1ReadyToSend); chan2_.reset(CreateChannel(net2_.get(), recv2_.get())); chan2_->set_debug_name("chan2/listener"); + chan2_->SignalReadyToSend.connect( + this, &SctpDataMediaChannelTest::OnChan2ReadyToSend); // Setup two connected channels ready to send and receive. net1_->SetDestination(chan2_.get()); net2_->SetDestination(chan1_.get()); @@ -330,6 +336,8 @@ class SctpDataMediaChannelTest : public testing::Test, SctpFakeDataReceiver* receiver1() { return recv1_.get(); } SctpFakeDataReceiver* receiver2() { return recv2_.get(); } + int channel1_ready_to_send_count() { return chan1_ready_to_send_count_; } + int channel2_ready_to_send_count() { return chan2_ready_to_send_count_; } private: rtc::scoped_ptr engine_; rtc::scoped_ptr net1_; @@ -338,6 +346,12 @@ class SctpDataMediaChannelTest : public testing::Test, rtc::scoped_ptr recv2_; rtc::scoped_ptr chan1_; rtc::scoped_ptr chan2_; + + int chan1_ready_to_send_count_; + int chan2_ready_to_send_count_; + + void OnChan1ReadyToSend(bool send) { if (send) chan1_ready_to_send_count_++; } + void OnChan2ReadyToSend(bool send) { if (send) chan2_ready_to_send_count_++; } }; // Verifies that SignalReadyToSend is fired. @@ -486,6 +500,15 @@ TEST_F(SctpDataMediaChannelTest, ClosesStreamsOnBothSides) { EXPECT_TRUE_WAIT(chan_1_sig_receiver.WasStreamClosed(4), 1000); } +TEST_F(SctpDataMediaChannelTest, EngineSignalsRightChannel) { + SetupConnectedChannels(); + EXPECT_TRUE_WAIT(channel1()->socket() != NULL, 1000); + struct socket *sock = const_cast(channel1()->socket()); + int prior_count = channel1_ready_to_send_count(); + cricket::SctpDataEngine::SendThresholdCallback(sock, 0); + EXPECT_GT(channel1_ready_to_send_count(), prior_count); +} + // Flaky on Linux and Windows. See webrtc:4453. #if defined(WEBRTC_WIN) || defined(WEBRTC_LINUX) #define MAYBE_ReusesAStream DISABLED_ReusesAStream