buglwIP - A Lightweight TCP/IP stack - Bugs: bug #20511, No persist timer

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #20511: No persist timer

Submitter:  Kieran Mansley <kieranm>
Submitted:  Wed 18 Jul 2007 09:31:50 AM UTC
   
 
Category:  TCP Severity:  4 - Important
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  kieranm
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Jump to the original submission

Fri 01 May 2009 12:27:41 PM UTC, comment #21: 

Closing as fixed (again) due to no reports to the contrary.

Kieran Mansley <kieranm>
Group Member
Thu 26 Mar 2009 02:43:09 PM UTC, comment #20: 

I've checked in a change to set the ACK flag on those probes.  Can anyone confirm whether there is a still an outstanding issue with this bug?

Kieran Mansley <kieranm>
Group Member
Thu 12 Feb 2009 01:39:13 PM UTC, comment #19: 

Status is: "not enough time at the moment"

I've bumped it up to 1.3.1 - should have done that when reopening the bug

Kieran Mansley <kieranm>
Group Member
Wed 11 Feb 2009 07:38:14 PM UTC, comment #18: 

Kieran, what's the status on this? It's still marked as planned for 1.3.0 ...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 18 Dec 2008 02:57:33 PM UTC, comment #17: 

It is definitely lack of ACK flag in zero window probe.
Without it windows host for example doesn't reply to this probe.

I think it should be special tested case for last packet with FIN flag (no data, only FIN flag).

In this case we should allocate not
pbuf_alloc(PBUF_IP, TCP_HLEN + 1, PBUF_RAM)
but only
pbuf_alloc(PBUF_IP, TCP_HLEN, PBUF_RAM)
+ FIN flag

Oleg Tyshev <olegreen>
Fri 28 Nov 2008 10:23:02 AM UTC, comment #16: 

OK, I'll have to give that some thought.  I think it would need some integration with the other parts of the tcp retransmission algorithm as it could result in a large number of packets being retransmitted, and so we should be updating some of the state.  It has the potential to be harmful in that it could put packets onto the wire quite aggresively at a time of loss (i.e. when you should be backing off).

I suspect the root of the problem is failing to recover properly when we do get the window-opening ACK, rather than the probe that we send.


As to the condition in tcp_receive: no, it's deliberately like that to cope with cases where packets have been reordered.  In words, it's approximating:
if segment contains new data
 or segment acks new data
 or segment increases the window
 then...

Kieran Mansley <kieranm>
Group Member
Fri 28 Nov 2008 10:06:19 AM UTC, comment #15: 

(A) This modification below resends the unacked segments till the send window allows.


tcp_zero_window_probe(...)
{
  ... some variables ...
//INSERTION-->
  u32_t wnd;
  u8_t resent_flag;
//<--INSERTION

  ... some LWIP_DEBUGF ...
  seg = pcb->unacked;

//INSERTION-->
  /* While unacked segment is available and window allows to (re-)transmit as a whole */
  wnd = LWIP_MIN(pcb->snd_wnd, pcb->cwnd);
  resent_flag = 0;
  while (seg != NULL &&
         ntohl(seg->tcphdr->seqno) - pcb->lastack + seg->len <= wnd) {
    LWIP_DEBUGF(TCP_OUTPUT_DEBUG,
                ("tcp_zero_window_probe: retransmitting segment %"U32_F"\n",
                 ntohl(seg->tcphdr->seqno)));
    /* Resend segment now */
    tcp_output_segment(seg, pcb);
    resent_flag = 1;

    seg = seg->next;
  }
  /* If there was at least one retransmitted segment, no need for zero-probe, so return */
  if(resent_flag)
    return;
//<--INSERTION

  if(seg == NULL)
    seg = pcb->unsent;
  ...
}


This is the only change I made in tcp_out.cpp.
I can confirm using Wireshark that 9-10 segments are retransmitted when segment lost.
I tested in my test environment and it was running well over the night (~16hrs) without breaking the socket.
Therefore I decided to use it in my code - if you accept it, please commit also into lwip CVS repository.

(B) Please note that the above is a solution for small sender window only, and not for zero receiver window. I cannot test the latter because it doesn't happen in my system. However I tried to set TCP_ACK flag of the 1-byte probe and then I got reply from the receiver. But despite of this, lwip sent out the 1-byte probe again, therefore I suspect that some further checks needed, so would be essential to test that part in
real situation.

