[Lsr] AD review of draft-ietf-lsr-isis-fast-flooding-05

[John Scudder <[e-mail]>]

Hi Authors, WG,

Thanks for this document.

I’ve supplied my questions and comments in the form of an edited copy of the draft. Minor editorial suggestions I’ve made in place without further comment, more substantive questions and comments are done in-line and prefixed with “jgs:”. You can use your favorite diff tool to review them; I’ve attached the iddiff output for your convenience if you’d like to use it. I’ve also pasted a traditional diff below in case you want to use it for in-line reply.

I’ve also requested a TSV early review of the document.

—John

--- draft-ietf-lsr-isis-fast-flooding-05.txt	2024-01-24 19:46:21.000000000 -0500
+++ draft-ietf-lsr-isis-fast-flooding-05-jgs-comments.txt	2024-01-25 12:43:39.000000000 -0500
@@ -285,6 +285,9 @@
 4.1.  LSP Burst Window sub-TLV
 
    The LSP Burst Window sub-TLV advertises the maximum number of LSPs
+--
+jgs: should this say "maximum number of un-acknowledged LSPs"?
+--
    that the node can receive without an intervening delay between LSP
    transmissions.
 
@@ -293,6 +296,16 @@
    Length: 4 octets
 
    Value: number of LSPs that can be sent back-to-back.
+--
+jgs: in this subsection and the following one, it seems like you're
+switching between writing from the perspective of the advertising IS in
+the first paragraph, to writing from the perspective of the receiving
+IS, in the "value" description. In this case, first you tell me it's the
+maximum number that the node can receive, but then you tell me the value
+means the maximum number that "can be sent". Which is it, sent, or
+received? I think you need to clarify this somehow, for example you
+could clarify by saying "can be sent" *by whom*.
+--
 
 4.2.  LSP Transmission Interval sub-TLV
 
@@ -300,6 +313,9 @@
    interval, in micro-seconds, between LSPs arrivals which can be
    received on this interface, after the maximum number of un-
    acknowledged LSPs have been sent.
+--
+jgs: here you're mixing sent and received in the same paragraph.
+--
 
    Type: 2
 
@@ -307,6 +323,9 @@
 
    Value: minimum interval, in micro-seconds, between two consecutive
    LSPs sent after LSP Burst Window LSPs have been sent
+--
+jgs: and above
+--
 
    The LSP Transmission Interval is an advertisement of the receiver's
    steady-state LSP reception rate.
@@ -351,6 +370,9 @@
    When the O-flag (Ordered acknowledgement) is set, the LSPs will be
    acknowledged in the order they are received: a PSNP acknowledging N
    LSPs is acknowledging the N oldest LSPs received.  The order inside
+--
+jgs: should that be "the N oldest unacknowledged LSPs"?
+--
    the PSNP is meaningless.  If the sender keeps track of the order of
    LSPs sent, this indication allows a fast detection of the loss of an
    LSP.  This MUST NOT be used to alter the retransmission timer for any
@@ -363,7 +385,7 @@
    Sequence Number PDUs.  This time will trigger the sending of a PSNP
    even if the number of unacknowledged LSPs received on a given
    interface does not exceed LPP (Section 4.3).  The time is measured
-   from the reception of the first unacknowldeged LSP.
+   from the reception of the first unacknowledged LSP.
 
    Type: 5
 
@@ -453,6 +475,14 @@
 5.1.  Rate of LSP Acknowledgments
 
    On point-to-point networks, PSNP PDUs provide acknowledgments for
+--
+jgs: up to you whether to fix this or not, but since PSNP is an
+abbreviation for "Partial Sequence Number PDU", the above expands as
+"Partial Sequence Number PDU PDUs". So pedantically, just "PSNPs" or
+if you want to get funky, "PSN PDUs".
+
+See also, ATM machine.
+--
    received LSPs.  [ISO10589] suggests that some delay be used when
    sending PSNPs.  This provides some optimization as multiple LSPs can
    be acknowledged by a single PSNP.
@@ -541,6 +571,12 @@
    as per the OSI model.  A typical example is the TCP protocol defined
    in [RFC9293] that relies on the flow control, congestion control, and
    reliability mechanisms of the protocol.
