lwIP - A Lightweight TCP/IP stack - Patches: patch #9384, Partial SACK (RFC 2018) support
You are not allowed to post comments on this tracker with your current authentication level.
patch #9384: Partial SACK (RFC 2018) support
Submitter: | Jakub Schmidtke <jschmidtke> | ||
Submitted: | Wed 21 Jun 2017 06:47:55 PM UTC | ||
Category: | TCP | Priority: | 5 - Normal |
Status: | Done | Privacy: | Public |
Assigned to: | None | Open/Closed: | Closed |
Planned Release: | None |
Jump to the original submission
Thu 27 Jul 2017 06:40:02 PM UTC, comment #33: |
Simon Goldschmidt <goldsimon> |
Wed 26 Jul 2017 09:39:40 PM UTC, comment #32: Agree, SACK_IN can be new ticket, this can be closed! |
Joel Cunningham <jcunningham> |
Wed 26 Jul 2017 07:54:05 PM UTC, comment #31: It is a "partial" SACK support, so I think that's it :)
|
Jakub Schmidtke <jschmidtke> |
Wed 26 Jul 2017 07:40:10 PM UTC, comment #30: Guys, is there anything left here? The patch is pushed, SACK_IN is missing, but I guess this can be closed, right? |
Simon Goldschmidt <goldsimon> |
Mon 03 Jul 2017 07:42:43 PM UTC, comment #29: Right. Just found that in my tests... Thanks for checking! |
Simon Goldschmidt <goldsimon> |
Mon 03 Jul 2017 07:39:35 PM UTC, comment #28: Also, it looks like the cleanup changes are broken.
|
Jakub Schmidtke <jschmidtke> |
Mon 03 Jul 2017 07:34:44 PM UTC, comment #27: Yes, Section 8 of RFC 2018 talks about it:
|
Jakub Schmidtke <jschmidtke> |
Mon 03 Jul 2017 07:31:05 PM UTC, comment #26: I'll have to admit that this was only reading the code, I haven't found the time to test it, yet... |
Simon Goldschmidt <goldsimon> |
Mon 03 Jul 2017 07:23:22 PM UTC, comment #25: I've just pushed the patch with some cleanup additions by myself.
|
Simon Goldschmidt <goldsimon> |
Thu 29 Jun 2017 10:21:38 PM UTC, comment #24: I updated the patch (twice, because I found one more issue - sorry abotu that). |
Jakub Schmidtke <jschmidtke> |
Thu 29 Jun 2017 07:53:49 PM UTC, comment #23:
|
Joel Cunningham <jcunningham> |
Thu 29 Jun 2017 07:52:15 PM UTC, comment #22:
|
Joel Cunningham <jcunningham> |
Thu 29 Jun 2017 06:55:31 PM UTC, comment #21: Ok, thanks! I will post updated patch when it's ready. |
Jakub Schmidtke <jschmidtke> |
Thu 29 Jun 2017 06:18:39 PM UTC, comment #20: I see three problems using memp:
|
Simon Goldschmidt <goldsimon> |
Thu 29 Jun 2017 05:21:37 PM UTC, comment #19: OK, so to have a precalculated array/list/whatever of SACKs, it can either be kept as an array in TCP PCB (current implementation), use memp pools, or somehow be integrated with ooseq.
|
Jakub Schmidtke <jschmidtke> |
Thu 29 Jun 2017 06:43:46 AM UTC, comment #18: Well, about lightweight: we certainly move away from that by adding these speed-related implementations. That why I want them optional. And given that, I guess storing a precalculated array is better than recalculating for every ACK... |
Simon Goldschmidt <goldsimon> |
Wed 28 Jun 2017 08:57:51 PM UTC, comment #17:
|
Joel Cunningham <jcunningham> |
Wed 28 Jun 2017 08:27:12 PM UTC, comment #16:
|
Jakub Schmidtke <jschmidtke> |
Wed 28 Jun 2017 08:04:28 PM UTC, comment #15: Re comment #11:
|
Simon Goldschmidt <goldsimon> |
Wed 28 Jun 2017 08:00:44 PM UTC, comment #14: This response comes kind of late but...
|
Simon Goldschmidt <goldsimon> |
Wed 28 Jun 2017 08:00:34 PM UTC, comment #13:
|
Jakub Schmidtke <jschmidtke> |
Wed 28 Jun 2017 07:46:13 PM UTC, comment #12: Re comment #10:
|
Simon Goldschmidt <goldsimon> |
Wed 28 Jun 2017 07:32:44 PM UTC, comment #11: I could modify the way SACKs are stored, if using a memp approach is preferred.
|
Jakub Schmidtke <jschmidtke> |
Wed 28 Jun 2017 02:11:51 PM UTC, comment #10:
This is because we don't calculate them dynamically, but rather when new data packets are received.
|
Joel Cunningham <jcunningham> |
Tue 27 Jun 2017 07:58:14 PM UTC, comment #9: The comment is wrong. It's because I originally kept SACKs as an u32[8] array (with mixed left,right,left,right,...) edges,
|
Jakub Schmidtke <jschmidtke> |
Tue 27 Jun 2017 07:33:55 PM UTC, comment #8: Jakub,
Shouldn't the maximum be 4 based on the comment in opt.h and my understanding that there is only space for 4 SACK entries in the TCP options?
I found a couple instances like this above where parameters were not separated by a space (the convention in LwIP code)
|
Joel Cunningham <jcunningham> |
Tue 27 Jun 2017 06:11:34 PM UTC, comment #7: Reserving space for SACKs simplifies memory management a lot. However, it means that each data packet that does not have SACKs wastes up 36 bytes (max 4 SACKs take 8 bytes, plus 2 bytes for option header, plus 2 bytes of padding to ensure proper alignment). Is it OK to do that?
|
Jakub Schmidtke <jschmidtke> |
Tue 27 Jun 2017 05:40:30 PM UTC, comment #6:
|
Simon Goldschmidt <goldsimon> |
Tue 27 Jun 2017 06:57:38 AM UTC, comment #5: The problem with SACKs in data packets is that the size of the TCP header is calculated when the data segment is created.
|
Jakub Schmidtke <jschmidtke> |
Tue 27 Jun 2017 06:41:38 AM UTC, comment #4: From what I read, the pbuf allocation is the limitation why you only include SACKs in empty ACKs? I would have thought it's the memory allocation (calculate the SACKs on the fly and send them out directly), but reading the patch, you added the info to the tcp pcb, so it should work.
|
Simon Goldschmidt <goldsimon> |
Mon 26 Jun 2017 04:26:58 PM UTC, comment #3: I saw this improve the performance while downloading files over a high bandwidth link with somewhat higher latency (around 100ms).
|
Jakub Schmidtke <jschmidtke> |
Mon 26 Jun 2017 02:44:39 PM UTC, comment #2: I can help spin up on SACKS support and hopefully provide some review feedback
|
Joel Cunningham <jcunningham> |
Wed 21 Jun 2017 08:04:19 PM UTC, comment #1: Without having the time to look at the patch file, I'd greatly appreciate having SACK support. I know it's not 'lw' any more then, but I think we should have it as well.
|
Simon Goldschmidt <goldsimon> |
Wed 21 Jun 2017 06:47:55 PM UTC, original submission:
This patch adds partial SACK (RFC 2018) support.
|
Jakub Schmidtke <jschmidtke> |
Depends on the following items: None found
Items that depend on this one: None found
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 6 latest changes.
Date | Changed by | Updated Field | Previous Value | => | Replaced by |
---|---|---|---|---|---|
2017-07-27 | goldsimon | Category | None | TCP | |
Status | None | Done | |||
Open/Closed | Open | Closed | |||
2017-06-29 | jschmidtke | Attached File | - | Added 0001-Partial-SACK-RFC-2018-support_v3.patch, #41078 | |
2017-06-29 | jschmidtke | Attached File | - | Added 0001-Partial-SACK-RFC-2018-support_v2.patch, #41077 | |
2017-06-21 | jschmidtke | Attached File | - | Added 0001-Partial-SACK-support.patch, #41016 |
Powered by Savane 3.13-f8d8.
Corresponding source code
Right. I've added task #14591 for SACK_IN.