Support for IPv6 Path MTU discovery

Support for IPv6 Path MTU discovery

[Tommaso Pecorella, 2013-06-29 07:42:04]

This iss the (not so preliminary) Path MTU support for IPv6.

I just need to check Valgrind, all the test cases seems to be working a
intended.

[Tom Henderson, 2013-06-29 16:17:01]

you mentioned "all test cases pass"; where are those tests?

there needs to be some documentation added (for IPv6 in general, and for this in
particular).  This is empty:
http://www.nsnam.org/docs/release/3.17/models/html/ipv6.html

In particular, it seems to me that this patch enables that a destination can
have an MTU set on it different than the link MTU, and it enables the sending of
packet-too-big back upon MTU mismatch discovery, but these aspects are not
implemented yet:
- ability for a transport protocol to adapt to this PMTU
- maintaining the PMTU over a long running connection (probing again when it
expires, probing to see whether path has changed and MTU should be expanded)

i.e. the scope of the implementation needs to be documented somewhere (such as
the ipv6 manual chapter)

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-l3-pro...
File src/internet/model/ipv6-l3-protocol.cc (right):

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-l3-pro...
src/internet/model/ipv6-l3-protocol.cc:99: m_pmtuCache =
CreateObject<Ipv6PmtuCache> ();
is there a reason that this cache is not created when the Ipv6L3Protocol object
is created, rather than deferred to Initialize?

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-l3-pro...
src/internet/model/ipv6-l3-protocol.cc:413: return 1280;
in other places in ns-3, we would define a constant at the top of this file like
IPV6_MIN_MTU and use it here, rather than a literal

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-l3-pro...
src/internet/model/ipv6-l3-protocol.cc:537: NS_LOG_FUNCTION_NOARGS ();
in general, the logging needs cleaned up in this file, outside of your patch
(Vedran may be working on it), but this should be changed at least.  should be
NS_LOG_FUNCTION (this)

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-l3-pro...
File src/internet/model/ipv6-l3-protocol.h (right):

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-l3-pro...
src/internet/model/ipv6-l3-protocol.h:278: virtual void SetPmtu (Ipv6Address
dst, uint32_t pmtu);
how does a client get the current Pmtu for a destination?  I don't see a getter
in this API.

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
File src/internet/model/ipv6-pmtu-cache.cc (right):

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
src/internet/model/ipv6-pmtu-cache.cc:27: NS_LOG_COMPONENT_DEFINE
("Ipv6PmtuCache");
this statement typically precedes 'namespace ns3'

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
src/internet/model/ipv6-pmtu-cache.cc:98: if (validity > Seconds(60*5))
suggest comment here to clarify the value (where does this check come from, a
MUST in the RFC?)

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
src/internet/model/ipv6-pmtu-cache.cc:104: NS_LOG_LOGIC("rejecting a PMTU
validity timer lesser than 5 minutes");
this has the property of silently performing a no-op if the user submits a low
value (and does not have logging turned on).  I think it would be better to
return either bool (true if the set was successful) or a Time of what it is set
to (and in the else case, return whatever time holds for the path if the
submitted time is rejected)

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
File src/internet/model/ipv6-pmtu-cache.h (right):

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
src/internet/model/ipv6-pmtu-cache.h:33: * \brief This class implements the Path
MTU cache, as defined by RFC XXX
RFC 1981

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
src/internet/model/ipv6-pmtu-cache.h:66: uint32_t GetPmtu(Ipv6Address dst);
suggest to run check-style.py on the new files

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
src/internet/model/ipv6-pmtu-cache.h:72: * \return the Path MTU (zero if
unknown)
there is no return to this function

[Tommaso Pecorella, 2013-06-29 20:14:49]

On 2013/06/29 16:17:01, Tom Henderson wrote:
> you mentioned "all test cases pass"; where are those tests?

Sorry, I meant "all my tests". Basically a modified ipv6 fragmentation example.
I'll push that as well.

> there needs to be some documentation added (for IPv6 in general, and for this
in
> particular).  This is empty:
> http://www.nsnam.org/docs/release/3.17/models/html/ipv6.html

True... adding it to the to-do list. I'll write it for 3.18.

> In particular, it seems to me that this patch enables that a destination can
> have an MTU set on it different than the link MTU, and it enables the sending
of
> packet-too-big back upon MTU mismatch discovery, but these aspects are not
> implemented yet:
> - ability for a transport protocol to adapt to this PMTU
> - maintaining the PMTU over a long running connection (probing again when it
> expires, probing to see whether path has changed and MTU should be expanded)

True, true, wrong.

True; you got it right. A too big packet already triggers the correct ICMP, but
no action was done upon receiving it. So two nodes would keep sending big
packets and rejecting them endlessly.
The patch simply enables the sender to react to the ICMP and fragment the
packets.

True: the upper layers should react as well. However this is not mandatory in
the RFC. Strongly suggested, but not mandatory. Enabling this feature would
require a bit more work and I'd defer it. Also because I'd like some
collaboration about it, since I'm not fully into the TCP code.
If the upper layers are unable to react to a PMTU change, the IP layer will use
fragmentation. Not 100% efficient, but it works.

Wrong: maintaining the PMTU is already in place. Better, it doesn't have to be
done. Rationale: IP is stateless and shouldn't be bothered with "upper layer"
connections. When the PMTU cache entry expires, it's removed. If the upper layer
will send another packet, that will be the probe. This, by the way, means that a
connection will loose one packet every 10 minutes (default cache validity).

There are a couple of implementation details that have to be ironed out, however
they are in TCP and UDP. As an example, TCP shouldn't react to a packet loss due
to packet size in the same way as a normal packet loss. However, again, I need
some help on this.

> i.e. the scope of the implementation needs to be documented somewhere (such as
> the ipv6 manual chapter)

... which is to be written... Dang, it seems that I'll really have to write it !

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-l3-pro...
File src/internet/model/ipv6-l3-protocol.cc (right):

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-l3-pro...
src/internet/model/ipv6-l3-protocol.cc:99: m_pmtuCache =
CreateObject<Ipv6PmtuCache> ();
On 2013/06/29 16:17:01, Tom Henderson wrote:
> is there a reason that this cache is not created when the Ipv6L3Protocol
object
> is created, rather than deferred to Initialize?  

None, and It would save a "new" function. I'll change it.

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-l3-pro...
src/internet/model/ipv6-l3-protocol.cc:413: return 1280;
On 2013/06/29 16:17:01, Tom Henderson wrote:
> in other places in ns-3, we would define a constant at the top of this file
like
> IPV6_MIN_MTU and use it here, rather than a literal

True. /bow.

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-l3-pro...
src/internet/model/ipv6-l3-protocol.cc:537: NS_LOG_FUNCTION_NOARGS ();
On 2013/06/29 16:17:01, Tom Henderson wrote:
> in general, the logging needs cleaned up in this file, outside of your patch
> (Vedran may be working on it), but this should be changed at least.  should be
> NS_LOG_FUNCTION (this)

Never code late night. Plus, the "SetMtuDiscover" is missing the logging...

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-l3-pro...
File src/internet/model/ipv6-l3-protocol.h (right):

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-l3-pro...
src/internet/model/ipv6-l3-protocol.h:278: virtual void SetPmtu (Ipv6Address
dst, uint32_t pmtu);
On 2013/06/29 16:17:01, Tom Henderson wrote:
> how does a client get the current Pmtu for a destination?  I don't see a
getter
> in this API.

The SetPmtu is used by ICMPv6 upon receiving a PACKET_TOO_BIG.
The ICMP is also forwarded to the TCP and UDP layers, which *might* react to it
(it's not mandatory).
We could as well (and we should indeed) define a callback to inform all the
upper layers that a PMTU is changed, however I wanted to limit the changes to
the minimum.

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
File src/internet/model/ipv6-pmtu-cache.cc (right):

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
src/internet/model/ipv6-pmtu-cache.cc:27: NS_LOG_COMPONENT_DEFINE
("Ipv6PmtuCache");
On 2013/06/29 16:17:01, Tom Henderson wrote:
> this statement typically precedes 'namespace ns3'

Gonna move it.

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
src/internet/model/ipv6-pmtu-cache.cc:98: if (validity > Seconds(60*5))
On 2013/06/29 16:17:01, Tom Henderson wrote:
> suggest comment here to clarify the value (where does this check come from, a
> MUST in the RFC?)

Yep, it's a must in the RFC. Gonna clarify.

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
src/internet/model/ipv6-pmtu-cache.cc:104: NS_LOG_LOGIC("rejecting a PMTU
validity timer lesser than 5 minutes");
On 2013/06/29 16:17:01, Tom Henderson wrote:
> this has the property of silently performing a no-op if the user submits a low
> value (and does not have logging turned on).  I think it would be better to
> return either bool (true if the set was successful) or a Time of what it is
set
> to (and in the else case, return whatever time holds for the path if the
> submitted time is rejected)

The validity time is generic. We could have a new function to probe the validity
timer left for a particular PMTU, but it's not really necessary.

The two functions are private and they're meant to be used by an attribute
changer. Probably I should play a bit with the attribute checker instead of
doing the check here.

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
File src/internet/model/ipv6-pmtu-cache.h (right):

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
src/internet/model/ipv6-pmtu-cache.h:33: * \brief This class implements the Path
MTU cache, as defined by RFC XXX
On 2013/06/29 16:17:01, Tom Henderson wrote:
> RFC 1981

Tnx, forgot.

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
src/internet/model/ipv6-pmtu-cache.h:66: uint32_t GetPmtu(Ipv6Address dst);
On 2013/06/29 16:17:01, Tom Henderson wrote:
> suggest to run check-style.py on the new files

Yep, good idea.

https://codereview.appspot.com/10779043/diff/1/src/internet/model/ipv6-pmtu-c...
src/internet/model/ipv6-pmtu-cache.h:72: * \return the Path MTU (zero if
unknown)
On 2013/06/29 16:17:01, Tom Henderson wrote:
> there is no return to this function

Copy-paste gone bad.

[Tommaso Pecorella, 2013-06-29 20:55:36]

On 2013/06/29 20:51:44, Tommaso Pecorella wrote:

I still have no idea on how to constrain an Attribute value to something, i.e.,
a time greater than 5 minutes. That's the only remaining problem (not counting
the manual and L4 protocols).

[Tom Henderson, 2013-07-02 12:36:00]

On 2013/06/29 20:55:36, Tommaso Pecorella wrote:
> On 2013/06/29 20:51:44, Tommaso Pecorella wrote:
> 
> I still have no idea on how to constrain an Attribute value to something,
i.e.,
> a time greater than 5 minutes. That's the only remaining problem (not counting
> the manual and L4 protocols).

see src/core/test/attribute-test-suite.cc:104 for an example integer that is
bounded; is that what you want?

[Tom Henderson, 2013-07-02 12:43:12]

https://codereview.appspot.com/10779043/diff/16001/examples/ipv6/fragmentatio...
File examples/ipv6/fragmentation-ipv6-two-MTU.cc (right):

https://codereview.appspot.com/10779043/diff/16001/examples/ipv6/fragmentatio...
examples/ipv6/fragmentation-ipv6-two-MTU.cc:19: *         Sebastien Vincent
<vincent@clarinet.u-strasbg.fr>
is this author string and copyright still accurate?

https://codereview.appspot.com/10779043/diff/16001/examples/ipv6/fragmentatio...
examples/ipv6/fragmentation-ipv6-two-MTU.cc:29: // // - Tracing of queues and
packet receptions to file "fragmentation-ipv6.tr"
what does this example show?  Explain here...

https://codereview.appspot.com/10779043/diff/16001/examples/ipv6/fragmentatio...
examples/ipv6/fragmentation-ipv6-two-MTU.cc:48: class StackHelper
this migrates to a helper like InternetStackHelper?

https://codereview.appspot.com/10779043/diff/16001/examples/ipv6/fragmentatio...
examples/ipv6/fragmentation-ipv6-two-MTU.cc:95: #if 0
better to put this under control of a command line argument such as "logging=1"

Similar to what we have done in WiFi, we could consider a logging helper (for
InternetStackHelper) that turns on all IPv6 logging:

see WifiHelper::EnableLogComponents()

[Tommaso Pecorella, 2013-07-02 13:10:49]

https://codereview.appspot.com/10779043/diff/16001/examples/ipv6/fragmentatio...
File examples/ipv6/fragmentation-ipv6-two-MTU.cc (right):

https://codereview.appspot.com/10779043/diff/16001/examples/ipv6/fragmentatio...
examples/ipv6/fragmentation-ipv6-two-MTU.cc:19: *         Sebastien Vincent
<vincent@clarinet.u-strasbg.fr>
On 2013/07/02 12:43:12, Tom Henderson wrote:
> is this author string and copyright still accurate?

The modifications over the original one are so few that the answer is yes. The
differences are 1 line added and two lines inverted.

https://codereview.appspot.com/10779043/diff/16001/examples/ipv6/fragmentatio...
examples/ipv6/fragmentation-ipv6-two-MTU.cc:48: class StackHelper
On 2013/07/02 12:43:12, Tom Henderson wrote:
> this migrates to a helper like InternetStackHelper?

Maybe... another orthogonal change tho. I'd rather leave it as it is now, and
modify *all* the examples so to remove this helper (which is basically i every
example).

Very good idea, tho, as these are supposed to be... examples.

https://codereview.appspot.com/10779043/diff/16001/examples/ipv6/fragmentatio...
examples/ipv6/fragmentation-ipv6-two-MTU.cc:95: #if 0
On 2013/07/02 12:43:12, Tom Henderson wrote:
> better to put this under control of a command line argument such as
"logging=1"
> 
> Similar to what we have done in WiFi, we could consider a logging helper (for
> InternetStackHelper) that turns on all IPv6 logging:
> 
> see WifiHelper::EnableLogComponents()

Orthogonal change, but good idea. Going to do it in another update.

[Tommaso Pecorella, 2013-07-02 13:18:07]

On 2013/07/02 12:36:00, Tom Henderson wrote:
> On 2013/06/29 20:55:36, Tommaso Pecorella wrote:
> > On 2013/06/29 20:51:44, Tommaso Pecorella wrote:
> > 
> > I still have no idea on how to constrain an Attribute value to something,
> i.e.,
> > a time greater than 5 minutes. That's the only remaining problem (not
counting
> > the manual and L4 protocols).
> 
> see src/core/test/attribute-test-suite.cc:104 for an example integer that is
> bounded; is that what you want?

