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

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

Hi David,

This works for us.  Thanks for keeping at it with us.

When I run the nits tool, I get the following...

Miscellaneous warnings:
  ----------------------------------------------------------------------------

  -- The exact meaning of the all-uppercase expression 'NOT REQUIRED' is not
     defined in RFC 2119.  If it is intended as a requirements expression, it
     should be rewritten using one of the combinations defined in RFC 2119;
     otherwise it should not be all-uppercase.

Should I just keep it all lower case?

Thanks,
Bob

> -----Original Message-----
> From: Black, David [mailto:[e-mail]]
> Sent: Saturday, September 14, 2013 12:14 AM
> To: Sharp, Robert O; [e-mail]
> Subject: RE: David Black's review of draft-ietf-storm-rdmap-ext-04
> 
> Hi Bob,
> 
> Ok, we're down to just topic [1] - thank you for picking up the other changes:
> 
> > > [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.
> > >
> > The specifics of the implementation (platform and/or adapter) as well
> > as anything we would need to cover in a verbs document do not seem in
> > scope.  We agree with what you expect and RNIC to at least attempt
> > provide, but do not believe that it should be discussed in this spec.
> 
> I'm not looking for a "discussion" - a single sentence saying that some RNIC
> implementations may provide atomicity behavior wrt local accesses
> performed via the RNIC would suffice.  See suggested text below that picks
> up this point in the second NEW sentence.
> 
> >
> > > 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.
> >
> > We can work to remove the "outside the scope" statement, but the new
> > text above implies that and RNIC will not provide wider guarantees and
> > that is not what we want to say here.  It is likely that
> > implementations will do more than the wire protocol can guarantee.  If we
> change the "do not" to a "may not"
> > this works for me.
> 
> How about this text:
> 
> NEW
>       The Requester of an atomic operation cannot rely on atomic operation
>         behavior at the Responder across multiple RNICs or with respect to other
>         applications/ULPs running at the Responder that can access the ULP
> Buffer.
>         Some RNIC implementations may provide such atomic behavior, but it is
>         NOT REQUIRED by the atomic operations specified in this document.
> 
> What I've tried to do is express the lack of guarantee as "cannot rely on"
> in the first sentence punctuated by the clear "NOT REQUIRED" statement
> about what RNIC implementations could do in the second sentence.
> 
> Thanks,
> --David
> 
> > -----Original Message-----
> > From: Sharp, Robert O [mailto:[e-mail]]
> > Sent: Wednesday, September 11, 2013 5:09 PM
> > To: Black, David; [e-mail]
> > Subject: RE: David Black's review of draft-ietf-storm-rdmap-ext-04
> >
> > Hi David,
> >
> > Thanks again for the review and comments.  Responses below...
> >
> > Thanks,
> > Bob
> >
> > > -----Original Message-----
> > > From: Black, David [mailto:[e-mail]]
> > > Sent: Friday, September 06, 2013 6:14 PM
> > > To: Sharp, Robert O; [e-mail]
> > > Subject: RE: David Black's review of draft-ietf-storm-rdmap-ext-04
> > >
> > > 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.
> > >
> > The specifics of the implementation (platform and/or adapter) as well
> > as anything we would need to cover in a verbs document do not seem in
> > scope.  We agree with what you expect and RNIC to at least attempt
> > provide, but do not believe that it should be discussed in this spec.
> >
> > > 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.
> >
> > We can work to remove the "outside the scope" statement, but the new
> > text above implies that and RNIC will not provide wider guarantees and
> > that is not what we want to say here.  It is likely that
> > implementations will do more than the wire protocol can guarantee.  If we
> change the "do not" to a "may not"
> > this works for me.
> >
> > >
> > > 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.
> >
> > Added a paragraph
> >
> > >
> > > [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.
> >
> > This is better.  Thanks!
> >
> > >
> > > [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.
> >
> > This is a nice clarification.  Thanks.
> >
> > >
> > > [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.
> >
> > Fixed.  Thanks.
> >
> > >
> > > [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.
> >
> > Fixed.  Thanks!
> >
> > >
> > > [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
> >