Hi Vlad,
The only thing that I find slightly odd about this behavior is that the bundling
occurs at the end of message and so I asked myself why aren't we accounting
for SACK chunk at segmentation time?
We could simply do something like this (completely not tested):
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index 1748ef9..3bca0a2 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -193,6 +193,15 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct
sctp_association *asoc,
hmac_desc->hmac_len);
}
+ /* Check to see if we have a pending SACK and try to let it be bundled
+ * with this message. Do this if we don't have any data to send. To
+ * check that, look at out_qlen and retransmit list.
+ */
+ if (timer_pending(&asoc->timers[SCTP_EVENT_TIMEOUT_SACK]) &&
+ asoc->outqueue.out_qlen == 0 &&
+ list_empty(&asoc->outqueue.retransmit))
+ max -= WORD_ROUND(sizeof(sctp_sack_chunk_t));
+
whole = 0;
first_len = max;
Doesn't this reduce the size of all chunks in a message? You've
descremented "max", which I think
is the maximum size of all chunks in the message. I could see maybe
doing this to leave space
in only the first chunk (ie: just decrement first_len by
sizeof(sctp_sack_chunk_t), but even that
doesn't buy much I think. It wouldn't reduce the number of packets
sent, it would only mean
that the SACK is bundled with the first fragment, and then an extra DATA
fragment would need
to be created to send the last sizeof(sctp_sack_chunk_t) bytes of
DATA. Example:
Assume that the PMTU is 1000, that the user is sending a message of 1990
bytes, and
that sizeof(sctp_sack_chunk_t) is 16. With my scheme, we'd send this:
DATA (1000 bytes)
SACK
DATA (990 bytes)
with your scheme exactly as you propose it, we'd send
SACK + DATA (1000 - 16 = 984 bytes)
DATA (984 bytes)
DATA (22 bytes)
If the scheme is modified to only reduce the size of the first fragment,
we'd send:
SACK + DATA (984 bytes)
DATA (1000 bytes)
DATA (6 bytes)
The main point being that if a SACK won't fit in the last chunk of a
message, then your scheme
would just push all the data down so that a new DATA chunk has to be
sent to send the last
few bytes of data,
I don't think there's any real advantage in sending the SACK in the
first packet rather than the
last one, but I agree that it seems to make more sense. It does seem
to complicate the
code a bit though. At the moment, all the code related to piggybacking
SACKs is in
one place, whereas with this patch it'd be spread out to seemingly
unrelated code.
--Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
