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

["Black, David" <[e-mail]>]

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.

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

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.

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

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.
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.

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.

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

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.

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.

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.

Section 10, IANA Considerations:

   0x9, Immediate Data with SE, [RFCXXXX]

Spell out SE as "Solicited Event"

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.

Thanks,
--David
----------------------------------------------------
David L. Black, Distinguished Engineer
EMC Corporation, 176 South St., Hopkinton, MA  01748
[phone]             FAX: [phone]
[e-mail]        Mobile: [phone]
----------------------------------------------------