[storm] AD Review of draft-ietf-storm-mpa-peer-connect

["David Harrington" <[e-mail]>]

AD Review of draft-ietf-storm-mpa-peer-connect

I have reviewed this document and have comments. I think this work is
important, but the way this document explains it could be better.

My goal as AD is to have this document go through IETF Last Call and
IESG review without any issues delaying advancement.

Some comments are technical/process comments that refer to text that
will likely cause delays during IESG Evaluation.

Some are stylistic, such as capitalization and wording consistency,
that are distracting and are likely to make readers wonder "is there
something special here that I need to understand?" and to waste time
trying to determine if there is or is not something special that led
to your capitalizing the text, or using different wording than
elsewhere. These could lead to IESG members raising unnecessary
questions that would delay advancement. 

Others are editorial in nature, mostly about writing clear and
unambiguous text. Generally, if it is simply a small editorial thing,
I ignored it, expecting the RFC Editor to clean up the text a bit. But
sometimes, the lack of clarity could lead to differing interpretations
of the text, or lack of understanding of the text, and those should be
fixed before I bring this to the IESG.

In some cases, I have asked a question. If I had to ask a question,
then the document apparently didn't explain it clearly enough. The
answer to the question usually needs to be explained "in the
document", not just in a private email to me.

1) idnits complains about unreachable reference:
http://www.rdmaconsortium.org/home/draft-hilland-iwarp-verbs-v1.0-RDMA
C.pdf

2) abstracts cannot contain references.

3) in 4.3.2, the conclusion could be stated more clearly, such as "If
a nop approach was used, it would require lower layers to know the
usage model, and would disrupt the calculations ..."

4) in 4.3.3, I have no idea what this text is saying or why it is
relevant to the problem at hand. I don't know what an untagged message
is; I don't know why a WC can only be generated by an untagged
message. Can you expand on this so readers that are not RDMA experts
understand what you're talking about? The IESG will need to review and
approve this document, and to my knowledge none of them are RDMA
experts. 

5) It would be helpful to explain why taking the bit from RES is
backwards compatible.
Explain why older implementation will not have a problem with the new
bit, and why new implemntations will be able to benefit from the new
bit.

6) Rev "MUST be set to two"; would this be better as "two or higher?"
to allow for a version three+ that would be backward compatible with
the enhanced features of version two?

7) I don't understand "The Enhanced Reject function code SHOULD be
used to indicate a configuration that would have been accepted." Would
have been accepted except was not because of what condition? Why is
this only a SHOULD and not a MUST?
 8) How can the private data be zero length if it must begin with
Enhanced RDMA Connection establishment data?
9) sometimes "Enhanced RDMA Connection Establishment Data" is
capitalized, sometimes not. Be consistent.
10) Received extended ... message SHOULD be reported to the ULP. Why
is this not a MUST? Anytime there is a SHOULD, the text should
identify when the rule is not mandatory - what are the valid
exceptions that make this not a MUST?
11) What is "enhanced DDP stream management"? 
12) "It should be noted that" - you're noting it, so this lead-in
phrase is totally unnecessary. If you leave it as "It should be noted
that", then explain WHY it should be noted. If "it is especially
important to recognize that" then say that, and explain WHY it is
important to recognize. 
It appears that it is important only because you've made a case for
peer-to-peer needing to initiate from either side, and the SCTP
adaption layer allows for that already. So you should probably explain
why you're duplicating that capability.
13) additional legal sequences - but then you don't seem to define any
**sequences**. Is "Legal Sequences" a special term? If so, please
explain where it is defined. if not, why is it capitalized?
14) I cannot parse "The protocol layers on which RDMA connection
establishment is layered upon [RFC5040] and [RFC5041] define layers
and error types."
15) if 5043 and 5044 do not define error codes, then how are the error
codes used? are they carried in messages? which messages?
16) In looking for the answer to 15, I found that error codes ARE
defined in RFC5044 section 8. So your text is apparently wrong. 
17) in section 9, the responder SHOULD NOT set its IRD higher than
Initiator ORD. Why is this not a MUST? Under what conditions is it
acceptable to set this higher than Initiator ORD? What are the
implications of doing so?
the responder SHOULD set its ORD lower. What are the valid exceptions
to this?
18) the Initiator MUST set its IRD to a value at least as large as the
responder ORD if it has sufficient resources 
"MUST if" means this is a SHOULD, and lack of resources is the
exception. If this is truly a MUST (which the following sentences seem
to say, requiring a termination if ti doesn't do so, then I would
remove the if clause from the first sentence, making it a definite
MUST.
19) "Control flag for using" doesn't discuss whatthe values are. Does
this mean that if A has a value 1, then FULPDU has a zero length?
This section might be better organized by defining what the fields are
(control flags and depths), then describing how the values are set.
You do that for A,B,C but describe IRD and ORD in-line. 
20) In section 9, "If there is no Standard RDMAP Configuration Data in
the MPA Reply Frame, and the Enhanced Connection Establishment version
number is used, it is the equivalent of setting 'A', 'B' and 'C'. "
Why do we need this extra complexity? Why not require that these be
set ONE way, not two? I also have concerns about "THE enhanced
connectiosn establshment version number"; will this document need to
be updated if a version three is ever developed?
21) "Setting no Control Flags in the MPA Reply indicates that an RDMA
Send message will be required. As this option will require the
initiator ULP to be involved it SHOULD NOT be used unless necessary. "
Please define what necessary is. I think this would be better as a
MUST NOT. 
22) "Initiator or Responder SHOULD generate the TERM message" - why is
thi snot a MUST? What are the valid exceptions to this?
in section 10:
23) "An initiator SHOULD NOT use the Enhanced DDP Connection
Establishment formats or function codes when no enhanced functionality
is desired. " what are the valid exceptions that make this a SHOULD
rather than a MUST?
24) the paragraph containing "If a responder does not support received
extended MPA message, then it MUST close the TCP connection and exit
MPA since MPA frame is improperly formatted for it as stated in
[RFC5044]." needs a bit of English cleanup.
25) section11 This document defines error codes; so does RFC5044.
Implementers and operators will need to hunt through documents to find
the various error codes. This document apparently overlooks the error
codes defined in 5044 section 8. Should there be a registry,
maintained by IANA, for the MPA error codes? 
section 12:
26) I am always concerned when a security coinsiderations section says
this document does not introduce any new security considerations. This
is also a red flag during IESG Evaluation. I suggest you document WHY
these changes do not impact security compared to 5043 and 5044.
Demonstrate that the WG actually **considered** the security issues.

27) expand acronyms on first use (MPA, RDMA, RDDP, ULP, iWARP, etc.)

28) references for sctp, infiniband, 

29) in 4.3.1, "required only for one transport" - mention the one
transport?

30) an explanation/definition of MPA Fencing would help readers. Maybe
a terminology section would be helpful, given all the acronyms, etc.

31) The information in 4.4 would be helful in the Introduction
section. This would help to organize the document into "Here's how it
works now; here is why the client/server model is undesirable in a P2P
environment; and this is how to modify the existing standard to
address this problem."

32) Is section 5 a description of how things currently work, or the
way this document proposes they work? Section 5 mentions "Enhanced
Connection Setup"; is that the same as "Enhanced RDMA Connection
Establishment"? if so, then naming section 5 "Enhanced RDMA Connection
Establishment" or "Enhanced MPA Connection Setup" might be helful. Use
terminology consistently, please.

I hope these suggestions help improve the document so we can advance
it through the approval process quickly,









David Harrington
Director, IETF Transport Area
[e-mail] (preferred for ietf)
[e-mail]
[phone] (cell)