+--
+jgs: this doesn't quite make sense as written. TCP doesn't "rely on"
+TCP, that would be a tautology. I'm not sure what the best way to
+rewrite it is, perhaps "... that provides flow control, congestion
+control, and reliability"?
+--
 
    Flow control creates a control loop between a transmiter and a
    receiver so that the transmitter does not overwhelm the receiver.
@@ -551,7 +587,7 @@
    multiple transmitters and multiple receivers to prevent the
    transmitters from overwhelming the overall network.  For an IS-IS
    adjacency, the network between two IS-IS neighbors is relatively
-   limited in scope and consist of a link that is typically over-sized
+   limited in scope and consists of a link that is typically over-sized
    compared to the capability of the IS-IS speakers, but may also
    include components inside both routers such as a switching fabric,
 
@@ -608,6 +644,9 @@
    implementations, this value should reflect the IS-IS socket buffer
    size.  Special care must be taken to leave space for CSNP and PSNP
    (SNP) PDUs and IIHs if they share the same input queue.  In this
+--
+jgs: Same "ATM machine" nit applies here.
+--
    case, this document suggests advertising an LSP Burst Window
    corresponding to half the size of the IS-IS input queue.
 
@@ -623,9 +662,13 @@
    advertised value, outside of LSP bursts.
 
    The LSP transmitter MUST NOT exceed these parameters.  After having
-   sent a full burst of un-acknowledged LSPs, it MUST send the following
+   sent a full burst of un-acknowledged LSPs, it MUST send the subsequent
    LSPs with an LSP Transmission Interval between LSP transmissions.
    For CPU scheduling reasons, this rate may be averaged over a small
+--
+jgs: this is one of those rare times when I suggest replacing "may" with 
+"MAY".
+--
    period, e.g., 10-30ms.
 
    If either the LSP transmitter or receiver does not adhere to these
@@ -733,7 +776,7 @@
 6.2.2.2.  Congestion signals
 
    The congestion signal can take various forms.  The more reactive the
-   congestion signals, the less LSPs will be lost due to congestion.
+   congestion signals, the fewer LSPs will be lost due to congestion.
    However, overly aggressive congestion signals will cause a sender to
    keep a very low sending rate even without actual congestion on the
    path.
@@ -755,7 +798,7 @@
 
    There are multiple strategies to set the timeout value t1.  It should
    be based on measures of the maximum acknowledgement time (MAT) of
-   each PSNP.  The simplest one is to use a exponential moving average
+   each PSNP.  The simplest one is to use an exponential moving average
    of the MATs, like [RFC6298].  A more elaborate one is to take a
    running maximum of the MATs over a period of a few seconds.  This
    value should include a margin of error to avoid false positives
@@ -765,7 +808,7 @@
    Reordering: a sender can record its sending order and check that
    acknowledgements arrive in the same order as LSPs.  This makes an
    additional assumption and should ideally be backed up by a
-   confirmation by the receiver that this assumption stands.  The O flag
+   confirmation by the receiver that this assumption holds.  The O flag
    defined in Section 4.4 serves this purpose.
 
 6.2.2.3.  Refinement 1
@@ -788,6 +831,9 @@
 
    It is possible to use a fast recovery phase once congestion is
    detected, to avoid going through this linear rate of growth from
+--
+jgs: Surely cwin += 1/cwin is sub-linear?
+--
    scratch.  When congestion is detected, a fast recovery threshold
    frthresh is set to frthresh = cwin / 2.  In this fast recovery phase,
    for every acked LSP, cwin += 1.  Once cwin reaches frthresh, the
@@ -870,7 +916,12 @@
    Expressed as an inter-packet interval in units of time:
 
    interval = (smoothed_rtt / congestion_window) / N
-
+--
+jgs: You haven't defined smoothed_rtt or congestion_window. It's readily
+apparent that congestion_window is cwin, but please be consistent in your
+terminology. As for smoothed_rtt, this is the only occurrence of "smooth"
+in the document. 
+--
    Using a value for N that is small, but at least 1 (for example, 1.25)
    ensures that variations in RTT do not result in underutilization of
    the congestion window.
@@ -908,6 +959,11 @@
    loss will reduce the usable receive window and the protocol
    mechanisms will allow the adjacency to recover.  Flooding several
    orders of magnitude slower than both nodes can support will hurt
