Subject: [storm] Review comments on updated draft-ietf-storm-rdmap-ext-05

From: Tom Talpey <[email-masked]>
Date: Fri, 6 Sep 2013 19:06:49 +0000

Authors, thanks for the updates to the RDMA Extensions draft based on comments received.

I have some comments on the recent set of changes, which I would like to discuss before considering this version ready for Working Group Last Call.

1) Section 1.1 "Discovery of RDMAP Extensions" was added and addresses many of my earlier concerns. However, it says the following:

     " Today there are RDMA applications and/or ULPs that are aware of the  
       existence of Atomic and Immediate data operations for RDMA  
       transports such as [IB].  Today, these applications need to be aware  
       that iWARP RNICs do not support these operations."
and
     "Negotiation of Atomic Operations typically are to determine the  
       scope of atomicity guarantees, not down to the individual Atomic  
       Operations supported.  Therefore, this specification requires all  
       Atomic Operations defined within to be supported if an RNIC supports  
       any Atomic Operations."
Subsequently, section 5 now says:
     " The Atomic Operations specified in this document provide  
       equivalent functionality to the [IB] RDMA transport, to allow  
       applications that use these primitives to work interchangeably over  
       iWARP. In addition, this document specifies a basic Swap operation."

That last sentence extends Atomics beyond those of Infiniband, and this in turn contradicts the goal stated in section 1.1 that applications should not need to be aware of differences. This inconsistency needs to be addressed.

2) While the text has been updated to specify the target of Atomic operations as a registered ULP buffer, it does not discuss the fact that this is compatible with (indeed, required by) the RFC5042 security model. This should be made explicit, and the reference made explicit.

3) In section 5.4, an "Implementation note" uses the invalid term "MAY NOT". Perhaps this means to say the behavior is "OPTIONAL"? Or is it "MUST NOT"? As a general comment, when implementation notes contain normative language, they are not actually notes,. It may be safer to make specific requirements here, and also check the other notes.

4) In section 6, it is not clear what advice this statement is making. Is it intended to be normative, saying that Immediate Data operations always do this? If so, it should move to the processing text below. Also, the term "RDMA Completion" appears three times in this paragraph, but is not defined. Do you mean simply "Completion"?

     " An Immediate Data operation that is not preceded by an RDMA  
       Write operation causes an RDMA Completion."

5) Nits:

Section 1.1 could use a paragraph break for improved readability.

In the Glossary, ULP is defined as "Upper Level Protocol". The existing 504x RFCs, and I believe common usage, use "Upper Layer Protocol". Suggest using this definition.

Also in the Glossary, this statement should be moved from the definition of Atomics, to section 5:
    " Atomicity guarantees  
       across multiple RNICs or between other applications/ULPs running on  
       the Responder with access to the ULP Buffer are outside the scope of  
       this specification.

In section 4.1, the sentence beginning "This extension defines RDMAP use Queue Number 3..." appears to be missing a word ("use of"?).

RFC5042 is not referenced in the body, but it appears as a normative reference. It should be referenced, perhaps from section 5 in the discussion of the target of Atomic operations.

The document has a small number of id-nits which should be corrected.

---

From: "Sharp, Robert O" <[email-masked]>
Date: Wed, 11 Sep 2013 21:03:04 +0000

Hi Tom,

Again, thanks for the review and comments.  Responses below...

Thanks,
Bob

> -----Original Message-----
> From: [email-masked] [mailto:[email-masked]] On Behalf
> Of Tom Talpey
> Sent: Friday, September 06, 2013 2:07 PM
> To: [email-masked]
> Subject: [storm] Review comments on updated draft-ietf-storm-rdmap-ext-
> 05
> 
> I have some comments on the recent set of changes, which I would like to
> discuss before considering this version ready for Working Group Last Call.
> 
> 
> 1) Section 1.1 "Discovery of RDMAP Extensions" was added and addresses
> many of my earlier concerns. However, it says the following:
> 
> That last sentence extends Atomics beyond those of Infiniband, and this in
> turn contradicts the goal stated in section 1.1 that applications should not
> need to be aware of differences. This inconsistency needs to be addressed.
> 

The Swap operation has been removed and a reference to OFA Verbs extensions for masked atomics has been added.

> 
> 2) While the text has been updated to specify the target of Atomic
> operations as a registered ULP buffer, it does not discuss the fact that this is
> compatible with (indeed, required by) the RFC5042 security model. This
> should be made explicit, and the reference made explicit.
> 

Added reference to RFC 5042 and the ULP Buffer address reference to the 5042 security model.

> 
> 3) In section 5.4, an "Implementation note" uses the invalid term "MAY
> NOT". Perhaps this means to say the behavior is "OPTIONAL"? Or is it "MUST
> NOT"? As a general comment, when implementation notes contain
> normative language, they are not actually notes,. It may be safer to make
> specific requirements here, and also check the other notes.
> 

The intention was to note that since this is a wire protocol, it is important to note that it is an implementation decision to provide greater scope of atomicity than just the RNIC.  Changed MAY NOT to may not.  We considered making this a positive MAY requirement, but that didn't seem like a good idea when we discussed it.

> 
> 4) In section 6, it is not clear what advice this statement is making. Is it
> intended to be normative, saying that Immediate Data operations always do
> this? If so, it should move to the processing text below. Also, the term
> "RDMA Completion" appears three times in this paragraph, but is not
> defined. Do you mean simply "Completion"?
> 

This text is descriptive and intended to describe the expected usage, but indicate that the expected usage is not absolutely required.   For example, there is no error checking if immediate data is not proceeded by an RDMA Write.   RDMA Completion is defined in RFC5040.

> Section 1.1 could use a paragraph break for improved readability.

 Fixed.

> 
> In the Glossary, ULP is defined as "Upper Level Protocol". The existing 504x
> RFCs, and I believe common usage, use "Upper Layer Protocol". Suggest
> using this definition.

Thanks.  Seems like we can just delete the definition then.  Do you agree?

>        the Responder with access to the ULP Buffer are outside the scope of
 Fixed.

> In section 4.1, the sentence beginning "This extension defines RDMAP use
> Queue Number 3..." appears to be missing a word ("use of"?).
> 

Thanks!  Fixed

> RFC5042 is not referenced in the body, but it appears as a normative
> reference. It should be referenced, perhaps from section 5 in the discussion
> of the target of Atomic operations.
> 

Long lines have been fixed.

We are not sure what to do with these at this point:

== Missing Reference: 'RFCXXXX' is mentioned on line 1266, but not
     defined
     'RFC Editor: Please replace XXXX in all instances of [RFCXXXX] abo...'
== Missing Reference: 'RFC5226' is mentioned on line 1264, but not
     defined
'Allocation Policy: Standards Action ([RFC5226])...'

> 
> _______________________________________________
> storm mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/storm