Yes, exactly that. Except that there isn't a "bound" checker for a TimeValue.

/serc/core/nstime.h, line 702: "ATTRIBUTE_CHECKER_DEFINE (Time);".

I'm a bit scared to change nstime.h...

[Tom Henderson, 2013-07-02 13:36:10]

On 2013/07/02 13:18:07, Tommaso Pecorella wrote:
> On 2013/07/02 12:36:00, Tom Henderson wrote:
> > On 2013/06/29 20:55:36, Tommaso Pecorella wrote:
> > > On 2013/06/29 20:51:44, Tommaso Pecorella wrote:
> > > 
> > > I still have no idea on how to constrain an Attribute value to something,
> > i.e.,
> > > a time greater than 5 minutes. That's the only remaining problem (not
> counting
> > > the manual and L4 protocols).
> > 
> > see src/core/test/attribute-test-suite.cc:104 for an example integer that is
> > bounded; is that what you want?
> 
> Yes, exactly that. Except that there isn't a "bound" checker for a TimeValue.
> 
> /serc/core/nstime.h, line 702: "ATTRIBUTE_CHECKER_DEFINE (Time);".
> 
> I'm a bit scared to change nstime.h...

Can you just indirect the attribute setter to a method that will enforce bounds
checking?  It is not as self-documenting as the "Checker" example I pointed to
but should accomplish the same.