WRT tcp_receive, I don't really know the meaning of that condition. It's just an
intuitive fix because it looks like the following pattern:
if ((a1 < a2) ||
    (a1 == a2 && b1 < b2) ||
    (a1 == a2 && b1 == b2 && c1 < c2))
But if "a1 == a2" is missing from the third line, the expression can be true in case of
a1 > a2 which is probably unwanted.

Tamas Somogyi <tsomogyi>
Thu 27 Nov 2008 02:08:09 PM UTC, comment #14: 

OK, reopening.  I'll take another look when I get a chance, but not sure when that will be.

If the previous version works for you, you should use that for the time being, but it does have the problem that it won't probe for more window when there is only a small amount available, and lwIP won't send in that case as we refuse to segment packets to fill a small window.

I haven't had a look at the trace you provide in detail yet, but it will be very useful when working on this, thanks.  The other end should ACK these probes as they are effectively retransmissions.  It might not ACK the first one, but the second will be a duplicate and so should always be ACKed. 

I can't remember why I didn't set the ACK flag, probably just didn't want to work out what to put in the ACK field.  PSH isn't appropriate here.

Regarding the condition in tcp_receive, I think the condition is wrong as it stands, but I'd prefer to change it to use the right_wnd_edge rather than the absolute value of the window so that we only take notice of window changes that increase the window.  By the time we get to that test we've established that the segment contains no new data and acknowledges no new data, so the only reason it would be interesting is if it had a larger window.  This is probably a separate bug - could you open one and describe the problem you're seeing with that condition?


Kieran Mansley <kieranm>
Group Member
Thu 27 Nov 2008 01:23:03 PM UTC, comment #13: 

Kieran, the problem I described in bug #24212 caused by your last modification in tcp_out.c: Rev 1.71 works well, but I get the error using 1.72. The difference is that Zero Window Probe is sent not when snd_wnd is zero, but also when the window is not enough to send the data.

RFC793 "Managing the Window" says:
"The sending TCP must be prepared to accept from the user and send at least one octet of new data even if the send window is zero." and
"When the receiving TCP has a zero window and a segment arrives it must still send an acknowledgment showing its next expected sequence number and current window (zero)."
In my understanding it means that the receiver is not liable for sending ACK to this probe, if its window is nonzero.

This happens in my attached scenario:
https://savannah.nongnu.org/bugs/download.php?file_id=16890
Note that receiver window is 1280 always (and thus non-zero).
Frame#21: Sender sends a segment as usual.
#21-22: segment#1847 lost
#22-26: sender sends 5*142=710 bytes. Including the lost segment, the sent data is 710+142=852 bytes.
#27: receiver ACKs frame#21 and expects the lost segment#1989 and advertises receiver window 1280.
#28: sender sends 426 (=3*142) bytes, so totally 852+426=1278 bytes. Close, right? With the next 142 bytes, 1278+142 > 1280, therefore lwip starts the persist timer in a next timeslice.
#29-34: dark and weird things happen: sender (lwip) and receiver tries to get back to normal way, finally lwip resends the lost segment
#35: receiver ACKs seg#1989
#36: are you still with me? Finally, persist timer event occurs, and lwip sends 1-byte Zero Window Probe.
#37: there's almost 3.4s gap between #36 and 37, which is very big comparing to the normal 0.2s cycle time. It means that receiver doesn't ACK the 1-byte message, perhaps because its window is non-zero.
#38...: lwip tries the Zero Window Probe again, but no success, finally the receiver gets bored and closes the socket.

Remarks:
A) A possible solution could be that instead of sending 1-byte in tcp_zero_window_probe, check if snd_wnd is greater or equal than the segment (seg->tcphdr->seqno) size, then (re-)send the whole segment.

B) Wireshark identifies Zero Window Probe segments as "Broken TCP" - maybe it is wrong, but wouldn't be better to set ACK (and PSH) flag?

So I suggest to re-open this bug. It takes some time to me to investigate in A) and B) as I have to carefully test the solution and this problem occurs randomly (5min...3hours). Welcome for any comment.

- Tamas

PS: In tcp_receive, the condition (pcb->snd_wl2 == ackno && tcphdr->wnd > pcb->snd_wnd) shouldn't be (pcb->snd_wl1 == seqno && pcb->snd_wl2 == ackno && tcphdr->wnd > pcb->snd_wnd)?

