Fix setting the MTU for SCTP.

Fix setting the MTU for SCTP.

It was being set at the wrong point in time and with the address
parameter missing, so it wasn't having any effect.

Committed: https://crrev.com/fe8f48962aacab4a8437c9d98dfcda225329c68e
Cr-Commit-Position: refs/heads/master@{#13909}

[Taylor Brandstetter, 2016-08-11 17:15:43]

deadbeef@webrtc.org changed reviewers:
+ honghaiz@webrtc.org, pthatcher@webrtc.org

[Taylor Brandstetter, 2016-08-11 17:15:44]

Here are the MTU changes split from the other CL.

Again, I don't know if this is something we should be doing in the first place.
The data channel spec says MTU discovery *should* be enabled. The only
restriction it places on the MTU is that it MUST start at 1200 for IPv4 and 1280
for IPv6, and discovery SHOULD restart when the route changes (which we don't
support).

So maybe for now we should leave MTU discovery enabled, set the initial MTU to
1200, and leave a TODO to hook up information about the route.

[Taylor Brandstetter, 2016-08-11 18:55:01]

On 2016/08/11 17:15:44, Taylor Brandstetter wrote:
> Here are the MTU changes split from the other CL.
> 
> Again, I don't know if this is something we should be doing in the first
place.
> The data channel spec says MTU discovery *should* be enabled. The only
> restriction it places on the MTU is that it MUST start at 1200 for IPv4 and
1280
> for IPv6, and discovery SHOULD restart when the route changes (which we don't
> support).
> 
> So maybe for now we should leave MTU discovery enabled, set the initial MTU to
> 1200, and leave a TODO to hook up information about the route.

Actually it looks like MTU discovery in usrsctp isn't even implemented for
userspace mode. Nowhere does it send probing padding packets as described in
https://tools.ietf.org/html/rfc4820.

In fact, if it's left disabled, then after 10 minutes you end up with an MTU of
about 1500 due to a hardcoded default value:
https://cs.chromium.org/chromium/src/third_party/usrsctp/usrsctplib/usrsctpli...

So maybe that's why we were trying to explicitly disable it.

[pthatcher1, 2016-08-16 21:24:53]

On 2016/08/11 18:55:01, Taylor Brandstetter wrote:
> On 2016/08/11 17:15:44, Taylor Brandstetter wrote:
> > Here are the MTU changes split from the other CL.
> > 
> > Again, I don't know if this is something we should be doing in the first
> place.
> > The data channel spec says MTU discovery *should* be enabled. The only
> > restriction it places on the MTU is that it MUST start at 1200 for IPv4 and
> 1280
> > for IPv6, and discovery SHOULD restart when the route changes (which we
don't
> > support).
> > 
> > So maybe for now we should leave MTU discovery enabled, set the initial MTU
to
> > 1200, and leave a TODO to hook up information about the route.
> 
> Actually it looks like MTU discovery in usrsctp isn't even implemented for
> userspace mode. Nowhere does it send probing padding packets as described in
> https://tools.ietf.org/html/rfc4820.
> 
> In fact, if it's left disabled, then after 10 minutes you end up with an MTU
of
> about 1500 due to a hardcoded default value:
>
https://cs.chromium.org/chromium/src/third_party/usrsctp/usrsctplib/usrsctpli...
> 
> So maybe that's why we were trying to explicitly disable it.

So what effect does our current code have?  If we just removed it, what would
happen?

[Taylor Brandstetter, 2016-08-16 22:34:36]

On 2016/08/16 21:24:53, pthatcher1 wrote:
> On 2016/08/11 18:55:01, Taylor Brandstetter wrote:
> > On 2016/08/11 17:15:44, Taylor Brandstetter wrote:
> > > Here are the MTU changes split from the other CL.
> > > 
> > > Again, I don't know if this is something we should be doing in the first
> > place.
> > > The data channel spec says MTU discovery *should* be enabled. The only
> > > restriction it places on the MTU is that it MUST start at 1200 for IPv4
and
> > 1280
> > > for IPv6, and discovery SHOULD restart when the route changes (which we
> don't
> > > support).
> > > 
> > > So maybe for now we should leave MTU discovery enabled, set the initial
MTU
> to
> > > 1200, and leave a TODO to hook up information about the route.
> > 
> > Actually it looks like MTU discovery in usrsctp isn't even implemented for
> > userspace mode. Nowhere does it send probing padding packets as described in
> > https://tools.ietf.org/html/rfc4820.
> > 
> > In fact, if it's left disabled, then after 10 minutes you end up with an MTU
> of
> > about 1500 due to a hardcoded default value:
> >
>
https://cs.chromium.org/chromium/src/third_party/usrsctp/usrsctplib/usrsctpli...
> > 
> > So maybe that's why we were trying to explicitly disable it.
> 
> So what effect does our current code have?  If we just removed it, what would
> happen?

I believe the current code does disable path MTU discovery, but does not modify
the default MTU.

If we removed it, path MTU discovery would be enabled (which as described above,
we probably don't want).

[pthatcher1, 2016-08-23 00:17:12]

lgtm

[honghaiz3, 2016-08-23 00:29:00]

lgtm

[Taylor Brandstetter, 2016-08-24 22:15:29]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-08-24 22:15:29]

The patchset sent to the CQ was uploaded after l-g-t-m from honghaiz@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2237073002/#ps20001 (title:
"Clarifying a comment.")

[commit-bot: I haz the power, 2016-08-24 22:15:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-24 23:26:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-24 23:27:04]

Description was changed from

==========
Fix setting the MTU for SCTP.

It was being set at the wrong point in time and with the address
parameter missing, so it wasn't having any effect.
==========

to

==========
Fix setting the MTU for SCTP.

It was being set at the wrong point in time and with the address
parameter missing, so it wasn't having any effect.

Committed: https://crrev.com/fe8f48962aacab4a8437c9d98dfcda225329c68e
Cr-Commit-Position: refs/heads/master@{#13909}
==========

[commit-bot: I haz the power, 2016-08-24 23:27:05]

Patchset 2 (id:??) landed as
https://crrev.com/fe8f48962aacab4a8437c9d98dfcda225329c68e
Cr-Commit-Position: refs/heads/master@{#13909}