[Tommaso Pecorella, 2013-07-02 15:38:59]

On 2013/07/02 13:36:10, Tom Henderson wrote:
> On 2013/07/02 13:18:07, Tommaso Pecorella wrote:
> > On 2013/07/02 12:36:00, Tom Henderson wrote:
> > > On 2013/06/29 20:55:36, Tommaso Pecorella wrote:
> > > > On 2013/06/29 20:51:44, Tommaso Pecorella wrote:
> > > > 
> > > > I still have no idea on how to constrain an Attribute value to
something,
> > > i.e.,
> > > > a time greater than 5 minutes. That's the only remaining problem (not
> > counting
> > > > the manual and L4 protocols).
> > > 
> > > see src/core/test/attribute-test-suite.cc:104 for an example integer that
is
> > > bounded; is that what you want?
> > 
> > Yes, exactly that. Except that there isn't a "bound" checker for a
TimeValue.
> > 
> > /serc/core/nstime.h, line 702: "ATTRIBUTE_CHECKER_DEFINE (Time);".
> > 
> > I'm a bit scared to change nstime.h...
> 
> Can you just indirect the attribute setter to a method that will enforce
bounds
> checking?  It is not as self-documenting as the "Checker" example I pointed to
> but should accomplish the same.

Hem... that's what the Setter does. You suggested to find another way because
the user could set a value which would be silently rejected. Am I missing
something ?

