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

["Sharp, Robert O" <[e-mail]>]

Hi Tom,

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

Thanks,
Bob

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]] On Behalf
> Of Tom Talpey
> Sent: Friday, September 06, 2013 2:07 PM
> To: [e-mail]
> Subject: [storm] Review comments on updated draft-ietf-storm-rdmap-ext-
> 05
> 
> 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.
> 

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"?
> 
>      " An Immediate Data operation that is not preceded by an RDMA
>        Write operation causes an RDMA 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.

> 
> 
> 5) Nits:
> 
> 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?

> 
> 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.
> 
 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.
> 
> The document has a small number of id-nits which should be corrected.

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