buglwIP - A Lightweight TCP/IP stack - Bugs: bug #50614, TCP: new ACK processing, unsent...

 
 

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

bug #50614: TCP: new ACK processing, unsent queue check and pcb->sndbuf update in wrong spot

Submitted by:  Joel Cunningham <jcunningham>
Submitted on:  Wed 22 Mar 2017 09:17:57 PM UTC  
 
Category:  TCP Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  Joel Cunningham <jcunningham>
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

( Jump to the original submission)

Wed 29 Mar 2017 09:33:01 PM UTC, comment #8: 

Committed in c72226114279adb7b5696c49913624add1dc1f33

I'll open a new task for combining the queue processing loops

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 29 Mar 2017 08:03:24 PM UTC, comment #7: 

>> Checking pcb->unsent for that case should NOT be unnecessary


> Ehrm?


Oops, there shouldn't be a double negative in there, should say "NOT be necessary"

I'll get the patch pushed

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 29 Mar 2017 07:57:37 PM UTC, comment #6: 

> Checking pcb->unsent for that case should NOT be unnecessary


Ehrm?

> I'd like to minimize the change and commit this patch


Right. That's fine by me.

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 29 Mar 2017 07:56:43 PM UTC, comment #5: 

> Another patch can generalize the logic to operate on both loops


That should say "both queues"

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 29 Mar 2017 07:55:04 PM UTC, comment #4: 

I agree that ACKs <= pcb->lastack would no longer invoke the check of pcb->unsent.  Checking pcb->unsent for that case should NOT be unnecessary because we would have already performed the check when lastack was updated.  The only way a segment could remain is if the ACK didn't fully ack the segment, at which-point an ACK greater than lastack would be required to ACK it.  Further, if a check was really needed in that case, it should also be performed on unacked as well (which hasn't been the case)

Good point on combining the two loops, they do almost identical logic on the queues.  For right now, I'd like to minimize the change and commit this patch.  Another patch can generalize the logic to operate on both loops

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 29 Mar 2017 06:40:28 PM UTC, comment #3: 

Sorry, this somehow slipped through...
The patch looks pretty straightforward, the only thing it seems to change is to not handle ACKs on pcb->unsent where 'ackno <= pcb->lastack'. But I don't see how that could happen...

An additional improvement might be to combine the two loops, since they perform nearly the same thing on 'unacked' and 'unsent'...

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 29 Mar 2017 01:43:29 PM UTC, comment #2: 

Ping, any one get a chance to look at this yet?

Thanks

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 22 Mar 2017 10:02:39 PM UTC, comment #1: 

Attached is patch doing two things:

1) moves unsent queue check and pcb->snd_buf update inside the conditional for new data ACK
2) unsent queue check can now use TCP_SEQ_LEQ rather than TCP_SEQ_BETWEEN because ackno has already been checked against pcb->snd_nxt

This ends up being a very minor performance improvement for duplicate ACKs because we aren't doing these checks.  Also a minor improvement by switching to TCP_SEQ_LEQ for each segment on the unsent queue that is acknowledged

Please scrutinize the patch to make sure I haven't over looked something since this code is very old :)

(file #40082)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 22 Mar 2017 09:17:57 PM UTC, original submission:  
Joel Cunningham <jcunningham>
Project MemberIn charge of this item.

 

Attached Files

 

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 jcunningham (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-03-29 jcunningham StatusIn Progress => Fixed
        Open/ClosedOpen => Closed
    2017-03-22 jcunningham Attached File- => Added 0001-bug-50614-move-unsent-queue-check-pcb-snd_buf-update.patch, #40082

    Back to the top


    Powered by Savane 3.5