Tamas Somogyi <tsomogyi>
Tue 04 Mar 2008 01:12:39 PM UTC, comment #12: 

Closing this as fixed due to lack of reports to the contrary.

Kieran Mansley <kieranm>
Group Member
Tue 15 Jan 2008 01:02:38 PM UTC, comment #11: 

I've modified it to activate the persist timer when we're prevented from sending by a small send window (not just a zero send window). 

Any more comments?

Kieran Mansley <kieranm>
Group Member
Fri 04 Jan 2008 05:23:52 PM UTC, comment #10: 

If we receive whole sized packet, it will be trimmed till window size.
If client application do not receive data,
with one byte probe we receive window update. (Window now one byte less).
And this one byte will be added to the receive queue.
In worst case we can have 100 packets...

>> Persist timer is always bounded between 5 and 60 seconds
>Do you have a reference for that? RFC793 or 1122 don't mention this bound as far as I could see.


At least Stevens say about that.
http://www.uic.rsu.ru/doc/inet/tcp_stevens/tcp_pers.htm

Oleg Tyshev <olegreen>
Fri 04 Jan 2008 01:13:17 PM UTC, comment #9: 


> I don't think that send 1 byte probe very good for our
> implementation.
> Imagine 100 TCP segments in receive queue each one byte long.
> May be better to send whole segment without split?


I wouldn't actually split the packet, just send one byte from it, and leave it unsplit on the unsent queue.  The one byte probe should hopefully result in an ACK and a window update from the other side, which would then allow us to send the whole packet.  So there should only ever be one 1-byte-probe in the receive queue, rather than 100.  The reason I like this method is that it requires very little extra code.  Sending the whole packet would be good in that sense too, but I was a bit uneasy about relying on the receiver to deal with the oversize packet.  If the one byte probe turns out to be causing problems, it should be easy to change in the future to send a whole packet, but I'd rather try one byte probes first.

> Persist timer is always bounded between 5 and 60 seconds


Do you have a reference for that?  RFC793 or 1122 don't mention this bound as far as I could see.

Kieran Mansley <kieranm>
Group Member
Fri 04 Jan 2008 12:53:21 PM UTC, comment #8: 

That is right.
The missed case - the window is not zero, but too small to send whole packet.

I don't think that send 1 byte probe very good for our implementation.
Imagine 100 TCP segments in receive queue each one byte long.
May be better to send whole segment without split?

And one more issue.
In suggested patch initial timeout for persistent timer is 1.5 seconds.
But the Persist timer is always bounded between 5 and 60 seconds.
So instead
const u8_t tcp_persist_backoff[7] = { 3, 6, 12, 24, 48, 96, 120 };
should be something like this
const u8_t tcp_persist_backoff[7] = { 10, 10, 12, 24, 48, 96, 120 };

Oleg Tyshev <olegreen>
Thu 03 Jan 2008 08:54:00 AM UTC, comment #7: 

Thanks for taking a look.

For the avoidance of confusion, I think you're describing the case where the window is not zero, but we can't send because the head of the send queue is bigger than the available window.

This is something I'd overlooked, so I think you're right.  Probably the easiest solution would be to just start the persist timer in that case, and send the one-byte probes just as we would in the zero-window case.  I'll look into this later.

Kieran Mansley <kieranm>
Group Member
Wed 02 Jan 2008 05:31:38 PM UTC, comment #6: 

Hi Kieran,

in original patch if send window is not 0
and we will send segment that greater then send window,
this segment will be splitted, and only first part sent.

In your implementation if send window is not 0
persistent timer is not started and segment can not be
moved from unsent queue to unacked queue.

It seems to me that this case is not covered with this patch.
May be instead of splitted segmented is possible to send whole segment, and other site should trim this segment if it does not have enough place.

Oleg Tyshev <olegreen>
Mon 31 Dec 2007 01:36:01 PM UTC, comment #5: 

I've checked this in, but again would like to see it more widely tested before we close this bug.

Kieran Mansley <kieranm>
Group Member
Thu 20 Dec 2007 03:49:07 PM UTC, comment #4: 

I've merged and simplified the persist timer portion of the patch but I think this at least needs testing to make sure it doesn't break anything else before it can be committed.  Any volunteers?  I'm not in a position to test it myself at the moment and this is the last issue currently gating 1.3.0

