# IESG review of draft-ietf-bmwg-sr-bench-meth-08

CC @MikeBishop

## Comments

### Section 3, paragraph 18
```
     *  Compression method used: None, NEXT-C-SID, REPLACE-C-SID and
        compressed SID size.
```
These aren't defined in this document; they're from RFC9800, as "NEXT-CSID" and
"REPLACE-CSID" (note the hyphenation difference). But RFC9800 is only an
Informative reference in this document and not even mentioned here. Does it need
to be Normative?

### Section 4.5, paragraph 2
```
     (Timestamp B).  The system recovery time is determined by subtracting
     Timestamp B from Timestamp A.  The test MUST be repeated several
```
Which is subtracted from which depends whether you're dealing with
time-since-epoch or time-before-now, and that's implementation-internal. I'd
sidestep the question and say "The system recovery time is the difference
between the two Timestamps."

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Grammar/style

#### Section 2.1, paragraph 7
```
oint behavior (H.Encaps, End, End.X, etc), the latter may have a few differen
                                     ^^^
```
In American English, abbreviations like "etc." require a period.

#### Section 4.6, paragraph 1
```
ith composite SR Policy testing as sub-case, if supported). Create Z SR Polic
                                   ^^^^^^^^
```
This word is normally spelled as one.

#### Section 4.6, paragraph 6
```
d execution is crucial to allow to reproduce and repeat the tests by the dif
                                ^^^^^^^^^^^^
```
Did you mean "reproducing"? Or maybe you should add a pronoun? In active voice,
"allow" + "to" takes an object, usually a pronoun.

#### Section 9.2, paragraph 5
```
each address in the list is a SID. Hence SRv6 Segment list typically contain
                                   ^^^^^
```
A comma may be missing after the conjunctive/linking adverb "Hence".

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF]. You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool

Thank you to Roni Even for the GENART review.

** Section 4.5
  At Timestamp A reduce the frame rate
   to 50% of the above rate and record the time of the last frame lost
   (Timestamp B).  The system recovery time is determined by subtracting
   Timestamp B from Timestamp A.  

Is there a typo in expressing this calculation?  As written:

Recover-time = A – B (i.e., subtracting B from A)

Shouldn’t it read “subtracting A from B”:

Recover-time = B – A

** Section 2.7
   The test time could be much shorter than recommended in [RFC9004]
   because typical SR DUT is hardware-based with claimed buffers between
   30ms to 100ms.  It is better to consult with the vendor to find a
   good starting search point. 

I’m assuming that this uncited “30ms – 100ms” is derived from current performance claims for hardware available today.  Will this age well?

** Section 4.7.  Multiple steps in this section refer to ECMP/wECMP:

-- Step 3, “The result is to find out the maximum supported SLs number and ECMP/wECMP works fine …”

-- Step 4, “Verify that traffic flows, with no drops and correct ECMP/wECMP.  The result is that the Active CP is working correctly with any SLs, 
ECMP/wECMP works as expected, …”

-- Step 5, “Create Z SR Policies, …  correct ECMP/wECMP”

Where is the correct, baseline behavior of ECMP/wECMP documented?