[barnes26_llnl.gov, 2013-07-02 23:48:43]

On Tuesday, July 2, 2013 6:18:07 AM UTC-7, Tommaso Pecorella wrote:
>
> On 2013/07/02 12:36:00, Tom Henderson wrote: 
> > On 2013/06/29 20:55:36, Tommaso Pecorella wrote: 
> > > On 2013/06/29 20:51:44, Tommaso Pecorella wrote: 
> > > 
> > > I still have no idea on how to constrain an Attribute value to 
> something, i.e., 
> > > a time greater than 5 minutes. That's the only remaining problem (not 
> counting 
> > > the manual and L4 protocols). 
> > 
> > see src/core/test/attribute-test-suite.cc:104 for an example integer 
> that is 
> > bounded; is that what you want? 
>
> Yes, exactly that. Except that there isn't a "bound" checker for a 
> TimeValue. 
>
> /src/core/nstime.h, line 702: "ATTRIBUTE_CHECKER_DEFINE (Time);". 
>
> I'm a bit scared to change nstime.h... 
>

I think I can provide that.

[Tommaso Pecorella, 2013-07-03 10:55:32]

On 2013/07/02 23:48:43, barnes26_llnl.gov wrote:
> 
> On Tuesday, July 2, 2013 6:18:07 AM UTC-7, Tommaso Pecorella wrote:
> >
> > Yes, exactly that. Except that there isn't a "bound" checker for a 
> > TimeValue. 
> >
> > /src/core/nstime.h, line 702: "ATTRIBUTE_CHECKER_DEFINE (Time);". 
> >
> > I'm a bit scared to change nstime.h... 
> >
> 
> I think I can provide that.

