[Differential] D5872: tcp: Don't prematurely drop receiving-only connections
lstewart added a comment. In https://reviews.freebsd.org/D5872#130806, @sepherosa_gmail.com wrote: > In https://reviews.freebsd.org/D5872#130805, @lstewart wrote: > > > In https://reviews.freebsd.org/D5872#130179, @sepherosa_gmail.com wrote: > > > > > We probably can leave the cwnd resetting to later rexmt timeout or possible later fast retransmit (I think fast retransmit could kick in under some cases, if ENOBUFS happened); instead of resetting the cwnd immediately upon ENOBUFS. > > > > > > Please leave the manipulation of cwnd as is so as to avoid conflating two different changes. The manipulation of cwnd on local drop has nothing to do with the subject of this particular change. > > > Yep, I am not going to delete the cwnd reset in this patch. errr, not sure if we're on the same page or not, but I am in strong agreement with Mike. Please leave the line "tp->snd_cwnd = tp->t_maxseg;" where it is and untouched i.e. don't delete or change it. >> Also, the patch we're reviewing here should be the commit candidate i.e. what will actually land in the tree. I understand that you need to test with something different than the commit candidate, but that should just be noted in the review description. We all need to see the final code as there are many subtleties here that require close attention from as many sets of eyes as possible. > > This is what I am testing now. Since this path is not on a hot code path and we obviously don't want to have timers unset for data/FIN/SYN, can we just use the "if() panic" here? No. This isn't a condition we should bring the whole machine down for. A rate limited log message would be appropriate so the admin knows there's a potential for hung connections, but I don't insist on this. > And another thing is abstract it as a macro. Maybe we just leave the macro abstraction to the next step? I don't see any good reason not to define and use the macro as part of this patch. It can then be used in follow up commits to check the other return points from the input and output processing paths. REVISION DETAIL https://reviews.freebsd.org/D5872 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, glebius, adrian, delphij, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, freebsd-net-list, lstewart, hiren, jtl, transport Cc: gnn, mike-karels.net, jtl ___ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
[Differential] D5872: tcp: Don't prematurely drop receiving-only connections
lstewart added a comment. In https://reviews.freebsd.org/D5872#130179, @sepherosa_gmail.com wrote: > We probably can leave the cwnd resetting to later rexmt timeout or possible later fast retransmit (I think fast retransmit could kick in under some cases, if ENOBUFS happened); instead of resetting the cwnd immediately upon ENOBUFS. Please leave the manipulation of cwnd as is so as to avoid conflating two different changes. The manipulation of cwnd on local drop has nothing to do with the subject of this particular change. Also, the patch we're reviewing here should be the commit candidate i.e. what will actually land in the tree. I understand that you need to test with something different than the commit candidate, but that should just be noted in the review description. We all need to see the final code as there are many subtleties here that require close attention from as many sets of eyes as possible. REVISION DETAIL https://reviews.freebsd.org/D5872 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, glebius, adrian, delphij, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, freebsd-net-list, lstewart, hiren, jtl, transport Cc: gnn, mike-karels.net, jtl ___ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
[Differential] D5872: tcp: Don't prematurely drop receiving-only connections
lstewart added a comment. In https://reviews.freebsd.org/D5872#128556, @hiren wrote: > In https://reviews.freebsd.org/D5872#128555, @lstewart wrote: > > > I thought that had been fixed ages ago... oops. > > > Fixed? i.e. doing something other than setting cwnd to 1 seg? Yes, but turns out it was a discussion I had privately with a colleague who never got around to creating the patch we discussed. >> It should be calling cc_cong_signal() with a new congestion type. > > Hum... tcp_quench() used to be there which essentially had this 1 line to set cwnd to 1 seg. > > Is there any (RFC) guidance for what to do in this situation? No, and it's an implementation detail that RFCs have no real business being concerned with either. Setting cwnd==maxseg is completely inappropriate though and I would argue that whatever reasoning was used to justify the original choice is as wrong today as it was back then. At any rate, that's something to follow up separately. REVISION DETAIL https://reviews.freebsd.org/D5872 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, glebius, adrian, delphij, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, freebsd-net-list, transport, jtl, hiren, lstewart Cc: gnn, mike-karels.net, jtl ___ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
[Differential] D5872: tcp: Don't prematurely drop receiving-only connections
lstewart added a comment. I thought that had been fixed ages ago... oops. It should be calling cc_cong_signal() with a new congestion type. Just leave that line as is for the moment though as Mike says. REVISION DETAIL https://reviews.freebsd.org/D5872 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, glebius, adrian, delphij, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, freebsd-net-list, transport, jtl, hiren, lstewart Cc: gnn, mike-karels.net, jtl ___ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
[Differential] D5872: tcp: Don't prematurely drop receiving-only connections
lstewart added a comment. ... but replace with a macro to check that the rexmit/persist timer is armed if appropriate! REVISION DETAIL https://reviews.freebsd.org/D5872 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, glebius, adrian, delphij, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, freebsd-net-list, transport, jtl, hiren, lstewart Cc: gnn, mike-karels.net, jtl ___ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
[Differential] [Updated] D5872: tcp: Don't prematurely drop receiving-only connections
lstewart added a comment. I agree with Mike's proposal (although FYI, I do belive tcp_output() will send an ACK on RTO). TCP ACKs are intentionally unreliable by design and setting the retransmit timer there is nonsense - either there is a bug elsewhere which needs to be fixed, or it is trying to paper over local ACK loss in a dubious manner. The ENOBUFS case should also become a thing of the past when the back pressure work goes in any way. For the immediate change, perhaps replacing with a macro that expands to a KASSERT to double check the appropriate conditions for the retransmit or persist timers being set would be a good idea. The macro should be used elsewhere in tcp_output() and tcp_intput() as well but that can be done in follow up commit(s). REVISION DETAIL https://reviews.freebsd.org/D5872 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, glebius, adrian, delphij, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, freebsd-net-list, transport, jtl, hiren, lstewart Cc: mike-karels.net, jtl ___ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
[Differential] [Commented On] D1761: Extend LRO support to accumulate more than 65535 bytes
lstewart added a comment. Ok, but that's anecdotal and gives us reviewers nothing to go on - without any methodology or raw data who knows whether the LRO change is solely responsible for the improvement and if it introduced any undesired side effects. It's also possible that with tuning, the same results could have been obtained without the jumbo LRO change. As there seems to be some sensitivity around sharing specific details from field deployments which is fine, the path forward is therefore for you and/or Mellanox test engineers to run experiments, capture + analyse data and present it for discussion. You should provide your methodology so anyone wanting to replicate your experiments and results can do so. That being said, I personally feel the energy would be better spent on batching, which would allow a tunable number of 64k correctly formed packets to be passed up the stack which should give 99% of the benefits of this work without the hackiness, plus gives us a win in many other workloads when LRO is unavailable or not used. REPOSITORY rS FreeBSD src repository REVISION DETAIL https://reviews.freebsd.org/D1761 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: hselasky, rrs, glebius, gnn, emaste, rwatson, bz, imp, np, jfv, adrian, lstewart Cc: imp, freebsd-net-list ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to freebsd-net-unsubscr...@freebsd.org
[Differential] [Commented On] D1761: Extend LRO support to accumulate more than 65535 bytes
lstewart added a comment. I hope I didn't delete it... from what I could see online, the Abandon Phabricator action is the means by which a reviewer indicates they have permanently rejected the patch (as opposed to suggesting changes). As to people using the patch, can you say who and why? REPOSITORY rS FreeBSD src repository REVISION DETAIL https://reviews.freebsd.org/D1761 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: hselasky, rrs, glebius, gnn, emaste, rwatson, bz, imp, np, jfv, adrian, lstewart Cc: imp, freebsd-net-list ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to freebsd-net-unsubscr...@freebsd.org
[Differential] [Abandoned] D1761: Extend LRO support to accumulate more than 65535 bytes
lstewart abandoned this revision. lstewart added a comment. Hans, Just because some hardware is capable of coalescing more than 64k of data doesn't mean we should feel obligated to support the functionality. I'd be curious to understand the anticipated use cases that led to hardware support being added. Without some compelling data to show that this is useful, I think this work should be put on ice until such time as it can be shown to be worthwhile. If such data exists, I'm willing to give it due consideration and revise my judgment, but at this stage I strongly suspect there is no workload we support or will support in the near future that would significantly benefit from raising the LRO chunk size above 64k vs the hacks required to make it work, so that's why I'm voting against this patch outright rather than suggesting changes. The real goal is to remove LRO entirely anyway, which I believe we have ideas on how to do e.g. packet batching techniques. As an aside, it would be useful to socialise ideas like this a bit more along with good data before investing the time and energy into doing the work unless it's trivial enough that it doesn't matter. Ideally we should have had this discussion on the mailing list centered around proposed use case(s) and a data set showing the limitations of the 64k limit on those use cases before the patch was proposed. REPOSITORY rS FreeBSD src repository REVISION DETAIL https://reviews.freebsd.org/D1761 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: hselasky, rrs, glebius, gnn, emaste, rwatson, bz, imp, np, jfv, adrian, lstewart Cc: imp, freebsd-net-list ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to freebsd-net-unsubscr...@freebsd.org
