buglwIP - A Lightweight TCP/IP stack - Bugs: bug #27329, dupacks by unidirectional data...

 
 

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

bug #27329: dupacks by unidirectional data transmit

Submitter:  Oleg Tyshev <olegreen>
Submitted:  Thu 27 Aug 2009 09:47:39 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
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

Wed 11 Nov 2009 04:51:46 PM UTC, comment #10: 

I think this can be closed.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 30 Oct 2009 12:38:09 PM UTC, comment #9: 

It looks good for me.

Oleg Tyshev <olegreen>
Thu 29 Oct 2009 04:38:40 PM UTC, comment #8: 

I just tested it and can confirm it works with my own unidirectional data transfer.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 Oct 2009 03:46:25 PM UTC, comment #7: 

I'll check this in, as I think it's a definite improvement, but would be grateful if Oleg could confirm it works for him and avoids the original problem before I release 1.3.2

Kieran Mansley <kieranm>
Group Member
Thu 29 Oct 2009 03:34:17 PM UTC, comment #6: 

OK, diff makes a bad job of displaying my changes, so here's what the code block that deals with dupacks now looks like:


    /* (From Stevens TCP/IP Illustrated Vol II, p970.) Its only a
     * duplicate ack if:
     * 1) It doesn't ACK new data
     * 2) length of received packet is zero (i.e. no payload)
     * 3) the advertised window hasn't changed
     * 4) There is outstanding unacknowledged data (retransmission timer running)
     * 5) The ACK is == biggest ACK sequence number so far seen (snd_una)
     *
     * If it passes all five, should process as a dupack:
     * a) dupacks < 3: do nothing
     * b) dupacks == 3: fast retransmit
     * c) dupacks > 3: increase cwnd
     *
     * If it only passes 1-3, should reset dupack counter (and add to
     * stats, which we don't do in lwIP)
     *
     * If it only passes 1, should reset dupack counter
     *
     */

    /* Clause 1 */
    if (TCP_SEQ_LEQ(ackno, pcb->lastack)) {
      pcb->acked = 0;
      /* Clause 2 */
      if (tcplen == 0) {
        /* Clause 3 */
        if (pcb->snd_wl2 + pcb->snd_wnd == right_wnd_edge){
          /* Clause 4 */
          if (pcb->rtime >= 0) {
            /* Clause 5 */
            if (pcb->lastack == ackno) {
              found_dupack = 1;
              if (pcb->dupacks + 1 > pcb->dupacks)
                ++pcb->dupacks;
              if (pcb->dupacks > 3) {
                /* Inflate the congestion window, but not if it means that
                   the value overflows. */
                if ((u16_t)(pcb->cwnd + pcb->mss) > pcb->cwnd) {
                  pcb->cwnd += pcb->mss;
                }
              } else if (pcb->dupacks == 3) {
                /* Do fast retransmit */
                tcp_rexmit_fast(pcb);
              }
            }
          }
        }
      }
      /* If Clause (1) or more is true, but not a duplicate ack, reset
       * count of consecutive duplicate acks */
      if (!found_dupack) {
        pcb->dupacks = 0;
      }
    } else if (TCP_SEQ_BETWEEN(ackno, pcb->lastack+1, pcb->snd_nxt)){
      /* We come here when the ACK acknowledges new data. */


Kieran Mansley <kieranm>
Group Member
Thu 29 Oct 2009 02:39:33 PM UTC, comment #5: 

I think this needs more work: I didn't think about it enough before.

If data are being sent unidirectional from host1 to host2, then host2 should not be getting duplicate ACKs. (From Stevens TCP/IP Illustrated Vol II, p970.) Its only a duplicate ack if:

   1) It doesn't ACK new data
   2) length of received packet is zero (i.e. no payload)
   3) the advertised window hasn't changed
   4) There is outstanding unacknowledged data
   5) The ACK is == biggest ACK sequence number so far seen (snd_una)
 
If it passes all five, should process as a dupack:
a) dupacks < 3: do nothing
b) dupacks == 3: fast retransmit
c) dupacks > 3: increase cwnd

If it only passes 1-3, should reset dupack counter (and add to stats, which we don't do in lwIP)

If it only passes 1, should reset dupack counter

Our problem in lwIP is that we don't maintain a snd_una sequence variable, and once we've moved everything from unacked to unsent, we can't distinguish unacknowledged data we've sent once already from data we've never sent.

Most obviously we need to add a test for tcplen != 0 before counting a packet as a dupack.  (Clause 2 above).  I would like to restructure the code a little to more closely match the description above, so I've reopened this to work on that.


Kieran Mansley <kieranm>
Group Member
Fri 23 Oct 2009 03:52:55 PM UTC, comment #4: 

Please see comment #3 on bug #27445 as that reworks the fix for this slightly to accommodate the other problem's solution

Kieran Mansley <kieranm>
Group Member
Thu 15 Oct 2009 08:10:35 PM UTC, comment #3: 

I've checked in a patch that does

if (pcb->unacked) {
++pcb->dupacks;
}
else {
pcb->dupacks = 0;
}

since that is less change to the old code (not checking if unsent==NULL), and it works for me.

Thanks for reporting and for the proposed fix. Please reopen if it's not OK.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 31 Aug 2009 09:10:22 AM UTC, comment #2: 


>Which one is running lwIP here, host1, host2 or both?

Host2 has lwip and only receives data.

>Does host1 have to set the ACK flag on all the data packets being sent? I suppose it may, though.

Host1 sends of course packets with ACK flag, ackno in each packet acknoledges e.g. SYN.

>Would unacked empty be enough? Probably not, as unacked might have been moved to unsent in order to retransmit...
>Maybe only incrementing pcb->dupacks if unacked or unsent is not empty would be better?

We need not only condition for incrementing,
but resetting too.
Something like that:
if (pcb->unacked || pcb->unsent) {
  ++pcb->dupacks;
}
else {
  pcb->dupacks = 0;
}

Oleg Tyshev <olegreen>
Fri 28 Aug 2009 09:00:42 AM UTC, comment #1: 

Which one is running lwIP here, host1, host2 or both?

Does host1 have to set the ACK flag on all the data packets being sent? I suppose it may, though.

> I think dupacks variable should be reseted if unacked and unsent
> queues are empty.


Would unacked empty be enough? Probably not, as unacked might have been moved to unsent in order to retransmit...

Maybe only incrementing pcb->dupacks if unacked or unsent is not empty would be better?

Simon Goldschmidt <goldsimon>
Group administrator
Thu 27 Aug 2009 09:47:39 AM UTC, original submission:  

If data are being sent unidirectional from host1 to host2,
host2 receives always duplicated acks.

In this case variable pcb->dupacks with each packet will be incremented.
If host2 send only one packet, after receiving next ACK it goes immediately in fast retransmit.

I think dupacks variable should be reseted if unacked and unsent queues are empty.

Oleg Tyshev <olegreen>

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2009-11-11 kieranm Open/ClosedOpen Closed
        StatusIn Progress Fixed
    2009-10-29 kieranm StatusFixed In Progress
        Assigned togoldsimon kieranm
        Open/ClosedClosed Open
    2009-10-15 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2009-10-15 kieranm Planned Release 1.3.2

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code