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

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

Hi Robert,

The new -05 draft is a definite improvement.  I have a few comments to add -
anything that I haven't commented on here is fine (at least for me).

[1] Atomics and software running on local host (3, although I agree with Tom's
suggestion to move the text about what's not covered, including whatever
is added/done to respond to this comment).

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

I'm surprised by the latter (host software not using the RNIC).  How does such
host software that wants to coordinate w/remote atomic operations do so?

Obviously, the main memory subsystem won't do that, but do RNIC implementations
provide a way to ask the RNIC to do something to local memory that behaves
atomically wrt remote atomic ops (obviously that would be slower than just
accessing the memory from the/a main CPU)?  If so, a sentence pointing out
that this may be available (possibly depending on the RNIC) would be helpful.

Also the following change would help:

OLD
        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.
NEW
        Atomic operations do not provide atomicity guarantees   across multiple
      RNICs or with other applications/ULPs running on the Responder that
      can access the ULP Buffer.

My primary reason for this change is to remove the "outside the
scope of this specification" language.  That's not typical IETF usage,
and is likely to attract unwanted/unwarranted reviewer attention.  It's
better to state what isn't happening.

[2] Discovery (1.1)

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

Even if the authors don't want to recommend this approach (try an atomic No-Op;
either it works or generates an Unexpected OpCode error), please add a sentence
pointing out that it is possible.

[3] Add Mask description (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:

I see (says the blind man) - I agree that the example is too long to add, but
it did help me figure out what appears to be missing.  My confusion was caused
by the following sentence:

   A bit set in the Add Mask field specifies the field boundary.

This longer sentence would have avoided my confusion:

   A bit set in the Add Mask field specifies the field boundary; for each
   field, a bit is set at the most significant bit position for each field,
   causing any carry out of that bit position to be discarded when the
   addition is performed.

Please use the longer sentence.

[3] FetchAdd pseudo code (5.1.1)

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

Ok, but that seems too clever; the following would be clearer ('>>' denotes a
right logical shift):

        carry = (sum & 2) >> 1

For the other two instances, the loop index provides the shift quantity, e.g.,

OLD
      val1 = !(!(Original Remote Data Value & bit_location))
NEW
      val1 = (Original Remote Data Value & bit_location) >> bit

I would prefer that shift operations be used instead of doubled complement
operations.

[4] Swap editorial nit (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.

Change the sentence before the itemized list to the new sentence above
that starts with "If a" and make the same change to the CmpSwap text in the
next section.

[5] Request Identifier for Atomic operations (5.2.1)

Editorial change:

OLD
         The Request Identifier specifies a number that is used to
         identify Atomic Operation Request Message. The value used in
         this field is implementation dependent and outside the scope
         of this specification. The value of this field is reflected
         back to the Local Peer in the Atomic Operation Response
         message.
NEW
         The Request Identifier specifies a number that is used to
         identify Atomic Operation Request Message. The value used in
         this field is selected by the RNIC that sends this message,
         and is reflected back to the Local Peer in the Atomic
         Operation Response message.

My primary reason this change is to remove the "outside the
scope of this specification" language.  That's not typical IETF usage,
and is likely to attract unwanted/unwarranted reviewer attention.

[6] IANA Considerations.

I'll almost certainly find some more text changes that'll be needed
here - I'll provide those as comments during WG Last Call.  For starters,
the last paragraph in Section 10 should call IANA's attention to the
two new registries (not just one) and say that they are in Sections
10.1 and 10.2.

Thanks,
--David

> -----Original Message-----
> From: Sharp, Robert O [mailto:[e-mail]]
> Sent: Thursday, September 05, 2013 6:20 PM
> To: Black, David; [e-mail]
> Subject: RE: David Black's review of draft-ietf-storm-rdmap-ext-04
>
> 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