Re: [storm] more comments on http://tools.ietf.org/html/draft-ietf-storm-rdmap-ext-00

Bernard,

Thanks for the comments! We will incorporate them in our next rev of the draft!

Below are the responses to your comments!

Hemal

-----Original Message-----
From: storm-bounces@ietf.org [mailto:storm-bounces@ietf.org] On Behalf Of Bernard Metzler
Sent: Tuesday, July 05, 2011 3:49 PM
To: storm@ietf.org
Subject: [storm] more comments on http://tools.ietf.org/html/draft-ietf-storm-rdmap-ext-00

Some comments on the RDMA Protocol Extension draft
(sorry when partially overlapping with Arkady's email
from 3 May).

The terms "Data Source" and "Data Sink" are used for Immediate
Data and Atomic Operations as well. For Atomics I found it
confusing and would prefer to have the 'Requester' and 'Responder'
terms used throughout the whole document.
[Hemal] Good comment! For Atomics, we will make sure that we use terms "Requester" and "Responder" instead of "Data Source" and "Data Sink". 

Immediate Data
The specification introduces a new RDMAP message format.
Section 1 and 6 are limiting its usage to Writes, eg.:
"Immediate Data messages allow the ULP at the sender to
provide a small amount of data following an RDMA Write payload."
Why not Sends, while SendWithImm is defined for other RDMA
transports and is part of a well established RDMA verbs
interface (OpenFabrics)?

[Hemal] Good point! The primary intent for ImmData was to use it with the RDMA Write. That being said, we can change the wording in Section 1 and 6 to allow the Immediate data's usage with any other RDMA message. But, in reality the verbs definition will not allow it to be used with Send Operations (as it will consume two RQ entries which is as good as sending two Send Messages).

While potentially out of the primary business of the
specification of a wire protocol - I assume Immediate Data
will consume a peers Receive Queue Element?[Hemal] Yes

A short discussion including appropriate RQ sizing might
be helpful for the reader (and implementer). It might go
into section 6.1.[Hemal]  This is a protocol specification. RQ sizing is outside the scope of this specification. That being said, Section 6.3 talks about ImmData consuming one Untagged Buffer. I think that text is sufficient. 

Atomics
5.1.1: I found it difficult to understand the FetchAdd decription.
In particular, the statement of
"The setting of "Add Mask" field to 0x0000000000000000 results
in Atomic Add of 64-bit Original Remote Data Value and 64-bit
"Add Data"."
was surprising to me. I would have expected all bits set to 1.
[Hemal] The "Add Mask" is used to exclude the bits from the operation rather than include the bits for the operations. The pseudo-code described in the spec reflects that.

FetchAdd sometimes extends to FetchAndAdd. I would prefer one term
only.
[Hemal] Good catch! We will use FetchAdd consistently in the doc and remove FetchAndAdd occurrences.

The abbreviation of RMW seem to refer to ReadModifyWrite.
Better to be written in full length? It could be confused
with RemoteMemoryWindow.
[Hemal] OK! We will spell out ReadModifyWrite.

Ordering and Completion Table
Section 7 provides a table of operation ordering.
For Immediate Data, I found the term "Placement" inappropriate.
Immediate Data are not placed in a target buffer. Immediate Data
are received and delivered to the ULP. Maybe exchanging "Immediate
Data is Placed at Remote Peer" with "Immediate Data is received
at Remote Peer" would clarify? I see the same issue for Atomic
Responses, which are also not Placed but received.
[Hemal] We would like to follow DDP layer term "Placement" here and be consistent with the RDMAP spec. So, we will keep the Section 7 text as it is.

The discussion of the two consecutive Atomic operations (page 22)
seem to allow re-ordering of Atomic responses. Is that true?
It might complicate response processing. 
[Hemal]  Re-ordering is allowed only from the Placement perspective. The generation of Response Messages is still done in order. So, I do not think it complicates the response processing.

General/Implementation
It might be a good idea to try an implementation of
the proposed RDMAP extension. The SoftiWARP stack might
be a good candidate for that.
[Hemal] Agree!

Thanks,
Bernard.
_______________________________________________
storm mailing list
storm@ietf.org
https://www.ietf.org/mailman/listinfo/storm