[mpls] AD review of draft-ietf-mpls-forwarding

["Adrian Farrel" <[e-mail]>]

Hi,

Thank you for a really thorough and clear document.

I have done my usual AD review upon receiving the publication request 
for this document. The purpose of the review is to catch any issues that
might otherwise show up in IETF last call and IESG evaluation. In this
case it is also an extra pair of eyes on what is a very detailed 
document.

I have a number of comments below. A few are editorial nits (sorry) and
some are questions for clarification of the text. There is very little 
where I come even remotely close to saying "wrong" or missing".

It looks to me that a new revision would be useful just to mop up these
comments, so I have put the document into "Revised I-D Needed" state and
will wait to see the next revision before starting the IETF last call.
Please debate any issues where you disagree with me or think that no
document change is needed.

Thanks for the work,
Adrian

===

idnits notes that...

  == Outdated reference: draft-ietf-pwe3-vccv-impl-survey-results has been
     published as RFC 7079

---

Andy may want to update his coordinates.

---

Your acronym list is commendably thorough, but a little enthusiastic.

AC is only used once and then only at the point of expansion.
CE doesn't appear to be used at all
FEC only seems to be used in this document in the context of 
  Forwarding Equivalence Classes (LDP)

The following are all well-known acronyms according to
  http://www.rfc-editor.org/rfc-style-guide/abbrev.expansion.txt
  so they don't need to be included
BGP
CPU
DDoS
DoS
GMPLS
IANA
IP
IPv4
IPv6
LDP
MPLS
NTP
QoS
RTP
TCP
UDP
WG


I think Curtis may have heard this before :-)
The "preferred" (by the RFC editor) expansion of ECMP is
"Equal-Cost Multipath"

Several acronyms are qualified with (LDP) (CE, FEC, P, PE). I haven't
checked the usage in this document, but it seems to me that the terms
might have wider applicability. Indeed, a quick search revealed 
"RSVP-TE PE".

The third expansion of PSC could have a reference to RFC 6378. I think
that to avoid confusion, you need to expand this in the text when you 
use it. You do this well everywhere except in Section 4.7 T#32, and in
this acronym list itself at E-LSP and L-LSP.

---

Section 1.3 bullet 5

   5.  The implementer and system designer MUST support pseudowire
       control word (CW) if MPLS-TP is supported or if ACH [RFC5586] is
       being used on a pseudowire.

The wording is a bit odd. "The implementation and system design..."?

Ditto bullets 6 and 7

---

Section 1.3

While there is not wrong with the statements made in the bullets, some
of the later ones refer to recent additions to the MPLS suite. Yet the
list is presented as "there were some misconceptions." Clearly the
early silicon did not have misconceptions about the inclusion of entropy
labels. 

Just tweak the words at the top of the list?

---

Section 2.1

   Tunneling encapsulations which may carry MPLS, such as MPLS in GRE,
   L2TP, or LDP, are out of scope.

I think s/LDP/UDP/

---

2.1.1

Maybe the first paragraph should clarify "special purpose labels at 
the top of the label stack"

---

2.1.1

I think that this section should note that labels 0-15 were commonly
referred to as "reserved labels" and are only renamed to "special
purpose labels" by [I-D.ietf-mpls-special-purpose-labels].

---

2.1.4

   MPLS hierarchy as described in [RFC4206] can in principle add up to
   four additional labels.  MPLS hierarchy is discussed in
   Section 2.1.6.

Similar text in 2.1.6

I wonder whether the choice of "four" here is because RFC 4203 defines
      1     Packet-Switch Capable-1 (PSC-1)
      2     Packet-Switch Capable-2 (PSC-2)
      3     Packet-Switch Capable-3 (PSC-3)
      4     Packet-Switch Capable-4 (PSC-4)
If that is the case, we should note that RFC 7074 deprecates PSC-2 
through 4 recognising that different switching levels within the PSC
world were not applied and that LSP nesting was arbitrary and not 
limited to four levels.

On the other hand, if "four" comes from somewhere else, perhaps you 
could give a clue in the text.

---

While per platform label space is mentioned in 2.1.7 I wonder whether
more information on per platform and per interface label spaces is 
needed. I recall early implementations that got very confused when
parallel interfaces used the same label for different purposes.

I guess the point there is that you cannot assume that your neighbor
is or is not using the per platform label space.

Upstream label allocation may also come into this.

---

2.1.8.1

   1.  The most common case is where reordering occurs is rare,

One too many instances of "is"

---

2.1.8.1

   3.  If the edge is not using pseudowire control word (CW) and the
       core is using multipath, reordering will be far more common.  If
       this is occurring, the best solution is to use CW on the edge,
       rather than try to fix the reordering using resequencing.

Completely agree, but isn't the sequence number contained in a control
word meaning that the resequencing could, in any case, not be done 
without using a control word?

---

2.1.8.1

   4.  Another avoidable case is where some core equipment has multipath
       and for some reason insists on periodically installing a new
       random number as the multipath hash seed.  If supporting MPLS-TP,
       equipment MUST provide a means to disable periodic hash reseeding
       and deployments MUST disable periodic hash reseeding.  Even if
       not supporting MPLS-TP, equipment should provide a means to
       disable periodic hash reseeding and deployments should disable
       periodic hash reseeding.

Are those two "should" really "SHOULD"?

---

Should 2.2 distinguish the order of magnitude of replication at branch
nodes? This impacts the replication method used (some devices make a
copy and cycle around, some devices can do multiple copies at once). On
the whole is no different from IP multicast processing except (as you
note) that each outgoing packet may be different by its label value.

---

2.4

So obvious you didn't say it?

   In order to support an adequately balanced load distribution across
   multiple links, IP header information must be used.  Common practice
   today is to reinspect the IP headers at each LSR and use the label
   stack and IP header information in a hash performed at each LSR.
   Further details are provided in Section 2.4.5.

Missing is the statement that a single "flow" must not be distributed 
across multiple paths because of the implication for potentially
significant packet misordering. And feeding that is a common requirement
that such packet misordering must not occur because applications and
transport protocol implementations cannot survive such misordering.

---

2.4.2 uses "composite link" and "component link". I suggest picking just
one term.

---

2.4.5.1 notes that special purpose and extended special purpose labels 
need to be excluded from the hash. Good.
But it seems that some special purpose labels will indicate that the 
next label stack entry contains a label with special meaning. (ELI is
an example that we specifically don't have to worry about.)
How do we handle that?
Should we be dividing up the extended special purpose label space to
have one set of code points meaning "just this label is special" and
another set meaning "this label is special and the next label stack
entry is magic"?

---

An issue that arises from the multipath support (2.4.5.1) is that
hardware assumes that after a label stack entry with the S-bit set,
there are only three possible next bytes...
- a control word (indicated by b0000 or b0001)
- an IPv4 header
- an IPv6 header
This is the case regardless of how the LSP was set up, and the next
bytes cannot ever be further MPLS stack entries.

While this comes up 2.4.5.1 it may merit further discussion in an
earlier section of the document.

I note that discussion of support of PWs without the CW drives you
to say that hashing beyond the S-bit should be a configurable option
which would (of course) support any payload including MPLS in MPLS
with repeated bottom of stack. However, you might want to specifically
preclude that.

---

Q#7 introduces the terms "short-pipe" and "uniform model". It provides
a pointer to 2.1.6, but that section does not mention these terms (and
doesn't refer to RFC 3270).

---

Section 7 could usefully point back at Section 2.6.1