Wed 16 Nov 2016 03:19:09 PM UTC, comment #10:
Patch committed in f79eabd24b0b266d0263b331afdc4df1fc545e22
|
Wed 16 Nov 2016 08:11:58 AM UTC, comment #9:
> tcp_output() is called from tcp_input(), so the logic will execute in the case you described.
Right, that was what I missed. In that case I'm OK with your patch.
|
Tue 15 Nov 2016 02:53:31 PM UTC, comment #8:
tcp_output() is called from tcp_input(), so the logic will execute in the case you described.
I think if we just had the check in tcp_receive(), we'd have a race condition between the application submitting the next segment and processing the window update. Consider the following case:
1. Window update comes in, ACKS all in-flight data and reduces the window to < 1 MSS (default segment allocation size)
2. No data is buffered in TCP, send pathway idle
3. Application sends at least 1 MSS of data
4. Logic is triggered via tcp_output() and persist timer is started
Using tcp_output() seemed appropriate because we are starting the persist timer in place of where we would normally fill the remaining window with a segment. So any execution paths that send segments should be triggering this logic
|
Tue 15 Nov 2016 11:36:41 AM UTC, comment #7:
Your patch starts the persist timer in tcp_output(). I wonder if that is enough. Couldn't we come into the same siatuation when we receive a window that is smaller than the next segment to be sent?
Might be as easy as changing tcp_receive() to start the persist timer if "pcb->snd_wnd < pcb->unacked->len" or something like that...
|
Mon 14 Nov 2016 09:56:13 PM UTC, comment #6:
Simon,
I've attached a new patch which starts the persist timer in the case we've identified.
I've tested in my environment and the issue is handled correctly.
Should I also update the CHANGELOG bugfix section with an entry for this issue? I'm not sure what level of bugs get recorded there
(file #38961)
|
Mon 14 Nov 2016 03:57:13 PM UTC, comment #5:
Let me sum up what I think would be correct behavior for lwIP:
- RTO timer prevents connection deadlock while unacked != NULL
- persist timer prevents connection deadlock when unacked == NULL and we have a zero-window-like condition (either real zero window or received window too small for next segment to be sent, which might also happen when enqueueing a segment)
|
Mon 14 Nov 2016 03:53:36 PM UTC, comment #4:
> this was LwIP's legacy behavior which was removed in bug
> #34517
Right. Sorry I missed your post in that bug 2 weeks ago...
And I'm also sorry I can't remember which bug I referenced there. Too long ago. I should have put the number there!
Anyway, seems like Kieran was right back then and this is what you have found now.
However, the behaviour before that was that RTO and persist timer were running at the same time, so that when persist timer fired, it sent 1 byte of an unacked segment instead of a retransmission. That was wrong, but my fix was obviously wrong, too.
|
Mon 14 Nov 2016 03:27:56 PM UTC, comment #3:
Simon,
I'm fine with starting the persistent timer in the specific case I've identified. I'm a little surprised at the suggestion because this was LwIP's legacy behavior which was removed in bug #34517 (commit d8f090a7595a79050c435545d00efc1261b9691c).
Are you more comfortable with using the persistent timer with the extra conditions I've identified (making sure wnd is snd.wnd and pcb->unacked == NULL)? The old behavior would start the timer at the end of sending a window's worth of data if there was still a segment that didn't fit
I can do some testing with this solution in my test environment.
|
Mon 14 Nov 2016 10:20:57 AM UTC, comment #2:
The patch looks written well, but it's still a lot of code. Can't we instead use the persist timer here by starting it when we enter such a condition where we can't send because of segment sizes?
I agree with your patch, performance might be better, but our TCP has already grown so much in the last years, I'd love to keep it as small as we can.
|
Fri 11 Nov 2016 02:27:29 PM UTC, comment #1:
Simon,
Did you have a chance to examine at this issue yet? What are you thoughts about a solution that splits the pbuf?
|
Fri 04 Nov 2016 06:40:18 PM UTC, original submission:
This bug continues the discussion that happened way back in bug #34517 where it was identified that if LwIP receives a window value smaller than the current unsent segment and there is no unacknowledged data, LwIP won't send and the subsequent window update can be dropped. When the update is dropped, the connection deadlocks because we didn't fill the window entirely and engage zero window probing
I've been able to observe this problem with git master on my products. I'm seeing the receiver's window is less than 1 MSS but the next packet on the unsent queue is 1 MSS, so LwIP stops sending. Then the window update from the receiver is dropped (verified update is sent in wireshark, but tcphdr in tcp_in.c only shows the previous ACK with < 1 MSS window).
In order to address this. I started with the original persistent timer patch from bug #20511 which contained support for splitting the unsent segment to fill the window (which was rejected due to complexity)
I modified the logic to use the splitting very conservative and target only this case (rather than always filling the window by splitting). Only when unsent segment is smaller than the send window (not congestion) AND the unacked queue is empty.
I further re-wrote the tcp_split_unsent_seg() function to be vastly simpler. The original implementation didn't use pbuf and TCP segment utility functions, vastly complicating the segment creation and pbuf copying. Also it didn't seem to be correct for read-only pbufs as it used memmove on the pbuf being split. My implementation creates a new segment for the remainder payload (copying the remainder) and then trims the original pbuf.
Please see attached patch and provide feedback
|