[Differential] D5872: tcp: Don't prematurely drop receiving-only connections

[sepherosa_gmail.com (Sepherosa Ziehau)]

sepherosa_gmail.com added a comment.


  In https://reviews.freebsd.org/D5872#130813, @lstewart wrote:
  
  > 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.
  
  
  Done.

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, transport, jtl
Cc: gnn, mike-karels.net, jtl