Subject: [forces] AD review of draft-ietf-forces-interfelfb

From: Alia Atlas <[email-masked]>
Date: Wed, 25 May 2016 18:03:29 -0400

As is customary - particularly with an AD sponsored draft, I have done a
final review before requesting that this document proceed to IETF Last Call.

First, I would like to thank Jamal and Damascene for their hard work on
this final document and really improving it.

Evangelos, thank you for shepherding this document.  Please do update the
shepherd's report.  I know it has been a couple years but many on the IESG
do read the entire shepherd's report and it needs to be up to date.

While I do have some minor comments from my most recent review, I believe
that these can all be fixed during IETF Last Call.  Please do so ASAP;
draft numbers are cheap and it is highly advantageous to have folks reading
the most recent version so they don't pick on the same issues.

With the expectation that any issues found in IETF Last Call will be dealt
with rapidly, I am placing this  on the telechat for June 16.

Minor:

a)In Sec 5.2:  "Ethertype 0xFEFE is to be used (XXX: Note to editor, likely
      we wont get that value - update when available)."   PLEASE update
this to be Ethertype TBA1 is to be used.  Then in IEEE assignment
considerations, specify that the EtherType TBA1 requires allocation &
possibly indicate that an experimental value of 0xFEFE has been used.  Is
that one of the playpen EtherTypes available?

b) In Sec 6.1.1:  Similarly, it says "If IFETYPE is absent then the standard
      Inter-FE LFB Ethernet type is used (XXX: Note to editor, to
be updated)."
Please update to TBA1 and add a reference to the IEEE Assignment
Considerations section.

c) In Sec 6.1.1: "Encapsulate each allowed Metadatum in a TLV. Use the
Metaid as
      the "type" field in the TLV header.  The TLV should be aligned to
      32 bits.  This means you may need to add padding of zeroes to
      ensure alignment."
Trivial, but please clarify that the padding of zeroes is added at the end
of the TLV.

d) In Sec 6.1.2: "If an unknown Metadatum id is encountered, or if the
metaid is not in
      the allowed filter list the implementation is expected to ignore
      it, increment the packet error statistic and proceed processing
      other Metadatum."
Can you please clarify if the "packet error statistic" is incremented once
per unknown Metadatum or metaid - or once per packet that has such a
metadatum?  The field description makes me think it is once per packet.
Knowing the excitement that exists around computing worst-case number of
counter increments per packet, I hope so.

It would also be useful to indicate which componentID is mentioned - since
this is normative.

Nit:

1) Figure 6:  the title should be "Packet format definition" and not
"Packet format suggestion".  This needs wording for a standard :-)

2) Sec 5.2: "However, at the time of publication we believe
      this is sufficient to carry all the info we need and approach
      taken would save us 4 bytes per Metadatum transferred."

Please reword as for a standard (e.g. "...need; this approach has been
selected because it saves 4 bytes per metadatum transferred."

Regards,
Alia

---

From: Jamal Hadi Salim <[email-masked]>
Date: Thu, 26 May 2016 07:39:07 -0400

Hi Alia,

> this final document and really improving it.
We wouldnt have gone this far without your dilligence to details. Thank
you.

> the most recent version so they don't pick on the same issues.
Some responses to your comments below and then we can proceed to
updating and publishing.

> that one of the playpen EtherTypes available?

OxFEFE is not one of the available playpen  EtherTypes. We ended
enforcing the user to specify it in the deployed tools (on Linux).
So people ended using different values none of which will stick. Once
this value is issued we'll make it the default. Would be an easy 1-2
line patch.

> Please update to TBA1 and add a reference to the IEEE Assignment
Will do.

> Trivial, but please clarify that the padding of zeroes is added at the end
Will do.

> counter increments per packet, I hope so.
The current deployment is _per metaid_;->
But i think it makes sense to make it per packet. It is a trivial change.
I can update the doc to make it per-packet and then patch Linux version.
Would that be fine with you?

> It would also be useful to indicate which componentID is mentioned - since
Parse error.  Which componentID?

> "Packet format suggestion".  This needs wording for a standard :-)
;-> Will fix.

> selected because it saves 4 bytes per metadatum transferred."
Will fix.

Alia, a pleasure as always - worth the wait to see your comments.

cheers,
jamal