Dear Boris, Many thanks for this thorough review!
It took us some time to get back to you, but your feedback helped a lot to improve the document. We believe that the changes we made as part of revision -09 ( https://datatracker.ietf.org/doc/draft-ietf-spring-srv6-srh-compression/9/) should address most of your feedback. We are still working on some of your comments and will address them in a future revision of the document. Please also see inline below our response to each of your comments. Thanks, Francois (on behalf of the C-SID co-authors) > *BEGINNING of the review* > > 1) Part 1.Introduction (p. 2) has incorrect reference: > > 1.1 "These flavors enable a compressed encoding of the SRv6 Segment-List > in the SRH and therefore address the requirements described in > [I-D.srcompdt-spring-compression-requirement]." > > It has to be draft-ietf-spring-compression-requirement. We removed the reference in revision -09. > 2) Part 2. Terminology > > 2.1 "Locator-Block: The SRv6 SID block (IPv6 prefix allocated for SRv6 > SIDs by the operator) of an SRv6 SID Locator, as defined in > Section 3.1 of [RFC8986]." > > IMO, sounds a bit strange for understanding, I mean this one:"The > SRv6 SID block of an SRv6 SID Locator." > > I think it has to be re-phrased: "IPv6 prefix which is used for > allocation of SRv6 SIDs by the operator". > > Especially if Section 3 says:" In an SRv6 domain, the SIDs are > allocated from a particular IPv6 prefix: the Locator-Block. All SRv6 > SIDs instantiated from the same Locator-Block share the same most > significant bits." We rephrased this definition in revision -09: | * Locator-Block: The most significant bits of a SID locator that | represent the SRv6 SID block. The Locator-Block is referred to as | "B" in Section 3.1 of [RFC8986]. > 2.2 "Locator-Node: The identifier of the parent node instantiating a SID > in an SRv6 SID Locator, as defined in Section 3.1 of [RFC8986]." > > I guess that the reference to Section 3.1 of RFC8986 might not > becorrect ( It describes just format of SID, allocation is in > Section 3.2) We rephrased this definition and clarified the reference to RFC 8986 in revision -09: | * Locator-Node: The least significant bits of a SID locator that | identify the SR segment endpoint node instantiating the SID. The | Locator-Node is referred to as "N" in Section 3.1 of [RFC8986]. > 3) Section 3. Basic concepts > > 3.1 "The compressed Segment List encoding supports any Locator-Block > allocation.While other options are supported and may provide higher > efficiency, each routing domain can be allocated a /48 prefix from a > global IPv6 block (see Section 6.2)." > > Which other options? Which routing domain can be allocated? Sounds > confusing... > > If the authors meant IPv6 prefix length so it should be explicitly > mentioned: "While other options of IPv6 prefix length allocation for > Locator-Block are supported it is RECOMMENDED to allocate a /48 > prefix from a global IPv6 block (see Section 6.2)." We rephrased this paragraph in revision -09: | The compressed segment list encoding works with any Locator-Block | allocation. For example, each routing domain within the SR domain | can be allocated a /48 Locator-Block from a global IPv6 block available to the operator, or from a prefix allocated to SRv6 SIDs as | discussed in Section 5 of [I-D.ietf-6man-sids]. > 4) SR Segment Endpoint Flavors > > 4.1 "This section defines several options to achieve compressed > Segment List encoding in the form of two new flavors for the End, > End.X, End.T, End.B6.Encaps, End.B6.Encaps.Red, and End.BM behaviors > of [RFC8986].These flavors could also be combined with behaviors > defined in other documents. " > > Which two flavors are mentioned here? it is not clear for the reader. > I would suggest to mention those flavors at the beginning because > there are many references for them in this section. For example, the > authors wrote: > > "The compressed encoding can be achieved by leveraging any of these > SR segment endpoint flavors." > > Next : "The NEXT-C-SID flavor and the REPLACE-C-SID flavor..." > > The reader should guess that these NEXT-C-SID and REPLACE-C-SID > flavors are those which were mentioned. IMO it is not quite correct. > I would change the structure of this section like this: > > " This section defines several options to achieve compressed > Segment List encoding in the form of two new flavors > (NEXT-C-SID, REPLACE-C-SID ) for the End, End.X, End.T, > End.B6.Encaps, End.B6.Encaps.Red, and End.BM behaviors of [RFC8986]. > Theseflavors could also be combined with behaviors defined in other > documents. > > ... " We rephrased this sentence in revision -09 based on your suggestion: | This section defines two SR segment endpoint flavors, NEXT-C-SID and | REPLACE-C-SID, for the End, End.X, End.T, End.B6.Encaps, | End.B6.Encaps.Red, and End.BM behaviors of [RFC8986]... > 4.2 This sentence is too long and hard for understanding: "The > NEXT-C-SID flavor and the REPLACE-C-SID flavor expose the same > high-level behavior in their use of the SID argument to determine the > next segment to be processed, but they have different low-level > characteristics that can make one more or less efficient than the > other for a particular SRv6 deployment." > > I would split and slightly change it:"The NEXT-C-SID f and the > REPLACE-C-SID flavors expose the same high-level behavior in their > use of the SID argument to determine which next segment will be > processed. The NEXT-C-SID f and the REPLACE-C-SID flavors have > different low-level characteristics that can make one of them more or > less efficient than the other for a particular SRv6 deployment." We simplified this sentence in revision -09 as follows: | The NEXT-C-SID flavor and the REPLACE-C-SID flavor both leverage the | SID Argument to determine the next segment to be processed, but | employ different segment list compression schemes. We then go on with a short description of both flavors. > 4.3 Here is the quite strange mapping of abbreviations and their > explanations: > > " Both flavors leverage the following *variables*: > > **Variable *LBL is the Locator-Block length of the SID. > > **Variable *LNFL is the sum of the Locator-Node and the Function > lengths of the SID.It is also referred to as C-SID length. > > **Variable *AL is the Argument length of the SID." > > Why do not write excluding the duplication of the word ‘variables’ > (also LBL, LNFL, AL could be added to Terminology section) : > > " Both flavors leverage the following variables: > > *LBL - the Locator-Block length of the SID. > > *LNFL - the sum of the Locator-Node and the Function lengths of the > SID.It is also referred to as C-SID length. > > *AL - the Argument length of the SID." We moved this text to the terminology section and slightly rephrased it to avoid the word "variable" (which wasn't so correct) in revision -09: | In this document, the length of each constituent part of a SID is | referred to as follows. | | * LBL is the Locator-Block length of the SID. | * LNL is the Locator-Node length of the SID. | * FL is the Function length of the SID. | * AL is the Argument length of the SID. | | In addition, LNFL is the sum of the Locator-Node length and the | Function length of the SID. It is also referred to as the C-SID | length. > 5) 4.1. NEXT-C-SID Flavor > 5.1 " A SID instantiated with the NEXT-C-SID > flavor takes an argument that carries the remaining C-SIDs in the > current C-SID container." > > I would change it for the better clarity like this: > > " A SID instantiated with the NEXT-C-SID flavor holds an argument > that carries the remaining C-SIDs in the current C-SID container. " > > I am not native English-speaker but it looks that "takes" is too > broad here (I may be mistaken of course). We clarified this sentence in revision -09 by adding some more explanatory text: | A C-SID sequence using the NEXT-C-SID flavor comprises one or more | C-SID containers. Each C-SID container is a fully formed 128-bit | SID. It carries a Locator-Block followed by a series of C-SIDs. The | Locator-Node and Function of the C-SID container are those of the | first C-SID, and its Argument is the contiguous series of subsequent | C-SIDs. > 5.2 " The length AL of the argument is equal to 128-LBL-LNFL." -> > "The length of AL calculates as AL == 128-LBL-LNFL." The current phrasing is probably not very good, but not sure if "calculates as" would be so much better. It might help to get the viewpoint of a native English speaker here. > 5.3 " An SR segment endpoint node instantiating a SID with the > NEXT-C-SID flavor MUST accept any argument value for that SID." > > -> "An SR segment endpoint node that instantiates a SID with the > NEXT-C-SID flavor MUST accept any argument value for that SID. " Isn't it the same? > 5.4 I would prefer to see an explanation diagram after that paragraph: > > "A C-SID sequence using NEXT-C-SID comprises one or more C-SID > containers, each carrying the common Locator-Block followed by a > series of C-SIDs.The Locator-Node and Function of the C-SID container > are those of the first C-SID, and its Argument is the contiguous > series of subsequent C-SIDs." > > 5.5 Some diagram is needed after this too: > > "When processing a SID with the NEXT-C-SID flavor, while the Argument > value is non-zero, the SR segment endpoint node constructs the next > SID by copying the SID Argument value immediately after the Locator- > Block, thus overwriting the previous SID Locator-Node and Function, > and filling the least significant bits of the argument with zeros." > > 5.6 Hard for understanding - why instead and instead of what? > > "When the Argument value is 0, the SR segment endpoint node *instead > *copies the next 128-bit Segment List entry from the SRH to the > Destination Address field of the IPv6 header. " > > Again, I may be mistaken but it looks that it should be written as : > "When the Argument value is 0, the SR segment endpoint node copies > the next 128-bit Segment List entry from the SRH to the Destination > Address field of the IPv6 header." > > 6) 4.1.1. End with NEXT-C-SID > > 6.1 "When processing an IPv6 packet that matches a FIB entry locally > instantiated as an End SID with the NEXT-C-SID flavor, the procedure > described in Section 4.1 of [RFC8986] is executed with the following > modifications." -> > > " When a node processes an IPv6 packet that matches a locally > instantiated FIB entry as an End SID with the NEXT-C-SID flavor, the > procedure described in Section 4.1 of [RFC8986] is executed with the > following modifications: ..." We clarified in revision -09 (section 4.1) that this packet processing is applied by the SR segment endpoint node where the SID is instantiated. | When processing an IPv6 packet that matches a FIB entry locally | instantiated as a SID with the NEXT-C-SID flavor, the SR segment | endpoint node applies the procedure specified in the one following | subsection that corresponds to the SID behavior. > 6.2 > > "... with the following modifications: > > - The below pseudocode is inserted between lines S01 and S02 of the > SRH processing in Section 4.1 of [RFC8986]; > > -The below pseudocode is inserteda second time before line S01 of the > upper-layer header processing in Section 4.1.1 of [RFC8986], or prior > to processing any extension header other than Hop-by-Hop or > Destination Option..." > > It is not really clear how this pseudocode should be finally > inserted: 1 time between lines S01/S02 then again after line S01, so > two times? > > I also think that the 2nd item should be corrected to clarify the > point (when this pseudocode will be inserted 2nd time after line S01? > Which conditions if any ?). We clarified in revision -09 how the pseudocode modification is applied. | The below pseudocode is inserted between lines S01 and S02 of the SRH | processing in Section 4.1 of [RFC8986]. In addition, this pseudocode | is executed before processing any extension header that is not an | SRH, a Hop-by-Hop header or a Destination Option header, or before | processing the upper-layer header, whichever comes first. We also applied this change to the similar text in sections 4.1.2 through 4.1.6. > 7)4.1.2.End.X with NEXT-C-SID > > 7.1 Same as in 6.1, I would suggest the following change: > > " When a node processes an IPv6 packet that matches a locally > instantiated FIB entry as an End.X SID with the NEXT-C-SID flavor, > the procedure described in Section 4.2 of [RFC8986] is executed with > the following modifications: ..." > Please see the reply to comment 6.1. > 7.2 Same comment about pseudocode insertion as in6.2 Please see the reply to comment 6.2. > 8)4.1.3.End.T with NEXT-C-SID > > 8.1 Same as above, I would suggest the following change: > > "When a node processes an IPv6 packet that matches a locally > instantiated FIB entry as an End.T SID with the NEXT-C-SID flavor, > the procedure described in Section 4.3 of [RFC8986] is executed with > the following modifications: ..." Please see the reply to comment 6.1. > 8.2 "The pseudocode in Section 4.1.1 of this document is modified by > replacing line S08 as shown below." > > I think instead of reference to the section 4.1 and suggestion for > changing lines S08, would be much clearer to place the link to > Appendix B with whole new pseudocode (to exclude any mistake in > interpretation) > > 8.3 The pseudocode insertion description here is also confusing as > before, so same comment as in 6.2: it must be clarified. Please see the reply to comment 6.2. > 9)4.1.4.End.B6.Encaps with NEXT-C-SID > > 9.1 Same as above, I would suggest the following change: > > "When a node processes an IPv6 packet that matches a locally > instantiated FIB entry as an End.B6.Encaps SID with the NEXT-C-SID > flavor, the procedure described in Section 4.13 of [RFC8986] is > executed with the following modifications: ..." Please see the reply to comment 6.1. > 9.2 Same as in 8.2, would be better to place the link to Appendix B > with whole updated pseudocode > > 9.3 The pseudocode insertion description here is also confusing as > before, so same comment as in 6.2: it must be clarified too. Please see the reply to comment 6.2. > 10) 4.1.6.End.BM with NEXT-C-SID > > 10.1 Same as above, I would suggest the following change: > > "When a node processes an IPv6 packet that matches a locally > instantiated FIB entry as an End.BM SID with the NEXT-C-SID flavor, > the procedure described in Section 4.15 of [RFC8986] is executed with > the following modifications: ..." Please see the reply to comment 6.1. > 10.2 Same as in 8.2, would be better to place the link to Appendix B > with whole updated pseudocode. > > 10.3 The pseudocode insertion description here is also confusing as > before, so same comment as in 6.2: it must be clarified too. Please see the reply to comment 6.2. > 11) 4.2.REPLACE-C-SID Flavor > > 11.1 "A SID instantiated with the REPLACE-C-SID flavor takes an > argument that indicates the index of the next C-SID in the > appropriate C-SID container." -> > > " A SID instantiated with the NEXT-C-SID flavor holds an argument > that carries the remaining C-SIDs in the current C-SID container. " We clarified this sentence in revision -09 as follows: | A C-SID sequence using the REPLACE-C-SID flavor starts with a C-SID | container in fully formed 128-bit SID format. The Locator-Block of | this SID is the common Locator-Block for all the C-SIDs in the C-SID | sequence, its Locator-Node and Function are those of the first C-SID, | and its Argument carries the index of the current C-SID in the | current C-SID container. > 11.2 As in 5.2:"The length of AL calculates asAL == 128-LBL-LNFL." Please see the reply to comment 5.2. > 11.3 " The index value is encoded in the least significant > ceil(log_2(128/LNFL)) bits of the argument field." > > Which index? What is a ceil here? If a rounding function was meant > (i.e. ceil in Python) it should be explicitly clarified in the text > or better changing. We clarified this sentence in revision -09 as follows: | The index value is encoded in the least significant X bits of the | Argument, where X is computed as ceil(log_2(128/LNFL)). The term "ceil(x)" refers to the ceiling function in mathematics ( https://en.wikipedia.org/wiki/Floor_and_ceiling_functions). This function is implemented in the base libraries of many programming languages, but the term is language-agnostic. > 11.4 This item needs to be illustrated by a diagram, IMO: > > " A C-SID sequence using REPLACE-C-SID starts with an uncompressed > REPLACE-C-SID flavored SID (as shown in Figure 2) carrying the common > Locator-Block, the Locator-Node and Function of the first C-SID, and > an argument with the index value 0.This first SID is copied into the > Destination Address field of the IPv6 header either at the SR source > node, if it is in the first position in the segment list, or at the > previous SR segment endpoint node." This is illustrated in Figure 2: | The structure of a SID with the REPLACE-C-SID flavor is shown in | Figure 2. The same structure is also that of the C-SID container for | REPLACE-C-SID in fully formed 128-bit SID format. | | +-------------------------------------------------------------------+ | | Locator-Block | Locator-Node | Argument | | | | + Function | | | +-------------------------------------------------------------------+ | <-------- LBL ---------> <---- LNFL ----> <--------- AL ----------> > 11.5 " When processing a SID with the REPLACE-C-SID flavor, if no SRH > is present, the SR segment endpoint node ignores the index value in > the SID argument and processes the upper-layer header as per [RFC8986]." > > -> "When processing a SID with the REPLACE-C-SID flavor, if no SRH is > present, the SR segment endpoint node MUST ignore the index value in > the SID argument and process the upper-layer header as per [RFC8986]." > > MUST or SHOULD, IMO. We clarified in revision -09 that this text is merely a high-level description of a generic REPLACE-C-SID flavor processing, and that the normative behaviors are described in the following subsections. | At high level, at the start of a C-SID sequence using the REPLACE-C- | SID flavor, the first C-SID container in fully formed 128-bit SID | format is copied to the Destination Address of the IPv6 header. Then, | for any SID with the REPLACE-C-SID flavor, the SR segment endpoint | node determines the next SID of the SID list as follows. When an SRH | is present, the SR segment endpoint node decrements the index value | in the Argument of the active SID if the index value is not 0 or, if | it is 0, decrements the Segments Left value in the SRH and sets the | index value in the Argument of the active SID to K-1. The updated | index value indicates the position of the next C-SID within the C-SID | container in packed format at the "Segment List" index "Segments | Left" in the SRH. The SR segment endpoint node then constructs the | next SID by copying this next C-SID to the bits that immediately | follow the Locator-Block in the Destination Address field of the IPv6 | header, thus overwriting the active SID Locator-Node and Function | with those of the next C-SID. If no SRH is present, the SR segment | endpoint node ignores the index value in the SID Argument (except | End.DT2M, see Section 4.2.7) and processes the upper-layer header as | per [RFC8986]. The C-SID sequence ends with a last C-SID in the last | C-SID container that does not have the REPLACE-C-SID flavor, or with | the special C-SID value 0, or when reaching the end of the segment | list, whichever comes first. > 11.6 " If an SRH is present, the SR segment endpoint node > decrements the index value in the SID argument or, if it is 0, > instead decrements the Segments Left value in the SRH and resets the > index value in the SID argument to the C-SID container capacity minus 1." > > ->" If an SRH is present, the SR segment endpoint node MUST decrement > the index value in the SID argument or, if it is 0 itMUST decrement > the Segments Left value in the SRH instead and it MUST reset the > index value in the SID argument to the C-SID container capacity minus 1." > > May be my proposal is not ideal but definitely the verbs like > ignores/decrements/resets are too inconcrete for the such important > draft, IMO. Please see the reply to comment 11.5. > 11.7 "The updated index value indicates the position of the next C-SID > within the C-SID container at the "Segment List" index "Segments > Left" in the SRH." > > I cannot get that: "... at the "Segment List" index "Segments Left" > in the SRH." Surely it must be re-phrased or accompanied by some diagram. This phrase is the same as "Segment List[Segments Left]" in RFC 8754 and RFC 8986, only in plain English instead of pseudocode. > 11.8 "The SR segment endpoint node then constructs the next SID by > copyingthis next C-SID immediately after the Locator-Block in the > Destination Address field of the IPv6 header, thus overwriting the > previous SID Locator-Node and Function." -> > > "The SRv6 segment endpoint node then SHOULD construct the next SID by > copying this next C-SID immediately after the Locator-Block in the > Destination Address field of the IPv6 header, thus overwriting the > previous SID Locator-Node and Function." Please see the reply to comment 11.5. > 12) 4.2.1.End with REPLACE-C-SID > > 12.1 "4.2.1.End with REPLACE-C-SID" -> "4.2.1End SID with REPLACE-C-SID" Not sure how this would help. The behavior name is used consistently alone in the section titles in RFC 8986 and in this document. > 12.2 "When processing an IPv6 packet that matches a FIB entry locally > instantiated as an End SID with the REPLACE-C-SID flavor, the SRH > processing described in Section 4.1 of [RFC8986] is executed with the > following modifications." -> > > " When a node processes an IPv6 packet that matches a locally > instantiated FIB entry as an End SID with theREPLACE-C-SID flavor,the > procedure described in Section 4.1 of [RFC8986] is executed with the > following modifications: ..." We clarified in revision -09 (section 4.2) that this packet processing is applied by the SR segment endpoint node where the SID is instantiated. | When processing an IPv6 packet that matches a FIB entry locally | instantiated as a SID with the REPLACE-C-SID flavor, the SR segment | endpoint node applies the procedure specified in the one following | subsection that corresponds to the SID behavior. > 12.3 End.X with REPLACE-C-SID > 12.3.1 “When processing an IPv6 packet that matches a FIB entry > locally instantiated as an End.X SID with the REPLACE-C-SID flavor, > the procedure described in Section 4.2 of [RFC8986] is executed with > the following modifications.“ -> > "When a node processes an IPv6 packet that matches a locally > instantiated FIB entry as an End.X SID with the REPLACE-C-SID flavor, > the procedure described in Section 4.2 of [RFC8986] is executed with > the following modifications: ..." Please see the reply to comment 12.2. > 12.3.2 "The pseudocode in Section 4.2.1 of this document is modified by replacing line S18 and S29 as shown below. S01. Submit the packet to the IPv6 module for transmission to the new destination via a member of J." > > I see here only line S01, where are the replacement for lines S18 and > S09? We clarified this in revision -09 as follows: | The pseudocode in Section 4.2.1 of this document is modified by | replacing line R10 and R21 as shown below. | | R10. Submit the packet to the IPv6 module for transmission to the | new destination via a member of J. | | R21. Submit the packet to the IPv6 module for transmission to the | new destination via a member of J. > Who is "a member of J."! is not clear from the text? RFC 8986 > Section 4.2 has that information: J is a set that contains several L3 > adjacencies. Because reader should not jump between different > documents for such details clarification, I think it will be much > easier to add that info in this section of the draft: > > “ S01. Submit the packet to the IPv6 module for transmission to the > new destination via a member of J.,where J is a set that contains > several L3 adjacencies “ We added an explicit reference to the definition of variable J in revision -09: | Note: the variable J is defined in Section 4.2 of [RFC8986]. However we would prefer for consistency to avoid redefining variables from other RFCs. > 13) 4.2.3.End.T with REPLACE-C-SID > > 13.1 "When processing an IPv6 packet that matches a FIB entry locally > instantiated as an End.T SID with the REPLACE-C-SID flavor, the > procedure described in Section 4.3 of [RFC8986] is executed with the > following modifications." -> " When a node processes an IPv6 packet > that matches a locally instantiated FIB entry as an End SID with > theREPLACE-C-SID f flavor, the procedure described in Section 4.3 of > [RFC8986] is executed with the following modifications: ..." Please see the reply to comment 12.2. > 13.2 "The pseudocode in Section 4.2.1 of this document is modified by > replacing line S18 and S29 as shown below. > > S01. Set the packet's associated FIB table to T. > > S02. Submit the packet to the egress IPv6 FIB lookup for transmission > to the new destination." > > So lines S18 and S29 should be replaced by S01, S02? Hm-m, sounds > confusing for the reader. We clarified this in revision -09 as follows: | The pseudocode in Section 4.2.1 of this document is modified by | replacing line R10 and R21 as shown below. | | R10.1. Set the packet's associated FIB table to T. | R10.2. Submit the packet to the egress IPv6 FIB lookup for | transmission to the new destination. | | R21.1. Set the packet's associated FIB table to T. | R21.2. Submit the packet to the egress IPv6 FIB lookup for | transmission to the new destination. > 14)4.2.4.End.B6.Encaps with REPLACE-C-SID > > 14.1 "When processing an IPv6 packet that matches a FIB entry > locally instantiated as an End.B6.Encaps SID with the REPLACE-C-SID > flavor, the procedure described in Section 4.13 of [RFC8986] is > executed with the following modifications." > > ->"When a node processes an IPv6 packet that matches a locally > instantiated FIB entry as an End.B6.Encaps SID with theREPLACE-C-SID > flavor, the procedure described in Section 4.13 of [RFC8986] is > executed with the following modifications: ..." Please see the reply to comment 12.2. > 14.2 Confusion again with pseudocode lines numbering: > > "The pseudocode in Section 4.2.1 of this document is modified by > replacing line S18 and S29 as shown below." > > But again, the example has the lines S01-S05, how to map them to S18 > and S29? Each vendor should decide by himself? We clarified this in revision -09 as follows: | The pseudocode in Section 4.2.1 of this document is modified by | replacing line R10 and R21 as shown below. | | R10.1. Push a new IPv6 header with its own SRH containing B. | R10.2. Set the outer IPv6 SA to A. | R10.3. Set the outer IPv6 DA to the first SID of B. | R10.4. Set the outer Payload Length, Traffic Class, Flow Label, | Hop Limit, and Next Header fields. | R10.5. Submit the packet to the egress IPv6 FIB lookup for | transmission to the next destination. | | R21.1. Push a new IPv6 header with its own SRH containing B. | R21.2. Set the outer IPv6 SA to A. | R21.3. Set the outer IPv6 DA to the first SID of B. | R21.4. Set the outer Payload Length, Traffic Class, Flow Label, | Hop Limit, and Next Header fields. | R21.5. Submit the packet to the egress IPv6 FIB lookup for | transmission to the next destination. > 15)4.2.5.End.B6.Encaps.Red with REPLACE-C-SID > > 15.1 "When processing an IPv6 packet that matches a FIB entry locally > instantiated as an End.B6.Encaps.Red SID with the REPLACE-C-SID > flavor, the procedure described in Section 4.14 of [RFC8986] is > executed with the same modifications as in Section 4.2.4 of this > document."-> > > "When a node processes an IPv6 packet that matches a locally > instantiated FIB entry as an End.B6.Encaps SID with theREPLACE-C-SID > flavor, the procedure described in Section 4.14 of [RFC8986] is > executedwith the same modifications as in Section 4.2.4 of this > document." Please see the reply to comment 12.2. > 16)4.2.6.End.BM with REPLACE-C-SID > > 16.1 "When processing an IPv6 packet that matches a FIB entry > locally instantiated as an End.BM SID with the REPLACE-C-SID flavor, > the procedure described in Section 4.15 of [RFC8986] is executed with > the following modifications." -> "When a node processes an IPv6 > packet that matches a locally instantiated FIB entry as an End.BM SID > with theREPLACE-C-SID flavor, the procedure described in Section 4.15 > of [RFC8986] is executed with the following modifications: ..." Please see the reply to comment 12.2. > 16.2 Again issue with pseudocode numbering (S18, S29 -> S01, S02) as > I already mentioned above. We clarified this in revision -09 as follows: | The pseudocode in Section 4.2.1 of this document is modified by | replacing line R10 and R21 as shown below. | | R10.1. Push the MPLS label stack for B. | R10.2. Submit the packet to the MPLS engine for transmission. | | R21.1. Push the MPLS label stack for B. | R21.2. Submit the packet to the MPLS engine for transmission. > 16.3 "S01. Push the MPLS label stack for B." > > In RFC 8986 Section 4.15 is a special comment which clarifies what is > B: " Any SID instance of this behavior is associated with an SR-MPLS > Policy B.". But in this draft it is not clarified and looks confusing > (a reference or clarification is needed.) We added an explicit reference to the definition of variable B in revision -09: | Note: the variable B is defined in Section 4.15 of [RFC8986]. > 17) 4.2.7. End.DX and End.DT with REPLACE-C-SID > > 17.1 ".... These new behaviors can be used to indicate the capability > of compression of Node and SID, which are required in path > computation and compressed SID list encoding." > > "Can be used" ? May be at least " MAY or SHOULD be used"? > > 17.2 "As per Section 6.4, when allocating a C-SID value from a > Local Identifiers Block (LIB), an extra prefix of > Locator_block:FunctionID::/LBL+FL is required on the Segment > Endpointnode to support LIB matching in forwarding.The new behaviors > with REPLACE-C-SID flavor explicitly require the node to do so in SID > initiation." > > I did not get - why an extra prefix is required from this text... :( > Looks I just have to believe. We removed this paragraph in revision -09, since it is redundant with section 5.4. As explained in the first paragraph of section 5.4, the prefix matching is one method that an implementer may use to identify a SID with argument on the SR segment endpoint node. > 17.2.1 I think LIB abbreviation has to be added to Terminology part. > > 17.3 "When processing an IPv6 packet that matches a FIB entry > locally instantiated as an End.DX6, End.DX4, End.DT6, End.DT4, > End.DT46, End.DX2, End.DX2V, End.DT2U or End.DT2M SID with the > REPLACE-C-SID flavor, the procedures described in [RFC8986] are > executed. " -> " When a node processes an IPv6 packet that matches a > FIB entry locally instantiated as an End.DX6, End.DX4, End.DT6, > End.DT4, End.DT46, End.DX2, End.DX2V, End.DT2U or End.DT2M SID with > the REPLACE-C-SID flavor, the procedures described in [RFC8986] are > executed." Please see the reply to comment 12.2. > 17.4 The next paragraph is really confusing - it is a huge and I do > not understand its logic: why by some reasons it focuses only on > End.DT2M... > > It also constantly referencing Arg.FE2 without any explanation, so > reader have to go to read RFC 8986, find section 4.15 there and find > its meaning. So I would suggest to add Arg.FE2 to Terminology part > too if it is so important (again I did not understand - why). I would > also add some explanation diagram here. > > 18) 4.2.8. Combination with PSP, USP, and USD flavors > > 18.1 "PSP: When combined with the REPLACE-C-SID flavor, the > additional PSP flavor instructions defined in Section 4.16.1.2 of > [RFC8986] are inserted after line S17 and S28 of the pseudocode in > Section 4.2.1, and the first line of the inserted instructions after > S28 is modified as follows." -> > > "PSP: When combined with the REPLACE-C-SID flavor, the additional PSP > flavor instructions defined in Section 4.16.1.2 of [RFC8986] are > inserted after *lines*S17 and S28 of the pseudocode in Section 4.2.1, > and the first line of the inserted instructions after S28 is modified > as follows:" > > It says that those additional instructions added after lines S17 and S28. > > But later only the instruction S.28.1 is mentioned, what about S17.1 ? The text says that the "instructions defined in Section 4.16.1.2 of [RFC8986] are inserted after lines S17 and S28" (R09 and R20 in revision -09). These inserted instructions are: | S14.1. If (Segments Left == 0) { | S14.2. Update the Next Header field in the preceding header to | the Next Header value from the SRH | S14.3. Decrease the IPv6 header Payload Length by | 8*(Hdr Ext Len+1) | S14.4. Remove the SRH from the IPv6 extension header chain | S14.5. } The text then goes on saying that "the first line of the inserted instructions after S28 (R20 in revision -09) is modified as follows", and provides the corresponding modification. This modification is only applied to the second occurrence of the inserted pseudocode (after S28/R20), but not to the first one (after S17/R09). > 18.2 "S28.1.If (Segments Left == 0 and (DA.Arg.Index == 0 or Segment > List[0][DA.Arg.Index-1] == 0)) { " > > I also think that final open { mightnot be needed here, better to > double check. Since the original line (`If (Segments Left == 0) {`) ends with an opening curly bracket, it seems better to keep it for correctness and consistency. > 19) 5.C-SID Allocation > > 19.1 "In order to efficiently manage the C-SID numbering space, it > may be beneficial to divide it into two non-overlapping sub-spaces: a > Global Identifiers Block (GIB) and a Local Identifiers Block (LIB). " > > GIB has to be added to Terminology part too. > > "it may be beneficial" sounds too open and inconcrete. IMO, at > least:"... MAY be divided into..." > > 20) 5.1.Global C-SID > > 20.1 "A Global C-SID typically identifies a shortest path to a node in > the SRv6 domain.An IP route is advertised by the parent node to each > of its global C-SIDs, under the associated Locator-Block.The parent > node executes a variant of the End behavior." -> "A Global C-SID > typically identifies a shortest path to a node in the SRv6 domain.An > *IPv6 *route is advertised by the parent node to each of its global > C-SIDs, under the associated Locator-Block.The parent node SHOULD > execute a variant of the END behavior." We rephrased this text in revision -09 as follows. | A global C-SID identifies a segment defined at the Locator-Block | level. The tuple (Locator-Block, C-SID) identifies the same segment | across all nodes of the SR domain. A typical example is a prefix | segment bound to the End behavior. > 20.2 "A node can have multiple global C-SIDs under the same > Locator-Block (e.g., one per IGP flexible algorithm).Multiple nodes > may share the same global C-SID (anycast)."-> " A node MAY have > multiple global C-SIDs under the same Locator-Block (e.g., one per > IGP flexible algorithm).Multiple nodes MAY share the same global > C-SID (anycast)." We rephrased this text in revision -09 as follows. | A node can have multiple global C-SIDs under the same Locator-Block | (e.g., one per IGP flexible algorithm ([RFC9350])). Multiple nodes | may share the same global C-SID (e.g., anycast). Note that the words "can" and "may" in this paragraph are merely a statement of fact, not normative text. > 21) 5.2. Local C-SID > > 21.1 "No IP route is advertised by a parent node for its local C-SIDs." > > -> "No IPv6 route is advertised by a parent node for its local C-SIDs." We rephrased this text in revision -09 as follows. | A local C-SID identifies a segment defined at the node level and | within the scope of a particular Locator-Block. The tuple (Locator- | Block, C-SID) identifies a different segment on each node of the SR | domain. A typical example is a non-routed Adjacency segment bound to | the End.X behavior. > 22) 7.2.Segment List Compression > > 22.1 I would add some explanation diagram about compression here. > > 23) 8.Inter Routing Domains with the End.XPS behavior > > 23.1 It is great to have such solution here ! Thanks! > *END of review.*
_______________________________________________ spring mailing list spring@ietf.org https://www.ietf.org/mailman/listinfo/spring
