Re: [storm] David Black's review of draft-ietf-storm-rdmap-ext-04

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

Hi David,

Thanks for the comments!  The author's responses are embedded below...

Thanks,
Bob

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]] On Behalf
> Of Black, David
> Sent: Friday, July 05, 2013 8:03 PM
> To: [e-mail]
> Subject: [storm] David Black's review of draft-ietf-storm-rdmap-ext-04
> 
> As I previously indicated, the authors should revise what they can reasonably
> edit this coming week and submit a -05 version of this draft by the midnight
> UTC cutoff on Monday, July 15 [WARNING - that's midnight Greenwich (Zulu)
> Time, not any US Time and not British Summer Time!].
> 
> I'm happy to carry any of the comments below over to become initial WG
> Last Call comments on that -05 version due to the severe delays in dealing
> with this draft.
> 
> Surprising omission - the term "atomic" is never defined (!).  No, the
> definition is not obvious ... a crucial question (which could be answered by a
> definition or in Section 5.3) is: "Atomic with respect to what?".
> There are atomic operations defined in other contexts that are atomic only
> with respect to other atomic operations (and not atomic wrt other non-
> atomic operations), and I think considerably more is intended here.
> Section 7 does not appear to be sufficient, as it describes behavior from the
> viewpoint of a single operation issuer, which does not include behavior with
> respect to operations that may have been issued from elsewhere, including
> local memory access by the Remote Peer.

[Authors] Significant text has been added to describe atomic behavior. The wire protocol only covers atomicity between atomic operations on streams in the context of a single RNIC.  It does not cover other RNIC behaviors that cover multiple RNICs or between host software not using the RNIC.

> 
> Section 4 - say how large the Atomic Request and Atomic Response headers
> are.

[Authors] Addressed in draft

> 
> Section 4.1 - I think Figure 1 actually contains 3 fields - the two Control Fields
> and the Invalidate STag field (see Figure 4 in RFC 5040).  It would be helpful
> to add some text below the figure to indicate which bytes are which field
> (this is also a problem in RFC 5040).  Appendix A helps, but the field
> delineations really need to be here.

[Authors] Text added 

> 
> Throughout section 5 - these operations are specified in terms of
> "destination address".  Would it be better to talk about "buffer offset"?

[Authors] Good comment.  Switched to ULP Buffer address terminology to be consistent with the other drafts.

> 
> Section 5:
> 
>    The discovery of whether the Atomic
>    Operations are implemented or not is outside the scope of this
>    specification and it should be handled by the ULPs or applications.
> 
> That's just asking for an IESG Discuss position :-).  At least say something
> about how discovery could work.  Moreover, what's wrong with just trying an
> atomic operation?  If the operation fails, the resulting error will be:
> 
>    0x0/0x2/0x06, Remote Operation Error / Unexpected OpCode
> 
> It looks like a FetchAdd with Add Data=0 is a no-op that could safely be used
> for this purpose, providing a simple robust discovery mechanism.

[Authors] Added text describing current discovery mechanisms and how this new capability fits in.

> As part of this, I would add a requirement that if any of the atomic operations
> are supported, then all of them MUST be supported, so that the FetchAdd
> no-op can be used to discover all of them.

[Authors] All MUST be supported has been added.

> 
> Section 5.1.1:
> 
> The "multiple fields of selectable length" text in the Add Mask description is
> not clear - I see how this works, but an example that uses an Add Mask to
> designate multiple fields would help.

[Authors] Since there are no other operation examples in the document, we are not sure if we want to add one for this.
However, here is an example:

Remote Data Value: 0x000000000000CDEF
Add Data: 0x000000000000789A
Add Mask: 0x0000000000000808 == b'0..0100000001000'

In this example, the Add Mask specifies that there are three independent fields to be added.  That is, the carry is not propagated from one field to the next.
The fields are:
	Bits 0 - 3
	Bits 4 - 11
	Bits 12 - 63

The results of adding the Remote Data Value and the Add Data for each field are:
	0xF + 0xA = 0X9		(carry is dropped)
	0xDE + 0x89 = 0x67	(carry is dropped)
	0xC + 0x7 = 0x13
The result of the operation is 0x0000000000013679.

> 
> the FetchAdd pseudo code contains a lot of doubled complement
> operations, e.g., carry = !(!(sum & 2)) - why is that not carry = sum & 2 ??

[Authors]  The double complement gives a result of 0 or 1 depending on whether the value is zero or non-zero.  The subsequent calculations rely on this.
It is the equivalent of:
	If (sum & 2)
		carry = 1
	else
		carry = 0

> 
> Section 5.1.2
> 
> This sentence in the last paragraph is problematic standing by itself:
> 
>    Additionally an error MUST be surfaced and a terminate
>    message MUST be generated.
> 
> I think that needs to be combined with the previous sentence to introduce a
> bullet list of what has to happen when a non-64-bit-aligned address is used:
> 
> 	If a Swap Atomic Operation is attempted on a target buffer address
> 	that is not 64-bit aligned, then:
> 
> 	o The operation MUST NOT be performed,
> 	o The Responder's memory MUST NOT be modified,
> 	o The result MUST be surfaced as an error, and
> 	o A terminate message MUST be generated.
> 
> In addition, it's important to say which error is generated.  Section 8.2
> indicates 0x0/0x2/0x07, Remote Operation Error / Catastrophic error,
> localized to RDMAP Stream - say that here also.

[Authors] Section reworked as suggest.  The specific error code is referenced instead of repeated.

> 
> Section 5.1.3
> 
> Same comments as last paragraph in Section 5.1.2
> 
> This section uses "Original Remote Data Value".  Sections 5.1.1 and 5.1.2 use
> "Original Remote Data".  All three should use the same term, and based on
> Section 5.2.2, "Original Remote Data Value" appears to be correct.

[Authors] Corrected

> 
> Section 5.2.1
> 
>      Request Identifier: 32 bits.
> 
>          The Request Identifier specifies a number that is used to
>          identify Atomic Operation Request Message. The use of this
>          field is implementation dependent and outside the scope of
>          this specification.
> 
> That's not right.  This identifier is returned in the response and used by the
> Requestor to associate the response to the request.  The Requestor can
> choose any value it likes that enables it to do that, and the details of how
> those values are generated is outside the scope of this specification, but
> their use is definitely in scope.

[Authors] Good catch.  Corrected.

> 
> Section 10, IANA Considerations:
> 
>    0x9, Immediate Data with SE, [RFCXXXX]
> 
> Spell out SE as "Solicited Event"

[Authors] Corrected.

> 
> Section 10.1
> 
> This one's my fault, partly.  The specification of the new registry is based on a
> version of the rddp registries draft that was subsequently changed (e.g., the
> Assignment Policy paragraph needs to be removed.
> 
> See the -02 version of the rddp registries draft for text to use as a model:
> 
> http://tools.ietf.org/id/draft-ietf-storm-rddp-registries-02.txt
> 
> I'd suggest working from Section 3.5 in that -02 version.

[Authors] Corrected.we think.  Also added a Queue Number Registry.

> 
> Thanks,
> --David
> ----------------------------------------------------
> David L. Black, Distinguished Engineer
> EMC Corporation, 176 South St., Hopkinton, MA  01748
> [phone]             FAX: [phone]
> [e-mail]        Mobile: [phone]
> ----------------------------------------------------
> 
> 
> _______________________________________________
> storm mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/storm