On Wed, Apr 4, 2018 at 6:42 AM Ilpo Järvinen <ilpo.jarvi...@helsinki.fi> wrote:
> On Wed, 28 Mar 2018, Yuchung Cheng wrote: ... > > Your patch looks good. Some questions / comments: The patch looks good to me as well (I read the Mar 28 version). Thanks for this fix, Ilpo. > Just to be sure that we understand each other the correct way around this > time, I guess it resolved both of your "disagree" points above? > > 1. Why only apply to CA_Loss and not also CA_Recovery? this may > > mitigate GRO issue I noted in other thread. > Hmm, that's indeed a good idea. I'll give it some more thought but my > initial impression is that it should work. Great. I would agree with Yuchung's suggestion to apply this fix to CA_Recovery as well. > > 2. minor style nit: can we adjust tp->delivered directly in > > tcp_clean_rtx_queue(). I am personally against calling "non-sack" reno > > (e.g. tcp_*reno*()) because its really confusing w/ Reno congestion > > control when SACK is used. One day I'd like to rename all these *reno* > > to _nonsack_. > That's what I actually did first but put ended up putting it into own > function because of line lengths (not a particularly good reason). > ...So yes, I can put it into the tcp_clean_rtx_queue in the next version. > I'll also try to keep that "reno" thing in my mind. Either approach here sounds fine to me. Though personally I find it more readable to keep all the non-SACK helpers together and consistently named, including this one (even if the name is confusing, at least it is consistent... :-). neal
