buglwIP - 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: 

OK, I've checked the patch in.  Resolving this bug as fixed.

Kieran Mansley <kieranm>
Group Member
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.
There is no reducing of efficiency.

It is not powerful argument, but if we accept packets that "almost:)" in announced window we increase the danger of TCP attacks.

For me the only reason is simplicity.
If we announced something we should follow this rule ourselves.
Can you remember dispute early that we shouldn't close already announced window?

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>
Group Member
Tue 31 Mar 2009 11:38:17 AM UTC, comment #27: 

OK, with macroses everything is correct.

That is right rcv_ann_wnd <= rcv_wnd.
Why do you think we should compare sequence number of the new received TCP packet with rcv_wnd?
Announced was only rcv_ann_wnd.
And if other side is trying to send something outer rcv_ann_wnd it should be rejected.
(the only exception is probe zero window)


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.


diff -r 02a296cfc521 core/tcp.c
--- a/core/tcp.c
+++ b/core/tcp.c
@@ -393,13 +393,19 @@ u32_t tcp_update_rcv_ann_wnd(struct tcp_
 {
   u32_t new_right_edge = pcb->rcv_nxt + pcb->rcv_wnd;

-  if (new_right_edge >= pcb->rcv_ann_right_edge + pcb->mss) {
+  if (TCP_SEQ_GEQ(new_right_edge, pcb->rcv_ann_right_edge + pcb->mss)) {
     /* we can advertise more window */
     pcb->rcv_ann_wnd = pcb->rcv_wnd;
     return new_right_edge - pcb->rcv_ann_right_edge;
   } else {
-    /* keep the right edge of window constant */
-    pcb->rcv_ann_wnd = pcb->rcv_ann_right_edge - pcb->rcv_nxt;
+    if (TCP_SEQ_GT(pcb->rcv_nxt, pcb->rcv_ann_right_edge)) {
+      /* Can happen due to other end sending out of advertised window,
+       but within actual available (but not yet advertised) window /
+      pcb->rcv_ann_wnd = 0;
+    } else {
+      /* keep the right edge of window constant */
+      pcb->rcv_ann_wnd = pcb->rcv_ann_right_edge - pcb->rcv_nxt;
+    }
     return 0;
   }
 }


(file #17844)

Kieran Mansley <kieranm>
Group Member
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>
Group Member
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>
Group Member
Tue 31 Mar 2009 10:01:17 AM UTC, comment #23: 

I replaced all occurencies of rcv_wnd in comparisons with rcv_ann_wnd.
With this change everything works.

Oleg Tyshev <olegreen>
Mon 30 Mar 2009 06:08:20 PM UTC, comment #22: 

I think exists one more error in implementation window update.

To the other side would be sent always rcv_ann_wnd.
It is means, other side is not allowed to send something out of the window. But in tcp_in.c will be always used rcv_wnd.
It is not correct.

For exampe in tcp_receive()

  /* The sequence number must be within the window (above rcv_nxt
     and below rcv_nxt + rcv_wnd) in order to be further
     processed. */
  if (TCP_SEQ_BETWEEN(seqno, pcb->rcv_nxt,
                      pcb->rcv_nxt + pcb->rcv_wnd - 1)){

in my case
  rcv_ann_wnd == 0, rcv_wnd == 1000
I get zero window probe packet (tcplen == 1)

and then in tcp_update_rcv_ann_wnd() we go to the the second branch,
rcv_ann_wnd = 65535

 


(file #17836)

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>
Group administrator
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().

tcp_recved() will call tcp_ack() only to send a window update in the case where it would allow the sender to transmit a significant amount more data.  This window-update-ACK is in addition to the data-path ACKs, so we may still send more ACKs than is strictly necessary, but I think it should be much better than before.

Happy to change that function name.


Kieran Mansley <kieranm>
Group Member
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:

- until now, tcp_ack was responsible for window-update-acks
- until now, we called tcp_ack from tcp_received and tcp_receive

- now, tcp_received calculates the window-update-ack on its own
- tcp_receive still uses the 'old' tcp_ack -> called for every packet received

If I remember the RFC correctly, the goal should be to send ACKs/window updates for every 2nd packet at least (no matter how big it is) and again if the window increase is notably high.

I guess this is achieved with your patch (although I would rather test it before releasing lwIP with this).


Only one comment on the code: To let future developers know what's going on, I'd rename the function tcp_set_header to tcp_output_set_header: We already had the idea to set the header when enqueueing (not sending) a segment to update chksum on-the-fly and we should prevent tcp_set_header being used in this case (as the value of rcv_ann_right_edge would then be wrong).

Simon Goldschmidt <goldsimon>
Group administrator
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.

As this is quite a critical part of the code, I would welcome review and comments.


Kieran Mansley <kieranm>
Group Member
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>
Group administrator
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>
Group Member
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>
Group Member
Tue 15 Apr 2008 04:03:20 PM UTC, comment #14: 


Simon... doesn't tcp_fasttmr already handle delayed ACKs?

Also, it appears we already do 'shrink' the window as described in the RFC (in tcp_output_segment):

  /* silly window avoidance */
  if (pcb->rcv_wnd < pcb->mss) {
    seg->tcphdr->wnd = 0;
  } else {
    /* advertise our receive window size in this TCP segment */
    seg->tcphdr->wnd = htons(pcb->rcv_wnd);
  }

I thought I'd seen that somewhere before. ;)

So.. maybe a flag in the pcb when we've last sent a 0 window, and an immediate ack in tcp_recved() if the flag is set but the window has become larger than mss (or using the algorithm Simon mentioned)?

Jim Pettinato <jim_pettinato>
Group Member
Tue 15 Apr 2008 03:57:20 PM UTC, comment #13: 


> I'm happy to incorporate a "only increase the advertised window
> by a minimum of a segment" rule in there though, which should do
> silly window avoidance.


I also think so. Although that isn't enough (because it would mean we would send an ACK with every segment).

I think I already worked on this a bit, but it's been a while ago...

RFC 1122 chapter 4.2.3.2 says "the delay MUST be less than 0.5 seconds, and in a stream of full-sized segments there SHOULD be an ACK for at least every second segment."

This would mean we need
- the last advertised window and
- a timer
and send an ACK if the last advertised window is <= current_wnd - (2 * mss) OR the timer has expired.

These are 2*16 bit plus a little code to check the timer in tcp_slow_tmr, I don't think it's too much: it saves a lot of ACKs, after all!

Simon Goldschmidt <goldsimon>
Group administrator
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.

I'm happy to incorporate a "only increase the advertised window by a minimum of a segment" rule in there though, which should do silly window avoidance.

Kieran Mansley <kieranm>
Group Member
Tue 15 Apr 2008 12:18:34 PM UTC, comment #11: 

Some more info on this...

It looks like the existing code may have been an attempt to implement Clark's solution for silly window avoidance by the receiver... but it's missing an important part. Clark's algorithm CLOSES the window until a complete segment can be received or the buffer is half empty, which implies that an immediate ACK to reopen the window should occur when those thresholds are again met. The code that's there almost does that... but it doesn't do it only if the window was closed, it does it all the time... and that's the problem.

So... do we want to implement a silly window avoidance solution on the receiving end completely as well? That would also mean we should be closing the window when it becomes too small for efficient transfer.

Jim Pettinato <jim_pettinato>
Group Member
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:
 - the last value advertised was small; or
 - the change in the available window is large

These two rules combined would effectively mean that we advertise a window when it would make a difference to the sender (i.e. when it might allow the sender to send more data) and avoid sending window updates when they'd make little difference.

Kieran Mansley <kieranm>
Group Member
Mon 14 Apr 2008 08:30:24 PM UTC, comment #9: 



Okay, I've taken a look at this issue, since I'm sick of settling for a separate ACK packet being sent for every Modbus/TCP command received when I KNOW I'm going to reply with data anyway. The delayed ACK feature is essentially useless in this type of application.

I've got a 64K window, it's reduced by 12-255 bytes by an incoming Modbus command, and lwIP wants to send an immediate window update? That seems awfully silly to me.

And, if you actually follow all of the program flow, at the point tcp_recved is called by the app, the TF_ACK_DELAY flag is ALWAYS set for the pcb... it's set when the segment is handled in tcp_receive(). It appears to me that there is no need to test the previous state of the pcb's flags here, just call tcp_ack_now() when you need an immediate window update sent.

Of course, we still have to determine the precise definition of when we need an immediate window update sent... ;)

Now that 1.3.0 has been released, I'd like to form a consensus on how to proceed on this bug.


Jim Pettinato <jim_pettinato>
Group Member
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>
Group Member
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>
Group administrator
Thu 19 Jul 2007 08:51:05 AM UTC, comment #6: 


> 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)


My idea (in the patch) was to send a window update when the previous window (before increasing it in tcp_recved) was small. That's similar as your 'increased by a large amount'...

I'm OK with postponing this until after the release of 1.3.0, but this doesn't keep us from discussing and testing before that release!

Simon Goldschmidt <goldsimon>
Group administrator
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.

As to the other point, the reason I wanted to keep tcp_ack() in tcp_recved() is because I thought you were getting rid of the if(!(pcb->flags & TF_ACK_DELAY)) check around it, and so then it could be much more than just setting the ACK_DELAY flag.

Kieran Mansley <kieranm>
Group Member
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?

If calling tcp_ack, we would change the code from the current status: right now, tcp_ack is not really called. Look at the definition of tcp_ack() in tcp.h:

if((pcb)->flags & TF_ACK_DELAY) { \
                            (pcb)->flags &= ~TF_ACK_DELAY; \
                            (pcb)->flags |= TF_ACK_NOW; \
                            tcp_output(pcb); \
                         } else { \
                            (pcb)->flags |= TF_ACK_DELAY; \
                         }

But as tcp_ack is only 'called' in tcp_recved 'if(!(pcb->flags & TF_ACK_DELAY))', this results in 'pcb->flags |= TF_ACK_DELAY' being set instead of tcp_output being called.

I'm also cautios about this change. But I would like to understand this code.

What the code currently does is setting TF_ACK_DELAY and sending an ACK if the recv_wnd is BIG.

Simon Goldschmidt <goldsimon>
Group administrator
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.

I would also leave the call to tcp_ack(), i.e.:

if (old_win <= TCP_WND/2) {
  tcp_ack_now(pcb);
else {
  tcp_ack(pcb);
}

Also, was the change in from ">= TCP_WND/2" to "<= TCP_WND/2" deliberate?

The reason I'm cautious is that we don't remember exactly why the code was written like that, and what problems were solved by the code being like this.  If we go ahead and change it, we could be rediscovering a set of bugs that were fixed long ago.

Kieran Mansley <kieranm>
Group Member
Wed 18 Jul 2007 08:38:29 PM UTC, comment #2: 

I propose the following change in tcp_recved():

Replace

>>>

  if (!(pcb->flags & TF_ACK_DELAY) &&
     !(pcb->flags & TF_ACK_NOW)) {
    tcp_ack(pcb);
  }
  else if (pcb->flags & TF_ACK_DELAY && pcb->rcv_wnd >= TCP_WND/2){
    tcp_ack_now(pcb);
  }
<<<

by

>>>

  if (old_win <= TCP_WND/2) {
    tcp_ack_now(pcb);
  else {
    pcb->flags |= TF_ACK_DELAY;
  }
<<<

That way, we send a TCP-window-update-ACK only if the previous window size was very small.

Note that the current code, the 'tcp_ack(pcb)' effectively resolves to 'pcb->flags |= TF_ACK_DELAY'.

I'm getting better results with the change, but like to have it tested, so I'm attaching a patch to tcp.c!

(file #13383)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 18 Jul 2007 06:47:33 PM UTC, comment #1: 

Oh, a link to an RFC:

RCF 1122, chapter 4.2.3.2 When to Send an ACK Segment

http://www.faqs.org/rfcs/rfc1122.html

Simon Goldschmidt <goldsimon>
Group administrator
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).

In my opinion, this should be changed.

The solution would be to change the if clause that calls tcp_ack_now() in tcp_recved().

Simon Goldschmidt <goldsimon>
Group administrator

 

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

Attached Files
file #17844:  window_updates added by kieranm (10KiB - application/octet-stream)
file #17836:  tcp_zerowin65535.pcap added by olegreen (154KiB - application/octet-stream)
file #13383:  tcp_recved.patch added by goldsimon (2KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by olegreen (Updated the item)
  • -email is unavailable- added by jim_pettinato (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-03-31 kieranm StatusNone Fixed
        Open/ClosedOpen 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 toNone kieranm
    2008-03-04 kieranm Planned Release 1.3.1
    2007-07-18 goldsimon Attached File- Added tcp_recved.patch, #13383
        SummaryTCP delyed ack does not work as expected TCP delayed ack does not work as expected

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code