lwIP - A Lightweight TCP/IP stack - Bugs: bug #20515, TCP delayed ack does not work as...
You are not allowed to post comments on this tracker with your current authentication level.
bug #20515: TCP delayed ack does not work as expected
Submitter: | Simon Goldschmidt <goldsimon> | ||
Submitted: | Wed 18 Jul 2007 06:46:51 PM UTC | ||
Category: | TCP | Severity: | 2 - Minor |
Item Group: | Change Request | Status: | Fixed |
Privacy: | Public | Assigned to: | kieranm |
Open/Closed: | Closed | Planned Release: | None |
lwIP version: | CVS Head |
Jump to the original submission
Tue 31 Mar 2009 02:24:03 PM UTC, comment #30: |
Kieran Mansley <kieranm> |
Tue 31 Mar 2009 01:04:06 PM UTC, comment #29: In principle, if other side follow TCP specification it wouldn't send anything beyond receive window that was announced.
|
Oleg Tyshev <olegreen> |
Tue 31 Mar 2009 12:08:42 PM UTC, comment #28: For the other side to send more than is allowed would be wrong, and we could reject it if we wanted and still be within the TCP spec. However, as we do actually have the window available to receive the data it seems silly to reject it and force them to retransmit again later: this would increase the overhead of the network and reduce efficiency for no gain. |
Kieran Mansley <kieranm> |
Tue 31 Mar 2009 11:38:17 AM UTC, comment #27: OK, with macroses everything is correct.
|
Oleg Tyshev <olegreen> |
Tue 31 Mar 2009 10:45:53 AM UTC, comment #26: OK, the change I've made is as follows. I also spotted I wasn't using the TCP_SEG_* macros to compare sequence numbers, which was another bug. I've also updated the patch to incorporate this change and attached it here. Oleg: could you check that this resolves your issue with the advertised window going to 65535? If so, I'll check it in so that it can get some wider testing.
|
Kieran Mansley <kieranm> |
Tue 31 Mar 2009 10:29:51 AM UTC, comment #25: Ahh, I see what it is. rcv_wnd is greater than rcv_ann_wnd as it should be. The problem is that rcv_nxt becomes greater than rcv_ann_right_edge due to receiving the zero window probe. I'll fix the code to not assume this. Thanks for spotting this |
Kieran Mansley <kieranm> |
Tue 31 Mar 2009 10:13:49 AM UTC, comment #24: Hmm, that's odd. rcv_wnd should always be >= rcv_ann_wnd. There must be a problem with that assumption if you're seeing something fixed by this. |
Kieran Mansley <kieranm> |
Tue 31 Mar 2009 10:01:17 AM UTC, comment #23: I replaced all occurencies of rcv_wnd in comparisons with rcv_ann_wnd.
|
Oleg Tyshev <olegreen> |
Mon 30 Mar 2009 06:08:20 PM UTC, comment #22: I think exists one more error in implementation window update.
|
Oleg Tyshev <olegreen> |
Mon 30 Mar 2009 04:50:51 PM UTC, comment #21: Then it should be fine. Just wanted to make sure I got that right. |
Simon Goldschmidt <goldsimon> |
Mon 30 Mar 2009 12:09:52 PM UTC, comment #20: The "every 2nd packet" bit should be guaranteed by the calls to tcp_ack() in tcp_receive().
|
Kieran Mansley <kieranm> |
Sun 29 Mar 2009 03:56:13 PM UTC, comment #19: Nice work! Let me summarize what I understood from reading the code, only to make sure I understood it correctly:
|
Simon Goldschmidt <goldsimon> |
Fri 27 Mar 2009 12:37:31 PM UTC, comment #18: I've reworked tcp_recved and attach a patch here that I think will help a lot.
|
Kieran Mansley <kieranm> |
Tue 15 Apr 2008 05:27:09 PM UTC, comment #17: Shouldn't we mark that part of code with a big fat warning (and the bug number) - e.g. "FIXME: this is necessary to fix bug #12345"??? |
Simon Goldschmidt <goldsimon> |
Tue 15 Apr 2008 04:07:52 PM UTC, comment #16: re: comment #14. Although we currently shrink the window I wish we didn't. That was put in for 1.3 to deal with an even worse problem whose bug number escapes me just now. I'm hoping that in fixing this issue with window updates we'll also fix the window being shrunk. |
Kieran Mansley <kieranm> |
Tue 15 Apr 2008 04:04:19 PM UTC, comment #15: I don't think we'd need a timer for window updates. The rule you quote is for ACKs which should (if we didn't then subsequently send a window update) already be for every other packet. Perhaps I should write some code for what I'm suggesting for the window updates and that would make it clearer. |
Kieran Mansley <kieranm> |
Tue 15 Apr 2008 04:03:20 PM UTC, comment #14:
|
Jim Pettinato <jim_pettinato> |
Tue 15 Apr 2008 03:57:20 PM UTC, comment #13:
|
Simon Goldschmidt <goldsimon> |
Tue 15 Apr 2008 02:21:00 PM UTC, comment #12: I never want to shrink the advertised window. That is naughty and can lead to other problems.
|
Kieran Mansley <kieranm> |
Tue 15 Apr 2008 12:18:34 PM UTC, comment #11: Some more info on this...
|
Jim Pettinato <jim_pettinato> |
Tue 15 Apr 2008 08:34:32 AM UTC, comment #10: If we keep track of the last value advertised we can then send an immediate update only when:
|
Kieran Mansley <kieranm> |
Mon 14 Apr 2008 08:30:24 PM UTC, comment #9:
|
Jim Pettinato <jim_pettinato> |
Thu 19 Jul 2007 08:56:45 AM UTC, comment #8: OK, that sounds better, sorry for misunderstanding you. To be honest what we want is to keep track of the last value that was advertised (not just what it was before the most recent increase) and use that to work out the difference. That's a bigger change though |
Kieran Mansley <kieranm> |
Thu 19 Jul 2007 08:54:42 AM UTC, comment #7: Maybe we should define exactly when we want to send an ACK? At least from the current code, this isn't really clear to me. |
Simon Goldschmidt <goldsimon> |
Thu 19 Jul 2007 08:51:05 AM UTC, comment #6:
|
Simon Goldschmidt <goldsimon> |
Thu 19 Jul 2007 08:36:37 AM UTC, comment #5: The "send an update only if the window is big" is probably some rather simplistic silly window avoidance. This could certainly be improved, but the alternative suggested would probably make it worse. We need to send a window update especially when the window is big (or more accurately when it has just increased by a large amount) as that allows the sender to send more packets. Sending an update when the window is small doesn't help the sender - it knew it was small already and so it only allows it to send a little bit more data.
|
Kieran Mansley <kieranm> |
Thu 19 Jul 2007 08:27:47 AM UTC, comment #4: The change from >= TCP_WND/2 to <= TCP_WND/2 was deliberate! You must keep in mind that most of the time, the ACK that is sent in tcp_recved() is only a window update. What's the use of sending a window update if the current receive window is big enough anyway?
|
Simon Goldschmidt <goldsimon> |
Thu 19 Jul 2007 07:52:21 AM UTC, comment #3: I would be extremely cautious about a change like this. Although it might make the traffic pattern you use faster, it could make another pathologically worse. I don't have a specific example, and not against the change on principle, but would prefer to wait on this till after 1.3.0 at least.
|
Kieran Mansley <kieranm> |
Wed 18 Jul 2007 08:38:29 PM UTC, comment #2: I propose the following change in tcp_recved():
if (!(pcb->flags & TF_ACK_DELAY) &&
if (old_win <= TCP_WND/2) {
|
Simon Goldschmidt <goldsimon> |
Wed 18 Jul 2007 06:47:33 PM UTC, comment #1: Oh, a link to an RFC:
|
Simon Goldschmidt <goldsimon> |
Wed 18 Jul 2007 06:46:51 PM UTC, original submission:
On a full-streaming connection (i.e. sending only MSS-size packets, e.g. a good FTP connection), other stacks (I know...) send an ACK with every second packet, while lwIP sends an ACK with every packet (if the rcv_wnd is > TCP_WND/2).
|
Simon Goldschmidt <goldsimon> |
Depends on the following items: None found
Items that depend on this one: None found
There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.
Follow 10 latest changes.
Date | Changed by | Updated Field | Previous Value | => | Replaced by |
---|---|---|---|---|---|
2009-03-31 | kieranm | Status | None | Fixed | |
Open/Closed | Open | Closed | |||
2009-03-31 | kieranm | Attached File | #17817 | Removed | |
2009-03-31 | kieranm | Attached File | - | Added window_updates, #17844 | |
2009-03-30 | olegreen | Attached File | - | Added tcp_zerowin65535.pcap, #17836 | |
2009-03-27 | kieranm | Attached File | - | Added window_updates, #17817 | |
2008-04-15 | kieranm | Assigned to | None | kieranm | |
2008-03-04 | kieranm | Planned Release | 1.3.1 | ||
2007-07-18 | goldsimon | Attached File | - | Added tcp_recved.patch, #13383 | |
Summary | TCP delyed ack does not work as expected | TCP delayed ack does not work as expected |
Powered by Savane 3.13-f8d8.
Corresponding source code
OK, I've checked the patch in. Resolving this bug as fixed.