That would be wonderful. Thanks.

[barnes26_llnl.gov, 2013-07-03 20:33:08]

On Jul 3, 2013, at 3:55 AM, <tommypec@gmail.com> wrote:
> On 2013/07/02 23:48:43, barnes26_llnl.gov wrote:
>>> On Tuesday, July 2, 2013 6:18:07 AM UTC-7, Tommaso Pecorella wrote:
>>> > Yes, exactly that. Except that there isn't a "bound" checker for a
>>> > TimeValue.
>>> >
>>> > /src/core/nstime.h, line 702: "ATTRIBUTE_CHECKER_DEFINE (Time);".
>>> >
>>> > I'm a bit scared to change nstime.h...
>>> >
>> 
>> I think I can provide that.
> 
> That would be wonderful. Thanks.

Done:

On Jul 3, 2013, at 12:02 PM, <code@nsnam.ece.gatech.edu>
 wrote:
> ---- Repair Time docs mangled by f0be2daaa38f
> user: Peter D. Barnes, Jr. <barnes26@llnl.gov>
> files: src/core/model/nstime.h
> url: http://code.nsnam.org/ns-3-dev/rev/3c64a1fbfcf9
> 
> 
> ---- Time attributes with enforced bounds.
> user: Peter D. Barnes, Jr. <barnes26@llnl.gov>
> files: src/core/model/nstime.h src/core/model/time.cc
src/core/test/attribute-test-suite.cc
> url: http://code.nsnam.org/ns-3-dev/rev/e12094c8b31b
> 
> 
> ---- Merge bounded Time attributes
> user: Peter D. Barnes, Jr. <barnes26@llnl.gov>
> files: CHANGES.html RELEASE_NOTES
> url: http://code.nsnam.org/ns-3-dev/rev/91a40fb09b58

https://codereview.appspot.com/10779043/

_______________________________________________________________________
Dr. Peter D. Barnes, Jr.                Physics Division
Lawrence Livermore National Laboratory  Physical and Life Sciences
7000 East Avenue, L-50                  email:  pdbarnes@llnl.gov
P. O. Box 808                           Voice:  (925) 422-3384
Livermore, California 94550             Fax:    (925) 423-3371

[Tommaso Pecorella, 2013-07-04 14:49:58]

On 2013/07/04 08:00:25, Tommaso Pecorella wrote:

The latest patch does include some "strange" functions related to bug #1702
http://www.nsnam.org/bugzilla/show_bug.cgi?id=1702

This is due to the particular order in my MercurialQueue. Sorry about that. In
case the patches are applied in a different order I'll make sure they'll be
modified accordingly.