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

"Black, David" <david.black@emc.com> Mon, 16 September 2013 22:02 UTC

Return-Path: <david.black@emc.com>
X-Original-To: storm@ietfa.amsl.com
Delivered-To: storm@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id BEA0211E81D0 for <storm@ietfa.amsl.com>; Mon, 16 Sep 2013 15:02:57 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -101.999
X-Spam-Level:
X-Spam-Status: No, score=-101.999 tagged_above=-999 required=5 tests=[BAYES_00=-2.599, J_CHICKENPOX_92=0.6, USER_IN_WHITELIST=-100]
Received: from mail.ietf.org ([12.22.58.30]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id TMz2D5op6sLN for <storm@ietfa.amsl.com>; Mon, 16 Sep 2013 15:02:54 -0700 (PDT)
Received: from mailuogwhop.emc.com (mailuogwhop.emc.com [168.159.213.141]) by ietfa.amsl.com (Postfix) with ESMTP id A79DF11E81C6 for <storm@ietf.org>; Mon, 16 Sep 2013 15:02:53 -0700 (PDT)
Received: from maildlpprd05.lss.emc.com (maildlpprd05.lss.emc.com [10.253.24.37]) by mailuogwprd02.lss.emc.com (Sentrion-MTA-4.3.0/Sentrion-MTA-4.3.0) with ESMTP id r8GM2qGl013111 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 16 Sep 2013 18:02:52 -0400
X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd02.lss.emc.com r8GM2qGl013111
DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=emc.com; s=jan2013; t=1379368972; bh=7uHtTWQINMLy+Rixg3RoJ4Rmgws=; h=From:To:Date:Subject:Message-ID:References:In-Reply-To: Content-Type:Content-Transfer-Encoding:MIME-Version; b=Hs3dkyFu4+OB3lOC4jVigfp5dstW5FI5ZjvQv3xGUojhmB9htDcP2sfF9ZpxVRlEz rPf3PjTxVFlpAmyINTo5iSdGbo7sh4gg5IZGZmDvYfDa8/BEn6hOP140gF/riEzfiJ qzOXTlvQJDpyBFsR1d0KI363ivP1CLPNev6HgDeY=
X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd02.lss.emc.com r8GM2qGl013111
Received: from mailusrhubprd53.lss.emc.com (mailusrhubprd53.lss.emc.com [10.106.48.18]) by maildlpprd05.lss.emc.com (RSA Interceptor); Mon, 16 Sep 2013 15:02:45 -0700
Received: from mxhub04.corp.emc.com (mxhub04.corp.emc.com [10.254.141.106]) by mailusrhubprd53.lss.emc.com (Sentrion-MTA-4.3.0/Sentrion-MTA-4.3.0) with ESMTP id r8GM2jo5030082 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=FAIL); Mon, 16 Sep 2013 18:02:45 -0400
Received: from mx15a.corp.emc.com ([169.254.1.46]) by mxhub04.corp.emc.com ([10.254.141.106]) with mapi; Mon, 16 Sep 2013 18:02:45 -0400
From: "Black, David" <david.black@emc.com>
To: "Sharp, Robert O" <robert.o.sharp@intel.com>, "storm@ietf.org" <storm@ietf.org>
Date: Mon, 16 Sep 2013 18:02:43 -0400
Thread-Topic: David Black's review of draft-ietf-storm-rdmap-ext-04
Thread-Index: Ac555JDwK6QsDppJRjas//rbxrYADQwn8w8QADLAiRAA+MIQMABvonpgAIsbdWAAArWL4A==
Message-ID: <8D3D17ACE214DC429325B2B98F3AE712025DA1C026@MX15A.corp.emc.com>
References: <8D3D17ACE214DC429325B2B98F3AE712983F2ABC@MX15A.corp.emc.com> <2ABFA3E36CBB794685BFBA191CC1964952B1D820@FMSMSX105.amr.corp.intel.com> <8D3D17ACE214DC429325B2B98F3AE712025D76270F@MX15A.corp.emc.com> <2ABFA3E36CBB794685BFBA191CC1964952B2D3F5@FMSMSX105.amr.corp.intel.com> <8D3D17ACE214DC429325B2B98F3AE712025DA1BD34@MX15A.corp.emc.com> <2ABFA3E36CBB794685BFBA191CC1964952B3A70A@FMSMSX105.amr.corp.intel.com>
In-Reply-To: <2ABFA3E36CBB794685BFBA191CC1964952B3A70A@FMSMSX105.amr.corp.intel.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
acceptlanguage: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-Sentrion-Hostname: mailusrhubprd53.lss.emc.com
X-EMM-GWVC: 1
X-RSA-Classifications: DLM_1, public
X-EMM-McAfeeVC: 1
Subject: Re: [storm] David Black's review of draft-ietf-storm-rdmap-ext-04
X-BeenThere: storm@ietf.org
X-Mailman-Version: 2.1.12
Precedence: list
List-Id: Storage Maintenance WG <storm.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/storm>, <mailto:storm-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/storm>
List-Post: <mailto:storm@ietf.org>
List-Help: <mailto:storm-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/storm>, <mailto:storm-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 16 Sep 2013 22:02:57 -0000

idnits is right about this one, mea culpa ...

        Some RNIC implementations may provide such atomic behavior, but it is
      NOT REQUIRED by the atomic operations specified in this document.

"NOT REQUIRED by" -> "OPTIONAL for" should work nicely.

Thanks,
--David


> -----Original Message-----
> From: Sharp, Robert O [mailto:robert.o.sharp@intel.com]
> Sent: Monday, September 16, 2013 4:45 PM
> To: Black, David; storm@ietf.org
> Subject: RE: David Black's review of draft-ietf-storm-rdmap-ext-04
>
> 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:david.black@emc.com]
> > Sent: Saturday, September 14, 2013 12:14 AM
> > To: Sharp, Robert O; storm@ietf.org
> > 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:robert.o.sharp@intel.com]
> > > Sent: Wednesday, September 11, 2013 5:09 PM
> > > To: Black, David; storm@ietf.org
> > > 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:david.black@emc.com]
> > > > Sent: Friday, September 06, 2013 6:14 PM
> > > > To: Sharp, Robert O; storm@ietf.org
> > > > 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:robert.o.sharp@intel.com]
> > > > > Sent: Thursday, September 05, 2013 6:20 PM
> > > > > To: Black, David; storm@ietf.org
> > > > > 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: storm-bounces@ietf.org [mailto:storm-bounces@ietf.org] On
> > > > > > Behalf Of Black, David
> > > > > > Sent: Friday, July 05, 2013 8:03 PM
> > > > > > To: storm@ietf.org
> > > > > > 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
> > > > > > +1 (508) 293-7953             FAX: +1 (508) 293-7786
> > > > > > david.black@emc.com        Mobile: +1 (978) 394-7754
> > > > > > ----------------------------------------------------
> > > > > >
> > > > > >
> > > > > > _______________________________________________
> > > > > > storm mailing list
> > > > > > storm@ietf.org
> > > > > > https://www.ietf.org/mailman/listinfo/storm
> > >
>