RFC: Latency reducing TCP modifications for thin-stream interactive applications
Subsystems (2)
Thank you for comprehensive feedback. We're sorry it took so long to reply. We will begin the work on a patch set against the net-next tree now and try to follow your formatting and functional advice.
Quoted 2 lines · Ilpo Järvinen
linux-net is users list, use netdev instead on matters relating development...
Bad case of typo :)
Quoted 3 lines
Please make sure that it's based on net-next tree next time since there are lots of changes already in the areas you're touching.
We will do that for the updated patch set.
Quoted 9 lines
Quoted 6 lines
This will also give us time to integrate any ideas that may arise from the discussions here.
We are happy for all feedback regarding this:
Is something like this viable to introduce into the kernel?No idea in general. But it has to be (nearly) minimal and clean if such thing is ever to be considered. The one below is definately not even close minimal in many areas...
We will clean up the noise, redo the formatting and divide it into logical segments for the new patch set.
Quoted 6 lines
Quoted 1 line
Is the scheme for thin-stream detection mechanism acceptable.
Does reduncancy happen in the initial slow start as well (depends on write pattern)? Why it is so in case the stream is to become thick right after initial rtts?
If the window is halved, (but still not at less than 4 segment), the mechanisms will be turned off (due to the < 4 packets in flight limit). If the stream backs off to a minimal window, the mechanisms will be turned on.
The bundling mechanism (if activated) will stay active since it relies on the packet sizes and interarrival time, not on number of packets in flight.
Quoted 5 lines
...In general this function should be split, having a separate skb (non-TCP) function(s) and tcp-side would be ideal.
It's misnamed as well, it won't merge but duplicates?
Quoted 11 lines
Also, this approach is extremely intrusive and adding non-linear seqno things into write queue will require _you_ to do
_fullaudit_over every single place to verify that seqno leaps backwards won't break anything (and you'll still probably miss some cases). I wonder if you realize how easily this kind of change manifests itself as a silent data corruption on stream level and have taken appropriate actions to validate that not a single one of scenario leads to data coming as different out as was sent in (every single byte, it's not enough to declare that application worked which could well happen with corrupted data too). TCP is very coreish and such bugs will definately hit people hard.
We have to admit we don't quite see the problem. Since a page can never be removed before all owners have decleared that they no longer needs it, all data will be correctly present. Also, since the packets are placed linearly on the queue and we don't support when SACK is enabled, no gaps will occur. But maybe we have misunderstood your comment, please let us know if that is the case.
Quoted 9 lines
Quoted 1 line
+
if(TCP_SKB_CB(skb)->seq <= (TCP_SKB_CB(prev_skb)->end_seq - ua_data)){So
ua_datamust beprev_skb->lenor this fails? Whyua_dataneeds such complex setup above?Does this thing of youes depend on skb being not cloned?
We will look into making the calculation ua_data easier and also the cloning requirement. The check is there to avoid duplicate data in the case when a previous packet has been bundled with later ones. However, when we think about it, this might not be neccessary since bundling on retransmission is not allowed to bundle with packets that are yet to be sent. Thank you for pointing this out.
Quoted 10 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ + if ((tp->thin_rdb || sysctl_tcp_force_thin_rdb) && skb == tcp_send_head(sk)) { + tcp_advance_send_head(sk, skb); + } +I kind of missed the point of this change, can you please justify it if it's still needed? I think it must either be bug in your code causing this to happen or unnecessary.
Thank you very much, this was placed there for a part of the development that we found out was unecessary.
Quoted 10 lines
Quoted 4 lines
+ * Make sure that the first skb isnt already in
+ * use by somebody else. */
+
+ if (!skb_cloned(skb)) {Relying on skb's not being cloned will make your change to work in a minority of the cases on many hardwares that have tx reclaims happening late enough. I recently got some numbers about clones after rtt and can claim this to happen for sure!
We were not aware of this and will look into the whole cloned-thin in this patch.
Quoted 5 lines
Quoted 2 lines
+
+if(skb_shinfo(skb)->frag_list || skb_shinfo(skb)->frag_list){If it wasn't cloned, why you do this check?
Good question, oversight by us, thank you for pointing it out.
Quoted 10 lines
I suppose this function was effectively:
{
tcp_write_queue_walk_safe(...){
...checks...
if (tcp_trim_head(...))
break;tcp_collapse_retrans(...);
}
}
Correct, except we dont collapse but bundle, will be modified and simplified in the next version of the patch.
Quoted 4 lines
It's sort of counter-intuitive that first you use redundancy but now in here when the path
_hasshownproblems_you now remove the redundancy if I understand you correctly? The logic is...?
Even though the path has problems (as shown by the need for retransmission), we still bundle as many packets as possible. If you are still not sure, please let us know what part of the code that is confusing.
Quoted 10 lines
It seems very intrusive solution in general. I doubt you succeed in pulling it off as is without breaking something. To me it seems rather fragile approach to make write queue seqno backleaps you're proposing. It also leads to troubles in the truesize as you have noticed. Why not just building those redundancy containing segments at the write time in case the stream is thin, then all other parts would not have to bother about dealing these things? Number of sysctls should be minimized, if they're to be added at all. Skb work functions should be separated from tcp layer things.
Quoted 4 lines
If you depend on non-changing sysctl value to select right branch, you're asking for trouble as the userspave is allowed to change it during the flow as well and even during the ack processing.
As far as we understand this comment, you want us to do it on the application layer instead? Do you mean as a middleware, application-specific solution or something similar? Also, we believe doing it on the application layer will lead to the same delays that we try to prevent, since sent data will be delayed on the transport layer in case of loss.
-Andreas Petlund & Kristian Evensen
Quoted 3 lines · Andreas Petlund
Thank you for comprehensive feedback. We're sorry it took so long to reply. We will begin the work on a patch set against the net-next tree now and try to follow your formatting and functional advice.
Sadly it will be quite a lot of work (I know, many changes have happened).
Quoted 5 lines
Quoted 2 lines · Ilpo Järvinen
linux-net is users list, use netdev instead on matters relating development...
Bad case of typo :)
Also you could have had a bit less exhaustive cc list, IMHO :-).
Quoted 4 lines
Quoted 2 lines
Please make sure that it's based on net-next tree next time since there are lots of changes already in the areas you're touching.
We will do that for the updated patch set.
Thanks, that will greatly help review when I don't have to apply into some very oldish kernel version (-p style patch would have helped to show the context better).
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
This will also give us time to integrate any ideas that may arise from the discussions here.
We are happy for all feedback regarding this:
Is something like this viable to introduce into the kernel?
Quoted 6 lines
Quoted 3 lines
No idea in general. But it has to be (nearly) minimal and clean if such thing is ever to be considered. The one below is definately not even close minimal in many areas...
We will clean up the noise, redo the formatting and divide it into logical segments for the new patch set.
I hope you get more feedback on viability then, my comments were mainly on technical side.
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
Is the scheme for thin-stream detection mechanism acceptable.
Does reduncancy happen in the initial slow start as well (depends on write pattern)? Why it is so in case the stream is to become thick right after initial rtts?
This is probably partially my misunderstanding as well, I just assumed tcp_stream_is_thin() is used everywhere without reading too carefully all those (a && b || (c || (!e && d))) mess.
But in general the problem is still there, ie., if even if some magic would claim that stream is "thin" we cannot apply that to the streams which, after the initial slow-start, will no longer match to that magic. Otherwise stating that we only apply this and this to thin streams won't be valid.
Quoted 2 lines
If the window is halved, (but still not at less than 4 segment), the mechanisms will be turned off (due to the < 4 packets in flight limit).
I'm sorry, this makes not sense to me...
Quoted 5 lines
If the stream backs off to a minimal window, the mechanisms will be turned on.
The bundling mechanism (if activated) will stay active since it relies on the packet sizes and interarrival time, not on number of packets in flight.
Likewise, maybe you're talking of some other version of the patch since I cannot find such parts that do what you describe (or the patch is just too messy :-)).
Quoted 16 lines
Quoted 5 lines
...In general this function should be split, having a separate skb (non-TCP) function(s) and tcp-side would be ideal.
It's misnamed as well, it won't merge but duplicates?
Quoted 10 lines
Also, this approach is extremely intrusive and adding non-linear seqno things into write queue will require _you_ to do
_fullaudit_over every single place to verify that seqno leaps backwards won't break anything (and you'll still probably miss some cases). I wonder if you realize how easily this kind of change manifests itself as a silent data corruption on stream level and have taken appropriate actions to validate that not a single one of scenario leads to data coming as different out as was sent in (every single byte, it's not enough to declare that application worked which could well happen with corrupted data too). TCP is very coreish and such bugs will definately hit people hard.
Quoted 6 lines
We have to admit we don't quite see the problem. Since a page can never be removed before all owners have decleared that they no longer needs it, all data will be correctly present. Also, since the packets are placed linearly on the queue and we don't support when SACK is enabled, no gaps will occur. But maybe we have misunderstood your comment, please let us know if that is the case.
Yes, the problems _may_ arise because you create afaict:skb->end_seq > next_skb->seq situations which is something that certainly needs an audit over every single seqno operation to see that they don't implicitly assume otherwise (ie., omit redundant compares)! There are certainly places I know of already which will break... I'm afraid that using the approach you've select you'll end up having very many places needing some extra tweaking, that's why I suggested the alternative approach to just construct the redundant things on-the-fly while keeping the write queue as is.
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
+
if(TCP_SKB_CB(skb)->seq <= (TCP_SKB_CB(prev_skb)->end_seq - ua_data)){So
ua_datamust beprev_skb->lenor this fails? Whyua_dataneeds such complex setup above?
Quoted 8 lines
Quoted 2 lines
Does this thing of youes depend on skb being not cloned?
We will look into making the calculation
ua_dataeasier and also the cloning requirement.The check is there to avoid duplicate data in the case when a previous packet has been bundled with later ones.
Ah, this is the already bundled check... Then it would be simpler to do
just: if (before(TCP_SKB_CB(skb)->seq, TCP_SKB_CB(prev_skb)->end_seq)), wouldn't it?
Quoted 3 lines
However, when we think about it, this might not be neccessary since bundling on retransmission is not allowed to bundle with packets that are yet to be sent. Thank you for pointing this out.
I guess you must still have some way to protect against duplication on in tcp_trans_merge_prev() on successive send calls.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
+
+if(skb_shinfo(skb)->frag_list || skb_shinfo(skb)->frag_list){If it wasn't cloned, why you do this check?
Good question, oversight by us, thank you for pointing it out.
Funny, it seems to be even buggy for the original purpose (if that ever was valid) as it is not (x || y) but (x || x) :-D.
Quoted 11 lines
Quoted 10 lines
I suppose this function was effectively:
{
tcp_write_queue_walk_safe(...){
...checks...
if (tcp_trim_head(...))
break;tcp_collapse_retrans(...);
}
}
Quoted 2 lines
Correct, except we dont collapse but bundle, will be modified and simplified in the next version of the patch.
Ah, I misunderstood this part, so my next comment is not valid:
Quoted 9 lines
Quoted 4 lines
It's sort of counter-intuitive that first you use redundancy but now in here when the path
_hasshownproblems_you now remove the redundancy if I understand you correctly? The logic is...?Even though the path has problems (as shown by the need for retransmission), we still bundle as many packets as possible. If you are still not sure, please let us know what part of the code that is confusing.
...see above.
Though there is not another question, why send them as small at all here but instead just combining two together and sending twice??? I somewhat can understand why including from the previous skb is necessary at the first time sending since the previous is already out but in here we don't have such obstacle.
Quoted 15 lines
Quoted 10 lines
It seems very intrusive solution in general. I doubt you succeed in pulling it off as is without breaking something. To me it seems rather fragile approach to make write queue seqno backleaps you're proposing. It also leads to troubles in the truesize as you have noticed. Why not just building those redundancy containing segments at the write time in case the stream is thin, then all other parts would not have to bother about dealing these things? Number of sysctls should be minimized, if they're to be added at all. Skb work functions should be separated from tcp layer things.
Quoted 4 lines
If you depend on non-changing sysctl value to select right branch, you're asking for trouble as the userspave is allowed to change it during the flow as well and even during the ack processing.
Quoted 6 lines
As far as we understand this comment, you want us to do it on the application layer instead? Do you mean as a middleware, application-specific solution or something similar? Also, we believe doing it on the application layer will lead to the same delays that we try to prevent, since sent data will be delayed on the transport layer in case of loss.
No but at the time we're supposed to actually send an skb to the lower layer and keeping the rest intact.
Signature · 1 line
i.
Hi Ilpo,
Quoted 25 lines
Quoted 6 lines
We have to admit we don't quite see the problem. Since a page can never be removed before all owners have decleared that they no longer needs it, all data will be correctly present. Also, since the packets are placed linearly on the queue and we don't support when SACK is enabled, no gaps will occur. But maybe we have misunderstood your comment, please let us know if that is the case.
Yes, the problems _may_ arise because you create afaict:
skb->end_seq>next_skb->seqsituations which is something that certainly needs an audit over every single seqno operation to see that they don't implicitly assume otherwise (ie., omit redundant compares)! There are certainly places I know of already which will break... I'm afraid that using the approach you've select you'll end up having very many places needing some extra tweaking, that's why I suggested the alternative approach to just construct the redundant things on-the-fly while keeping the write queue as is.Quoted 6 lines
As far as we understand this comment, you want us to do it on the application layer instead? Do you mean as a middleware, application-specific solution or something similar? Also, we believe doing it on the application layer will lead to the same delays that we try to prevent, since sent data will be delayed on the transport layer in case of loss.
Quoted 2 lines
No but at the time we're supposed to actually send an skb to the lower layer and keeping the rest intact.
I have a small question regarding these two comments. Are you allowed to modify the packet data stored in a clone? Based on my understanding of the code and what a clone is, I thought the data was shared between the original and the clone. Thus, any data appended/inserted into the clone will also be appended to the original.
If I have understood the code correctly, what will then be the difference between our current solution and the one you suggest (except we can remove one of the bundling methods and when a packet is retransmitted)? If I have not understood the code correctly, feel free to yell :) (if it is a misunderstanding, it also explains all the checks for skb->cloned).
-Kristian
Trimmed all but netdev (and .no addresses) from cc list.
Quoted 25 lines · On Fri, 16 Jan 2009, [email protected] wrote:
Quoted 25 lines
Quoted 6 lines
We have to admit we don't quite see the problem. Since a page can never be removed before all owners have decleared that they no longer needs it, all data will be correctly present. Also, since the packets are placed linearly on the queue and we don't support when SACK is enabled, no gaps will occur. But maybe we have misunderstood your comment, please let us know if that is the case.
Yes, the problems _may_ arise because you create afaict:
skb->end_seq>next_skb->seqsituations which is something that certainly needs an audit over every single seqno operation to see that they don't implicitly assume otherwise (ie., omit redundant compares)! There are certainly places I know of already which will break... I'm afraid that using the approach you've select you'll end up having very many places needing some extra tweaking, that's why I suggested the alternative approach to just construct the redundant things on-the-fly while keeping the write queue as is.Quoted 6 lines
As far as we understand this comment, you want us to do it on the application layer instead? Do you mean as a middleware, application-specific solution or something similar? Also, we believe doing it on the application layer will lead to the same delays that we try to prevent, since sent data will be delayed on the transport layer in case of loss.
Quoted 8 lines
Quoted 2 lines
No but at the time we're supposed to actually send an skb to the lower layer and keeping the rest intact.
I have a small question regarding these two comments. Are you allowed to modify the packet data stored in a clone? Based on my understanding of the code and what a clone is, I thought the data was shared between the original and the clone. Thus, any data appended/inserted into the clone will also be appended to the original.
To avoid corruption you are not allowed to change data for cloned skbs. ...I'm not fully sure how this is related though.
Quoted 5 lines
If I have understood the code correctly, what will then be the difference between our current solution and the one you suggest (except we can remove one of the bundling methods and when a packet is retransmitted)? If I have not understood the code correctly, feel free to yell :) (if it is a misunderstanding, it also explains all the checks for
skb->cloned).
It didn't mean clone an skb but copy the relevant data into new skb which is then not put into write queue at all but given to lower layers only.
Signature · 1 line
i.
Hello,
Quoted 1 line
Trimmed all but netdev (and .no addresses) from cc list.
Thank you.
Quoted 11 lines
Quoted 8 lines
If I have understood the code correctly, what will then be the difference
between our current solution and the one you suggest (except we can remove
one of the bundling methods and when a packet is retransmitted)? If I have
not understood the code correctly, feel free to yell :) (if it is a misunderstanding, it also explains all the checks forskb->cloned).It didn't mean clone an skb but copy the relevant data into new skb which is then not put into write queue at all but given to lower layers only.
Thank you, now I understand what you meant and I agree that it is a better solution. However, when I think of it, copying might be too resource intensive and thus remove all gains from RDB. We have seen streams with small packets and very low interarrival times, which would lead to a large number of copy-operations every second. I will implement it and compare performance.
-Kristian
Quoted 12 lines · On Wed, 21 Jan 2009, [email protected] wrote:
Quoted 11 lines
Quoted 8 lines
If I have understood the code correctly, what will then be the difference
between our current solution and the one you suggest (except we can remove
one of the bundling methods and when a packet is retransmitted)? If I have
not understood the code correctly, feel free to yell :) (if it is a misunderstanding, it also explains all the checks forskb->cloned).It didn't mean clone an skb but copy the relevant data into new skb which is then not put into write queue at all but given to lower layers only.
Quoted 6 lines
Thank you, now I understand what you meant and I agree that it is a better solution. However, when I think of it, copying might be too resource intensive and thus remove all gains from RDB. We have seen streams with small packets and very low interarrival times, which would lead to a large number of copy-operations every second. I will implement it and compare performance.
The latencies caused by network related problems are hardly in the same order of magnitude than processing latencies due to simple alloc + copy of <= mss sized content, and if data is in frags you won't be copying even that much. For every avoided retransmission (with the associated rtt latency) you can copy a lot of data.
Ah, one more thing I already forgot earlier... Your solution is lacking means to deal with ambiguity problem which is a serious problem since RTT measurements are valid only if there was not ambiguity.
Signature · 1 line
i.