Don't zero tx window when there are no ready segments

[talex5]

If there are no ready segments, leave the tx window alone. Before, we set it to size zero, preventing us from sending any further data. See: #140.

Not sure if this the correct fix, but after some light testing, it seems to help (no more hangs observed so far).

[talex5]

(Travis build failures look related to the recent conduit/http API breakage, not to my changes)

[samoht]

I've rerun the tests.

[talex5]

mirage-www doesn't work with mirage-dev:

Mirage       File "config.ml", line 89, characters 2-13:
Mirage       Error: This function has type
Mirage                Mirage.conduit Mirage.impl -> Mirage.http Mirage.impl
Mirage              It is applied to too many arguments; maybe you forgot a `;'.
Mirage       Command exited with code 2.

[samoht]

Need to clone my fork instead of mirage one to make mirage-www compile ... I'll fix that later today.

Can someone knowledgeable in TCP review that PR? @yomimono or @hannesm maybe?

[samoht]

@talex5 can you rebase with master so there is only one commit on that PR? thanks!

[yomimono]

I'll have a look later today.

[hannesm]

cool. I'd first need to find a TCP/IP illustrated and read up on windowing (won't happen today most likely)

[talex5]

@samoht : done (surprised github couldn't figure this out itself)
@yomimono : thanks!

[balrajsingh]

When a segment arrives it may arrive out of order in which case it gets put
into the "waiting" segments. When a missing segment arrives all the
correctly arranged segments are put into "ready". Usually one segment
arrives and it is in order so its put into ready directly and it proceeds
from there. The window is read off of this segment and used where needed
(to send pkts etc). If multiple segments are released into "ready" because
previous reordering was straightened out because of a packet then,
reasonably, the max window of all the segments ins ready are used. The
problem seems to be when the first out of order packet arrives - at this
point there is nothing in ready so where should the window be read from?
Clearly arbitrarily setting the window to 0 as it currently does is wrong.
So...

Could you try one of the following please (if it's not what you've done
already):

1. if ready is empty then set win from seg instead - this would happen if
   seg arrived out of order and so ready was empty.
2. if ready is empty then set win from Window.ack_win instead - this would
   mean reusing old value of window.

Both are valid solutions - the first means using something newer that the
other side has said, the second is as if this packet was lost so we
continue to operate on older info.

Please let me know if unclear. I don't have a fully working setup here so
am unable to try it myself.

On Fri, Jun 5, 2015 at 2:59 AM, Thomas Leonard notifications@github.com
wrote:

@samoht https://github.com/samoht : done (surprised github couldn't
figure this out itself)
@yomimono https://github.com/yomimono : thanks!

—
Reply to this email directly or view it on GitHub
#146 (comment).

[balrajsingh]

so for 1:

$ diff -u segment.ml segment_test.ml
--- segment.ml 2015-06-05 05:06:52.000000000 -0700
+++ segment_test.ml 2015-06-05 05:08:38.000000000 -0700
@@ -97,9 +97,9 @@
with Not_found -> false *)

(* Determine the transmit window, from the last segment *)

• let window q =
• let window q s =
  try (S.max_elt q).window
• with Not_found -> 0
• with Not_found -> s.window

let is_empty q = S.is_empty q.segs

@@ -140,7 +140,7 @@
let tx_ack =
if seg.ack then begin
StateTick.tick q.state (State.Recv_ack seg.ack_number);

•      let win = window ready in
  

•      let win = window ready seg in
     let data_in_flight = Window.tx_inflight q.wnd in
     let seq_has_changed = (Window.ack_seq q.wnd) <> seg.ack_number in
     let win_has_changed = (Window.ack_win q.wnd) <> win in
  

[talex5]

Thanks for the clarification! (2) is what this pull request does.

[talex5]

[ Travis test timed out. I clicked refresh and it worked the second time. ]

[avsm]

This looks correct to me. From an API perspective, it might be slightly nicer to explicitly differentiate between the default window and the actual window. If this function returned an option instead, then this would be easy to do in the caller.

[balrajsingh]

On further thought, the option of taking it from seg may be more correct as that information is fresher. worth a test at least.

[yomimono]

I think that the really-right thing to do is figure out whether we're failing to process received segments quickly enough (i.e., there's a lot of stuff in the receive queue), and to shrink and grow the window (with a ceiling of MSS) in accordance with that.

As far as quick fixes, I think just returning Default (i.e., MSS) is less wrong than 0 or copying the receive window from a received packet.

[talex5]

@balrajsingh is it always fresher, though? If it's an out-of-order packet then it's probably fresher that the last one (but might not be), but if it's a duplicate ack then it could be very old. e.g. what about this case:

1. Remote is busy. It sends ack1 with a window size of zero.
2. Remote finishes its work and sends ack2 with window size of 64K.
3. Remote thinks ack1 was lost and retransmits it.

Assuming we receive all these segments, we'll end up with a transmit window size of zero, and we'll never get out of this state because the remote end knows we've already received all its segments. I guess in that case we'd deadlock?

[balrajsingh]

The situation (3) would not occur since a pure ACK (no payload) is not
delivered reliably, ie, it's not saved for retransmission. If there is a
payload then the segment may be retransmitted, but it will carry the latest
ACK and window state, so in this case it will be the opened window.
Packets could be reordered in the network to cause this kind of situation,
but then it will appear like it was a window rescinding (not closing) which
is very bad form.

That said, I think using the previous value of the window is fine for now
if the tests are passing.

My concern was that the window is given relative to the ack number - so
they should be read together from the same pkt. So we shouldn't advance
the ack number but still use the old window. Basically, the current code
that picks the max window from reordered pkts is just wrong! The code
should simply implement the following: if the ack number stays the same or
advances then update the ack number and window from that packet!

A simpler interim fix (though I can't test it) would be to always take win
from the seg that is being processed. So

•      let win = window ready in
  

•      let win = seg.window in
  

Also, I don't think it is right to change the window to MSS. Changes of
that kind are done to the congestion window on the transmission side in
response to multiple lost segments, but the rx window should always be the
concrete thing the other side has said.

On Sat, Jun 6, 2015 at 4:52 AM, Thomas Leonard notifications@github.com
wrote:

@balrajsingh https://github.com/balrajsingh is it always fresher,
though? If it's an out-of-order packet then it's probably fresher that the
last one (but might not be), but if it's a duplicate ack then it could be
very old. e.g. what about this case:

1. Remote is busy. It sends ack1 with a window size of zero.
2. Remote finishes its work and sends ack2 with window size of 64K.
3. Remote thinks ack1 was lost and retransmits it.

Assuming we receive all these segments, we'll end up with a transmit
window size of zero, and we'll never get out of this state because the
remote end knows we've already received all its segments. I guess in that
case we'd deadlock?

—
Reply to this email directly or view it on GitHub
#146 (comment).

[balrajsingh]

@talex5 could you please take a look and if possible try out

balrajsingh@fe8f63bfde

I can't test it unf., but I think this is the right fix (I hope it builds
ok).

Great eye spotting the problem. It was basically a vestige of an earlier
state of the stack (when none of the congestion control, retransmissions,
timers, state machine etc existed).

On Sat, Jun 6, 2015 at 8:46 AM, Balraj Singh balrajsingh@ieee.org wrote:

The situation (3) would not occur since a pure ACK (no payload) is not
delivered reliably, ie, it's not saved for retransmission. If there is a
payload then the segment may be retransmitted, but it will carry the latest
ACK and window state, so in this case it will be the opened window.
Packets could be reordered in the network to cause this kind of situation,
but then it will appear like it was a window rescinding (not closing) which
is very bad form.

That said, I think using the previous value of the window is fine for now
if the tests are passing.

My concern was that the window is given relative to the ack number - so
they should be read together from the same pkt. So we shouldn't advance
the ack number but still use the old window. Basically, the current code
that picks the max window from reordered pkts is just wrong! The code
should simply implement the following: if the ack number stays the same or
advances then update the ack number and window from that packet!

A simpler interim fix (though I can't test it) would be to always take win
from the seg that is being processed. So

•      let win = window ready in
  

•      let win = seg.window in
  

Also, I don't think it is right to change the window to MSS. Changes of
that kind are done to the congestion window on the transmission side in
response to multiple lost segments, but the rx window should always be the
concrete thing the other side has said.

On Sat, Jun 6, 2015 at 4:52 AM, Thomas Leonard notifications@github.com
wrote:

@balrajsingh https://github.com/balrajsingh is it always fresher,
though? If it's an out-of-order packet then it's probably fresher that the
last one (but might not be), but if it's a duplicate ack then it could be
very old. e.g. what about this case:

1. Remote is busy. It sends ack1 with a window size of zero.
2. Remote finishes its work and sends ack2 with window size of 64K.
3. Remote thinks ack1 was lost and retransmits it.

Assuming we receive all these segments, we'll end up with a transmit
window size of zero, and we'll never get out of this state because the
remote end knows we've already received all its segments. I guess in that
case we'd deadlock?

—
Reply to this email directly or view it on GitHub
#146 (comment).

[talex5]

@balrajsingh thanks - that looks like a better patch than mine, and it's working for me so far!

[samoht]

@talex5 can you update the PR with the new patch? Thanks!

[talex5]

@samoht done

[samoht]

Merged, thanks!