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

From: "Sharp, Robert O" <[email-masked]>
Date: Thu, 5 Sep 2013 22:20:11 +0000

Hi David,

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

Thanks,
Bob

> -----Original Message-----
> From: [email-masked] [mailto:[email-masked]] On Behalf
> Of Black, David
> Sent: Friday, July 05, 2013 8:03 PM
> To: [email-masked]
> 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
> [email-masked]        Mobile: +1 (978) 394-7754
> ----------------------------------------------------
> 
> 
> _______________________________________________
> storm mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/storm

---

From: "Black, David" <[email-masked]>
Date: Fri, 6 Sep 2013 19:14:14 -0400

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

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)

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

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

> [Authors] Section reworked as suggest.  The specific error code is referenced

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

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

---

From: "Sharp, Robert O" <[email-masked]>
Date: Wed, 11 Sep 2013 21:09:01 +0000

Hi David,

Thanks again for the review and comments.  Responses below...

Thanks,
Bob

> From: Black, David [mailto:[email-masked]]
> Sent: Friday, September 06, 2013 6:14 PM
> To: Sharp, Robert O; [email-masked]
> [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.  

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

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

This is better.  Thanks!

> > [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
> 
>       val1 = !(!(Original Remote Data Value & bit_location)) NEW

This is a nice clarification.  Thanks.

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

Fixed.  Thanks.

> 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
> 
> > > From: [email-masked] [mailto:[email-masked]] On
> > > Behalf Of Black, David
> > > 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
> > > 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.
> >
> > > 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
> >
> > >
> > > 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:
> > >
> > > It looks like a FetchAdd with Add Data=0 is a no-op that could
> > > safely be
> > used
> > > are supported, then all of them MUST be supported, so that the
> > > FetchAdd no-op can be used to discover all of them.
> >
> > > 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.
> > 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:
> > [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
> > >
> > > 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:
> > >
> > > 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.
> >
> > > 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
> > >
> > > 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.
> >
> > > 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
> > > David L. Black, Distinguished Engineer EMC Corporation, 176 South
> > > St., Hopkinton, MA  01748

---

From: "Black, David" <[email-masked]>
Date: Sat, 14 Sep 2013 01:13:46 -0400

Hi Bob,

Ok, we're down to just topic [1] - thank you for picking up the other changes:

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

> 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

> Sent: Wednesday, September 11, 2013 5:09 PM
> 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.
>
> 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.
>
> >

---

From: "Sharp, Robert O" <[email-masked]>
Date: Mon, 16 Sep 2013 20:44:44 +0000

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

> Sent: Saturday, September 14, 2013 12:14 AM
> > > [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).
> > >
> > > > 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?
> > >
> > > 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.
> 
> 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.
> 
> >
> > 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
> in the first sentence punctuated by the clear "NOT REQUIRED" statement
> about what RNIC implementations could do in the second sentence.
> 
> Thanks,
> --David
> 
> > > 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).
> > >
> > > > 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?
> > >
> > > 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 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.
> >
> > 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.
> >
> > >
> > > 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:
> > > > >
> > > 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
> >
> > >
> > > > > 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.
> > > 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:
> > >
> > > > > 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
> > > I would prefer that shift operations be used instead of doubled
> > > complement operations.
> >
> > > 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.
> >
> > >
> > > 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
> > > 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
> > > > > 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
> > > > > 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
> > > > > behavior with respect to operations that may have been issued
> > > > > from elsewhere, including local memory access by the Remote Peer.
> > > >
> > > > 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.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:
> > > > >
> > > > > 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:
> > > > >
> > > > > As part of this, I would add a requirement that if any of the
> > > > > atomic
> > > > operations
> > > > > 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.
> > > > Remote Data Value: 0x000000000000CDEF Add Data:
> 0x000000000000789A
> > > > 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:
> > > > > 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
> > > > >
> > > > > 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.
> > > >
> > > > > 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
> > > > > See the -02 version of the rddp registries draft for text to use
> > > > > as a
> > model:
> > > > >
> > > > > David L. Black, Distinguished Engineer EMC Corporation, 176
> > > > > South St., Hopkinton, MA  01748

---

From: "Black, David" <[email-masked]>
Date: Mon, 16 Sep 2013 18:02:43 -0400

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

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

Thanks,
--David

> Sent: Monday, September 16, 2013 4:45 PM