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

[arkady kanevsky <[e-mail]>]

David,
version 5 of the draft is posted that responds to your comments.
Again, thank you very much for detailed comments and suggestions.
Arkady

On Fri, May 20, 2011 at 11:06 AM, David Harrington <[e-mail]>wrote:

> 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)
>
> _______________________________________________
> storm mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/storm
>



-- 
Cheers,
Arkady Kanevsky