Subject: [tsvwg] SCTP SACK-I detailed comments

From: Gorry Fairhurst <[email-masked]>
Date: Wed, 13 Mar 2013 11:36:34 -0400

I've done a fairly detailed read of the above draft, and have some 
specific comments that I'd like to submit as an individual as we head 
towards WGLC.

Gorry
(as an individual)

---

Abstract
- The abstract will be read by people who do not read any other part of 
the document. I think the current abstract is terse. It doesn't say some 
things which I think it should. Missing is a statement that says this 
defines a new chunk flag, called the I-bit, that can be set by the SCTP 
stack or by an application.

Section 1
   "According to [RFC4960] the receiver of a DATA chunk should use
    delayed SACKs."
- I think it is important to say why this is normally the right thing to 
do - e.g. this remains the normal case.

   "In specific situations the delaying of SACKs results in reduced
    performance of the protocol"
- A separate subsection on examples of usage, would be really useful 
(see note on section 4.1), You could then (forward) refer to this here.

Section 4.1
   "Reasons for setting the I-bit include but are not limited to the
    following:"
- Insert commas around /but not limited/?

I think the set of examples provided in 4.1 are really important to 
publishing the spec. However, the current examples are really terse. 
Although they seem good examples, the motivation for using the I-but is 
not clear.

   "o  The application requests to set the I-bit of the last DATA chunk
       of a user message when providing the user message to the SCTP
       implementation (see Section 6)."
- What is the benefit?

   "o  The sender is in the SHUTDOWN-PENDING state."
- Is this that the immediate SACK can then expedite the shutdown of a 
stream?

   "o  The sending of a DATA chunk fills the congestion or receiver
       window."
- An immediate SACK can then allow the sender to transmit the missing 
chunk that will release the congestion/receiver window to send new data?

- I think this it is confusing that we have two lists, one in the second 
part of the introduction, and one in section 4.1. I'd suggest it may be 
useful to have one subsection in the introduction to do this.

- To me, the important aspect that should have been evident in 4.1 was 
that there are two use-cases, one when the I-bit is set by the stack as 
an internal transport function, and one as an application-triggered 
function via the API in section 6.

Section 4.2
   "On reception of an SCTP packet containing a DATA chunk with the I-bit
    set, the receiver SHOULD NOT delay the sending of the corresponding
    SACK chunk and SHOULD send it back immediately."

- This contains two RFC 2119 requirements. I think the two are the same, 
and suggest it is better to have only one. One way to resolve this would 
be to make the second /SHOULD/ read /should/.

Section 6
- It would perhaps avoid ambiguity to say at the very start that the API 
allows the I-bit to be set for any specific data chunk.

   "or the
    sinfo_flags field of the struct sctp_sndrcvinfo structure, which is
    deprecated."
- I think the "or" is confusing, since the second part is deprecated. 
Could we make this a separate statement, by replacing the "or" by a stop 
and starting a new sentence with /The/.

Section 8 (recently noted)
- Section 4.1 implies this need to discuss cost of processing the I-bit 
when in 4.1 it says: "Please note that it is
    irrelevant to the receiver why the sender has set the I-bit."

[RFC6525]
- This seems like an informative reference?

---

From: Michael Tuexen <[email-masked]>
Date: Thu, 14 Mar 2013 15:36:01 -0400

On Mar 13, 2013, at 11:36 AM, Gorry Fairhurst wrote:

> I've done a fairly detailed read of the above draft, and have some specific comments that I'd like to submit as an individual as we head towards WGLC.
Hi Gorry,

thank you vert much for the detailed comments. I hope they are addressed by the next revision.
See my comments in-line. Please let me know if they address your comments. They have been include
in rev-01.

Best regards
Michael
> 
> Gorry
> (as an individual)
> 
> ---
> 
> Abstract
> - The abstract will be read by people who do not read any other part of the document. I think the current abstract is terse. It doesn't say some things which I think it should. Missing is a statement that says this defines a new chunk flag, called the I-bit, that can be set by the SCTP stack or by an application.
OK, the new abstract reads:

<t>This document defines a method for the sender of a DATA chunk
to indicate that the corresponding SACK chunk should be sent
back immediately and not be delayed.
It is done by specifying a bit in the DATA chunk header, called the I-bit,
which can get set either by the SCTP implementation or by the application
using an SCTP stack.</t>
> - I think it is important to say why this is normally the right thing to do - e.g. this remains the normal case.
The text will read:
<t>According to <xref target='RFC4960'/> the receiver of a DATA
chunk should use delayed SACKs. This delaying is completely controlled by
the receiver of the DATA chunk and remains the to be the default behavior.</t>
> - A separate subsection on examples of usage, would be really useful (see note on section 4.1), You could then (forward) refer to this here.
OK, I moved it to a separate section.
> - Insert commas around /but not limited/?
I inserted commas around /but not limited to/
> 
> I think the set of examples provided in 4.1 are really important to publishing the spec. However, the current examples are really terse. Although they seem good examples, the motivation for using the I-but is not clear.
> 
> - Is this that the immediate SACK can then expedite the shutdown of a stream?
> 
> - An immediate SACK can then allow the sender to transmit the missing chunk that will release the congestion/receiver window to send new data?
> 
> - I think this it is confusing that we have two lists, one in the second part of the introduction, and one in section 4.1. I'd suggest it may be useful to have one subsection in the introduction to do this.
OK. I have a section which describes the use-cases and refer in the list to this section for the benefits.
> 
> - To me, the important aspect that should have been evident in 4.1 was that there are two use-cases, one when the I-bit is set by the stack as an internal transport function, and one as an application-triggered function via the API in section 6.
You are right. I split the new use cases section into two subsections: application level triggered
and stack level triggered.
> - This contains two RFC 2119 requirements. I think the two are the same, and suggest it is better to have only one. One way to resolve this would be to make the second /SHOULD/ read /should/.
Why not use

<t>On reception of an SCTP packet containing a DATA chunk with the I-bit set,
the receiver SHOULD NOT delay the sending of the corresponding SACK chunk
and send it back immediately.</t>

> 
> Section 6
> - It would perhaps avoid ambiguity to say at the very start that the API allows the I-bit to be set for any specific data chunk.
Not for any specific data chunk via the API, because the upper layer has no control on the
data chunks. It only provides user messages.

So what about starting with:

<t>A socket API implementation based on <xref target='RFC6458'/>
is extended to allow the application to set the I-bit of the last DATA
chunk for each provided user message.
> - I think the "or" is confusing, since the second part is deprecated. Could we make this a separate statement, by replacing the "or" by a stop and starting a new sentence with /The/.
And continuing with:
This can be done by by setting a flag called SCTP_SACK_IMMEDIATELY
in the snd_flags field of the struct sctp_sndinfo structure when using
sctp_sendv() or sendmsg().
If the deprecated struct sctp_sndrcvinfo structure is used instead
when calling sctp_send(), sctp_sendx(), or sendmsg(), the SCTP_SACK_IMMEDIATELY
flags can be set in the sinfo_flags field. When using the deprecated function
sctp_sendmsg() the SCTP_SACK_IMMEDIATELY flag can be in the flags parameter.</t>
> - Section 4.1 implies this need to discuss cost of processing the I-bit when in 4.1 it says: "Please note that it is
I added (as indicated in your other e-mail):
It should be noted that an malicious sender can force its peer to send packets
containing SACK chunks for each received packet containing DATA chunks instead
of every other. However, every receiver has to be able to do this anyway.
It might be configured to do so or has to do this because of packet loss or
reordering in the network.</t>

> - This seems like an informative reference?
Done.