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

[arkady kanevsky <[e-mail]>]

David,
Again thank you very much for your comments.
Enclosed, please, find the latest draft for you review before submission.
Also in-line below I included responses to your comments.
Thanks,
Arkady

On Mon, Jul 18, 2011 at 12:55 AM, David B Harrington <
[e-mail]> wrote:

> Hi,
>
> AD Review update for -06-
> comments inline
>
> dbh
>
> > -----Original Message-----
> > From: David Harrington [mailto:[e-mail]]
> > Sent: Friday, May 20, 2011 11:06 AM
> > To: [e-mail];
> > [e-mail]; [e-mail]
> > Subject: AD Review of draft-ietf-storm-mpa-peer-connect
> >
> > 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
>
> I can reach this, but idnits seems to have a problem parsing it.
>
> >
> > 2) abstracts cannot contain references.
>
> fixed.
>
> >
> > 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 ..."
>
> fixed.
>
> >
> > 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.
>
> fixed.
>
> >
> > 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.
>
> fixed. text is still a bit rough though.
>

Cleaned the text:

Res:  One bit smaller than in [RFC5044], otherwise unchanged.  In
      [RFC5044] 'Res' with 'S' is reserved for future use.  [RFC5044]
      specifies that 'RES' MUST be set to zero when sending, and MUST
      NOT be checked on reception, making use of S bit backwards
      compatible with the original MPA frame format.  When the S bit is
      set to zero, no additional private data is used for enhanced RDMA
      connection establishment, and therefore the resulting MPA request
      and reply frames are identical to the unenhanced protocol.



>
> >
> > 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?
>
> fixed.
>
> relative to a note from dblack, you changed "nop" to dummy operation.
> 'nop' is still used in 4.3
>
> Good catch.
Fixed 4.3 as

The
   natural question is why the application layer would not simply
   generate a dummy message when there was no other message to submit.



> >
> > 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?
>
> fixed.
>
> >  8) How can the private data be zero length if it must begin with
> > Enhanced RDMA Connection establishment data?
>
> fixed.
>
> > 9) sometimes "Enhanced RDMA Connection Establishment Data" is
> > capitalized, sometimes not. Be consistent.
>
> fixed.
>
> > 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?
>
> fixed.
>
> > 11) What is "enhanced DDP stream management"?
>
> I didn't see this addressed, but it might be clear enough. We'll see
> if IESG has concerns over this terminology.
>
> > 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.
>
> fixed.
>
> > 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?
>
> fixed.
>
> > 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.
>
> fixed.
>
> > 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?
>
> in -06-,
>        Responder SHOULD set its IRD at least to Initiator ORD
> "at least" seems to indicate that setting it higher than or equal to
> Initiator ORD is expected.
> But the next sentence says "Responder SHOULD NOT set its IRD higher
> than Initiator ORD."
> But the next sentence says "Responder MAY set IRD to one if Initiator
> ORD is zero if ..."
> so the logic seems unclear.
> I think this paragraph should be rewritten to be much clearer than the
> current text.
>
> It is possiblr that the text is actually a clear description of the
> way things should be processed.
> If that is true, then you might want to see if you could simplify the
> processing.
>

Section 9 is rewritten to address comments 17-21.

>
> > 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.
>
> (same as #17 - please rewrite the paragraph for clarity)
>
>
> > 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.
>
> I am not sure how you interpeted my suggestion. In -04-, you had the
> descriptions of the flags AFTER defining the fields,
> but you had the descriptions of IRD and ORD in line. Now you seem to
> have moved the descriptions for IRD and ORD out of line, but put the
> descriptions for the flags inline. I recommend that for ALL of these
> fields, you define the field, and then put the descriptions of the
> values AFTER the field definitions.
>
> -06- describes the meanings of the flags differently than -04-
> Is this consistent with WG consensus?
>
> The description says to set the flags to TRUE, but doesn't define
> TRUE.
> These flags are all 1-bit, so the choice seems to be 0 or 1.
> Is TRUE equal to 1? Then why not say 1 rather than TRUE?
> If TRUE has some special meaning for RDMA, please provide a reference
>
> "Control flag for using a zero length FULPDU as the RTR message."
> This is a little ambiguous.
> Please describe what the values mean for each flag, as you do for flag
> A.
>

Done.

>
> > 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.
>
> wow. this text has become way more complicated than the -04- version.
> Can this be simplified?
> Can you start by saying
> "If the value of A is 0, then:
>        B,C,D flags must be set to zero and ignored by the reciever.
>        processing steps for client-server mode are:
>        and then provide bullets for the processing steps for
> client-server support?
> "If the value of A is 1, then:"
>        and then provide bullets for the processing steps for
> peer-to-peer support?
>
> > 22) "Initiator or Responder SHOULD generate the TERM message" - why
> is
> > thi snot a MUST? What are the valid exceptions to this?
>
> fixed.
>
> > 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?
>
> fixed
>
> > 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.
>
> fixed.
>
> > 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?
>
> I repeat my question. Should there be a registry for the error codes
> so there is a consistent place to look for which error codes are
> defined, and their values and meanings?
>

Since RFC5044 does not have any IANA consideration and RFC5043 only uses
IANA
since it extends IANA registered SCTP  PPIDs we do not feel that IANA
registry is needed.

>
>
> > 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.
>
> well, this wasn't handled, but the refernce to 5044 secCons might be
> enough. We'll see if IESG has any comments.
>
> >
> > 27) expand acronyms on first use (MPA, RDMA, RDDP, ULP, iWARP, etc.)
>
> fixed, although not always on first usage.
> A large section was added to the Introduction that uses the words
> before what used to be the first use.
> I can live with this, and the RFC Editor can fix if necessary.
>
> >
> > 28) references for sctp, infiniband,
>
> fixed.
>
> >
> > 29) in 4.3.1, "required only for one transport" - mention the one
> > transport?
>
> fixed.
>
> >
> > 30) an explanation/definition of MPA Fencing would help readers.
> Maybe
> > a terminology section would be helpful, given all the acronyms, etc.
>
> fixed.
>
> >
> > 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."
>
> fixed.
> >
> > 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.
>
> fixed
> >
> > 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