Added send-thresholding and fixed text packet dumping.

talk/media/sctp/sctpdataengine.cc

 /*
  * libjingle
  * Copyright 2012 Google Inc. and Robin Seggelmann
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 91 matching lines @@
 }  // namespace
 
 namespace cricket {
 typedef rtc::ScopedMessageData<SctpInboundPacket> InboundPacketMessage;
 typedef rtc::ScopedMessageData<rtc::Buffer> OutboundPacketMessage;
 
 // The biggest SCTP packet.  Starting from a 'safe' wire MTU value of 1280,
 // 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
 };
 
 struct SctpInboundPacket {
   rtc::Buffer buffer;
   ReceiveDataParams params;
   // The |flags| parameter is used by SCTP to distinguish notification packets
   // from other types of packets.
@@ skipping 48 matching lines @@
     case SctpDataMediaChannel::PPID_NONE:
       *dest = cricket::DMT_NONE;
       return true;
 
     default:
       return false;
   }
 }
 
 // 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);
     }
   }
 }
 
 // This is the callback usrsctp uses when there's data to send on the network
 // that has been wrapped appropriatly for the SCTP protocol.
 static int OnSctpOutboundPacket(void* addr, void* data, size_t length,
                                 uint8_t tos, uint8_t set_df) {
@@ skipping 42 matching lines @@
     // The ownership of |packet| transfers to |msg|.
     InboundPacketMessage* msg = new InboundPacketMessage(packet);
     channel->worker_thread()->Post(channel, MSG_SCTPINBOUNDPACKET, msg);
   }
   free(data);
   return 1;
 }
 
 // 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<SctpDataMediaChannel*> SctpDataEngine::channels_;

[tommi, 2015/08/27 11:38:02]

how is thread safety guaranteed?

[lally1, 2015/08/27 17:56:08]

Accesses to the member are via message-posting to single threads that manage
this.  Normally.  In this case, the library invoking the callback I added may be
using its own generated thread, so it's good that you reverted!

 
 SctpDataEngine::SctpDataEngine() {
   if (usrsctp_engines_count == 0) {
     // First argument is udp_encapsulation_port, which is not releveant for our
     // AF_CONN use of sctp.
     usrsctp_init(0, cricket::OnSctpOutboundPacket, debug_sctp_printf);
 
     // To turn on/off detailed SCTP debugging. You will also need to have the
     // SCTP_DEBUG cpp defines flag.
     // usrsctp_sysctl_set_sctp_debug_on(SCTP_DEBUG_ALL);
 
     // 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
     // when a new address is added or removed. This feature is enabled by
     // default.
     // usrsctp_sysctl_set_sctp_auto_asconf(0);
 
     // TODO(ldixon): Consider turning this on/off.
     // Add a blackhole sysctl. Setting it to 1 results in no ABORTs
     // being sent in response to INITs, setting it to 2 results
@@ skipping 34 matching lines @@
     }
     LOG(LS_ERROR) << "Failed to shutdown usrsctp.";
   }
 }
 
 DataMediaChannel* SctpDataEngine::CreateChannel(
     DataChannelType data_channel_type) {
   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(

[tommi, 2015/08/27 11:38:02]

On what thread does this method run and how do you know it's the same thread for
all data engines?

[lally1, 2015/08/27 17:56:09]

I/O thread for usrsctplib, which only has once instance ever loaded (controlled
via static int).  But it's all CritSec'd now.

+    struct socket* sock) {
+  for (auto p:channels_) {

[tommi, 2015/08/27 11:38:02]

nit: spaces around :
nit: would be good to use the actual type and not auto.

Also though, doesn't this create a copy of every channel when iterating?

[lally1, 2015/08/27 17:56:08]

Done and done.  It's a vector of pointers to 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);

[tommi, 2015/08/27 11:38:02]

SctpDataMediaChannel* channel

[lally1, 2015/08/27 17:56:08]

Acknowledged.

+  if (!channel) {
+    LOG(LS_ERROR) << "SendThresholdCallback: Failed to get channel for socket "
+                  << sock;
+    return 0;
+  }
+  channel->SignalReadyToSend(true);
+  return 0;
+}
+

[tommi, 2015/08/27 11:38:02]

nit: one empty line

[lally1, 2015/08/27 17:56:08]

Done.

+
 SctpDataMediaChannel::SctpDataMediaChannel(rtc::Thread* thread)
     : worker_thread_(thread),
       local_port_(kSctpDefaultPort),
       remote_port_(kSctpDefaultPort),
       sock_(NULL),
       sending_(false),
       receiving_(false),
       debug_name_("SctpDataMediaChannel") {
 }
 
 SctpDataMediaChannel::~SctpDataMediaChannel() {
   CloseSctpSocket();
+  SignalDestroyed(this);
 }
 
 sockaddr_conn SctpDataMediaChannel::GetSctpSockAddr(int port) {
   sockaddr_conn sconn = {0};
   sconn.sconn_family = AF_CONN;
 #ifdef HAVE_SCONN_LEN
   sconn.sconn_len = sizeof(sockaddr_conn);
 #endif
   // Note: conversion from int to uint16_t happens here.
   sconn.sconn_port = rtc::HostToNetwork16(port);
   sconn.sconn_addr = this;
   return sconn;
 }
 
 bool SctpDataMediaChannel::OpenSctpSocket() {
   if (sock_) {
     LOG(LS_VERBOSE) << debug_name_
                     << "->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;

[tommi, 2015/08/27 11:38:02]

nit: kSendThreshold

[lally1, 2015/08/27 17:56:09]

Done.

+
   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;
   }
 
   // Make the socket non-blocking. Connect, close, shutdown etc will not block
   // the thread waiting for the socket operation to complete.
   if (usrsctp_set_non_blocking(sock_, 1) < 0) {
     LOG_ERRNO(LS_ERROR) << debug_name_ << "Failed to set SCTP to non blocking.";
     return false;
@@ skipping 24 matching lines @@
 
   // Nagle.
   uint32_t nodelay = 1;
   if (usrsctp_setsockopt(sock_, IPPROTO_SCTP, SCTP_NODELAY, &nodelay,
                          sizeof(nodelay))) {
     LOG_ERRNO(LS_ERROR) << debug_name_ << "Failed to set SCTP_NODELAY.";
     return false;
   }
 
   // Disable MTU discovery
-  struct sctp_paddrparams params = {{0}};
+  struct sctp_paddrparams params;

[tommi, 2015/08/27 11:38:02]

prefer this the way it was (without memset)

[lally1, 2015/08/27 17:56:08]

The old way was leaving some bytes uninitialized, that triggered some sanitizer.
 I forget the bug (and can't find it right now), but that's why I changed it.

+  memset(&params, 0, sizeof(params));
   params.spp_assoc_id = 0;
   params.spp_flags = SPP_PMTUD_DISABLE;
   params.spp_pathmtu = kSctpMtu;
   if (usrsctp_setsockopt(sock_, IPPROTO_SCTP, SCTP_PEER_ADDR_PARAMS, &params,
       sizeof(params))) {
     LOG_ERRNO(LS_ERROR) << debug_name_
                         << "Failed to set SCTP_PEER_ADDR_PARAMS.";
     return false;
   }
 
@@ skipping 490 matching lines @@
 }
 
 bool SctpDataMediaChannel::SetRecvCodecs(const std::vector<DataCodec>& codecs) {
   return GetCodecIntParameter(
       codecs, kGoogleSctpDataCodecId, kGoogleSctpDataCodecName, kCodecParamPort,
       &local_port_);
 }
 
 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);
 }
 
 bool SctpDataMediaChannel::SendQueuedStreamResets() {
   if (!sent_reset_streams_.empty() || queued_reset_streams_.empty())
     return true;
 
   LOG(LS_VERBOSE) << "SendQueuedStreamResets[" << debug_name_ << "]: Sending ["
                   << ListStreams(queued_reset_streams_) << "], Open: ["
@@ skipping 41 matching lines @@
     }
     case MSG_SCTPOUTBOUNDPACKET: {
       rtc::scoped_ptr<OutboundPacketMessage> pdata(
           static_cast<OutboundPacketMessage*>(msg->pdata));
       OnPacketFromSctpToNetwork(pdata->data().get());
       break;
     }
   }
 }
 }  // namespace cricket