(file #14676)

Kieran Mansley <kieranm>
Group Member
Tue 18 Dec 2007 01:50:39 PM UTC, comment #3: 

(from lwip-users, by Per-Henrik Lundblom)

Hi,

I guess it is about time to commit the mods and fixes we have done on the lwIP stack. I have included and attached one large patch (I know it's bad with one patch to fix several problems but take it or leave it). The patch was made against a CVS copy from 2007-09-20.

The patch should fix the following things:

- Silly window problem (bug #20199)

- Implementation of the TCP persist timer which lwIP curretly lacks. The
  persist timer is MANDATORY for any TCP implementation. This
  implementation can be slimmed down. This implementation doesn't share
  any code with the retransmit timer even though it should be possible.
  Also, the tcp_split_unsent_seq() function has code copied from other
  tcp_*() functions. I guess you could leave out the split part of the
  persist timer if you want to reduce code size but then you don't fully
  implement the functionality for the persist timer.

Note that this code was 100% written during work time at connectBlue AB
(www.connectblue.se) but is released under the same license as the rest of lwIP. We really hope it will find its way into the the trunk and that others will find it useful.

Merry Christmas to all of you!

/PH

(file #14652)

Frédéric Bernon <fbernon>
Group Member
Tue 06 Nov 2007 01:03:47 PM UTC, comment #2: 

(from bug #21522, by Oleg Tyshev)

This document has good description of different approaches to zero window and keep alive implementations. http://www.usenix.org/publications/library/proceedings/bos94/full_papers/lin.a

Frédéric Bernon <fbernon>
Group Member
Thu 19 Jul 2007 01:06:16 AM UTC, comment #1: 

This problem can show up under the following circumstances:

If lwIP has send enough data that the "remote" has filled its received window and has returned a zero (or smaller than the segment lwIP wants to send) window, lwIP stops. When the window opens, the remote sends an ACK with a window update. If this packet gets lost (this is a common occurrence), then lwIP won't send data until the other end sends another window update. It will only do this if it has some data to send back. So in a unidirectional data transfer setup, one missed window update/ACK packet results in lwIP locking up.

The reference for this is in the section "Managing the Window" on
page 42 of RFC793:

    ftp://ftp.rfc-editor.org/in-notes/rfc793.txt

And in section 4.2.2.17 of:

    ftp://ftp.rfc-editor.org/in-notes/rfc1122.txt

For previous discussion on this issue, and why it might be a bit tricky, please refer to the following posts in the lwip-users list:

http://lists.nongnu.org/archive/html/lwip-users/2007-06/msg00033.html
http://lists.nongnu.org/archive/html/lwip-users/2007-06/msg00034.html
http://lists.nongnu.org/archive/html/lwip-users/2007-06/msg00037.html
http://lists.nongnu.org/archive/html/lwip-users/2007-06/msg00042.html
http://lists.nongnu.org/archive/html/lwip-users/2007-06/msg00043.html
http://lists.nongnu.org/archive/html/lwip-users/2007-06/msg00032.html
http://lists.nongnu.org/archive/html/lwip-users/2007-06/msg00077.html

Tom Evans <tom_evans>
Wed 18 Jul 2007 09:31:50 AM UTC, original submission:  

TCP implementations must have the persist timer to send zero-window probes allowing it to cope with a lost window update.  We don't currently have one.  Should be straightforward enough as rather similar to the keepalive probes and timer.

Kieran Mansley <kieranm>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #14676:  persist_patch added by kieranm (9KiB - application/octet-stream)
file #14652:  lwip_working.diff added by fbernon (21KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by tsomogyi (Posted a comment)
  • -email is unavailable- added by olegreen (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by tom_evans (Posted a comment)
  • -email is unavailable- added by kieranm (Submitted the item)
  •  

    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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-05-01 kieranm StatusNone Fixed
        Open/ClosedOpen Closed
    2009-02-12 kieranm Planned Release1.3.0 1.3.1
    2008-11-27 kieranm StatusFixed None
        Open/ClosedClosed Open
    2008-03-04 kieranm StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2007-12-31 kieranm StatusNone Ready For Test
    2007-12-20 kieranm Attached File- Added persist_patch, #14676
    2007-12-18 fbernon Attached File- Added lwip_working.diff, #14652
    2007-08-29 kieranm Assigned toNone kieranm

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code