+--
+jgs: is "several orders of magnitude" doing any useful work here? I would
+Think that simply flooding slower than both nodes can support will hurt 
+performance, and the reader can figure out the rest.
+--
    performance, as will consistently overloading the receiver.
 
    The values advertised need not be dynamic as feedback is provided by
@@ -918,12 +974,19 @@
    advertising relatively static parameters, we expect to produce
    overall flooding behavior similar to what might be achieved by
    manually configuring per-interface LSP rate-limiting on all
-   interfaces in the network.  The advertised values may be based, for
-   example, on an offline tests of the overall LSP-processing speed for
+   interfaces in the network.  The advertised values could be based, for
+   example, on offline tests of the overall LSP-processing speed for
    a particular set of hardware and the number of interfaces configured
    for IS-IS.  With such a formula, the values advertised in the
    Flooding Parameters TLV would only change when additional IS-IS
    interfaces are configured.
+--
+jgs: It took me a while to understand the above, I proposed some
+clarifying/correcting edits. I also wonder if "would only change" should
+more properly be "would only be changed" since I think the assumption is
+the parameters are being configured by system management (i.e.,
+manually).
+--
 
    The values may be updated dynamically, to reflect the relative change
    of load on the receiver, by improving the values when the receiver
@@ -935,6 +998,11 @@
 
    The values may also be absolute value reflecting relevant average
    hardware resources that are been monitored, typically the amount of
+--
+jgs: "that are been" is definitely wrong, but I'm not sure what you're
+trying to say so can't propose a fix, other than perhaps you could
+delete "that are been monitored".
+--
    buffer space used by incoming LSPs.  In this case, care must be taken
    when choosing the parameters influencing the values in order to avoid
    undesirable or unstable feedback loops.  It would be undesirable to
@@ -983,7 +1051,7 @@
    packets will be punted.
 
    An input queue in the control plane may then be used to assemble PDUs
-   from multiple linecards, separate the IS-ISs PDU from other types of
+   from multiple linecards, separate the IS-IS PDUs from other types of
    packets, and place the IS-IS PDUs on an input queue dedicated to the
    IS-IS protocol.
 
@@ -1014,7 +1082,7 @@
 
    The congestion control algorithm described in this section does not
    depend upon direct signaling from the receiver.  Instead it adapts
-   the tranmsmission rate based on measurement of the actual rate of
+   the transmission rate based on measurement of the actual rate of
    acknowledgments received.
 
    When flow control is necessary, it can be implemented based on
@@ -1040,7 +1108,7 @@
    LSPTxRate.
 
    The flow control algorithm MUST NOT assume the receive performance of
-   a neighbor are static, i.e., it MUST handle transient conditions
+   a neighbor is static, i.e., it MUST handle transient conditions
    which result in a slower or faster receive rate on the part of a
    neighbor.
 
@@ -1056,7 +1124,8 @@
 7.1.  Flooding Parameters TLV
 
    IANA has made the following temporary allocation from the IS-IS TLV
-   codepoint registry.
+   codepoint registry. This document requests the allocation be made
+   permanent.
 
 
 
@@ -1075,6 +1144,10 @@
 7.2.  Registry: IS-IS Sub-TLV for Flooding Parameters TLV
 
    This document creates the following sub-TLV Registry:
+--
+jgs: we've already discussed the need to say what group the registry is
+under, for this and the following.
+--
 
    Name: IS-IS Sub-TLVs for Flooding Parameters TLV.
 
@@ -1155,7 +1228,7 @@
 8.  Security Considerations
 
    Security concerns for IS-IS are addressed in [ISO10589] , [RFC5304] ,
-   and [RFC5310] .  These documents describe mechanisms that provide the
+   and [RFC5310] .  These documents describe mechanisms that provide for the
    authentication and integrity of IS-IS PDUs, including SNPs and IIHs.
    These authentication mechanisms are not altered by this document.
 
@@ -1165,7 +1238,7 @@
 
    In the absence of cryptographic authentication, as IS-IS does not run
    over IP but directly over the link layer, it's considered difficult
-   to inject false SNP/IHH without having access to the link layer.
+   to inject false SNP/IIH without having access to the link layer.
 
    If a false SNP/IIH is sent with a Flooding Parameters TLV set to
    conservative values, the attacker can reduce the flooding speed