[bug #27329] dupacks by unidirectional data transmit

[Ben Asselstine-2]

URL:
  <http://savannah.nongnu.org/bugs/?27329>

                 Summary: dupacks by unidirectional data transmit
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: olegreen
            Submitted on: Thu 27 Aug 2009 09:47:39 AM GMT
                Category: TCP
                Severity: 3 - Normal
              Item Group: Faulty Behaviour
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release:
            lwIP version: CVS Head

    _______________________________________________________

Details:

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.





    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Ben Asselstine-2]

Follow-up Comment #1, bug #27329 (project lwip):

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?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Ben Asselstine-2]

Follow-up Comment #2, bug #27329 (project lwip):

>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;
}


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Ben Asselstine-2]

Update of bug #27329 (project lwip):

         Planned Release:                         => 1.3.2                  


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Ben Asselstine-2]

Update of bug #27329 (project lwip):

                  Status:                    None => Fixed                  
             Assigned to:                    None => goldsimon              
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up 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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Ben Asselstine-2]

Follow-up Comment #4, bug #27329 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Ben Asselstine-2]

Update of bug #27329 (project lwip):

                  Status:                   Fixed => In Progress            
             Assigned to:               goldsimon => kieranm                
             Open/Closed:                  Closed => Open                  

    _______________________________________________________

Follow-up 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.



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Ben Asselstine-2]

Follow-up Comment #6, bug #27329 (project lwip):

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. */



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Ben Asselstine-2]

Follow-up Comment #7, bug #27329 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Ben Asselstine-2]

Follow-up Comment #8, bug #27329 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Ben Asselstine-2]

Follow-up Comment #9, bug #27329 (project lwip):

It looks good for me.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Ben Asselstine-2]

Follow-up Comment #10, bug #27329 (project lwip):

I think this can be closed.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Ben Asselstine-2]

Update of bug #27329 (project lwip):

                  Status:             In Progress => Fixed                  
             Open/Closed:                    Open => Closed                


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?27329>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel