Page MenuHomeFreeBSD
Differential D30036 Diff 88572 sys/netinet/cc/cc_newreno.c

Changeset View

Standalone View

View Options

sys/netinet/cc/cc_newreno.c

Context not available.
static void newreno_post_recovery(struct cc_var *ccv); static void newreno_post_recovery(struct cc_var *ccv);
static int newreno_ctl_output(struct cc_var *ccv, struct sockopt *sopt, void *buf); static int newreno_ctl_output(struct cc_var *ccv, struct sockopt *sopt, void *buf);
VNET_DEFINE_STATIC(uint32_t, newreno_beta) = 50; VNET_DEFINE(uint32_t, newreno_beta) = 50;
VNET_DEFINE_STATIC(uint32_t, newreno_beta_ecn) = 80; VNET_DEFINE(uint32_t, newreno_beta_ecn) = 80;
#define V_newreno_beta VNET(newreno_beta) #define V_newreno_beta VNET(newreno_beta)
#define V_newreno_beta_ecn VNET(newreno_beta_ecn) #define V_newreno_beta_ecn VNET(newreno_beta_ecn)
Context not available.
.ctl_output = newreno_ctl_output, .ctl_output = newreno_ctl_output,
}; };
struct newreno {
uint32_t beta;
uint32_t beta_ecn;
};
static inline struct newreno * static inline struct newreno *
newreno_malloc(struct cc_var *ccv) newreno_malloc(struct cc_var *ccv)
{ {
Context not available.
* XXXLAS: Find a way to signal SS after RTO that * XXXLAS: Find a way to signal SS after RTO that
* doesn't rely on tcpcb vars. * doesn't rely on tcpcb vars.
*/ */
uint16_t abc_val;
if (ccv->flags & CCF_USE_LOCAL_ABC)
abc_val = ccv->labc;
else
abc_val = V_tcp_abc_l_var;
if (CCV(ccv, snd_nxt) == CCV(ccv, snd_max)) if (CCV(ccv, snd_nxt) == CCV(ccv, snd_max))
incr = min(ccv->bytes_this_ack, incr = min(ccv->bytes_this_ack,
ccv->nsegs * V_tcp_abc_l_var * ccv->nsegs * abc_val *
CCV(ccv, t_maxseg)); CCV(ccv, t_maxseg));
else else
incr = min(ccv->bytes_this_ack, CCV(ccv, t_maxseg)); incr = min(ccv->bytes_this_ack, CCV(ccv, t_maxseg));
rscheffUnsubmitted
Not Done
Inline Actions

Why did you remove this RFC2861 related bound of ssthresh?

rscheff: Why did you remove this RFC2861 related bound of ssthresh?
rrsAuthorUnsubmitted
Done
Inline Actions

The standard does not call for this. The standard calls for
ssthresh to be infinity at start. Yes you can set it to less, but I find
this to be incorrect and it does lead to issues we have seen
in production (low throughput due to getting into CA too soon).

rrs: The standard does not call for this. The standard calls for ssthresh to be infinity at start.
rscheffUnsubmitted
Not Done
Inline Actions

Don't quite understand this. This is the after-idle function (thus has no bearing with the initial ssthresh on connection establishment. Also, since this is mid-session, ssthresh should NOT be infinitiy here).

RFC2861 suggests to set ssthresh to the maximum of the former ssthresh, or 3/4 of the former cwnd. So taking this out may leave ssthresh at a smaller, former value - but never restrict it as you suggest.

If you have observations, that increasing ssthresh in after-idle is counter-productive (e.g. running into extensive burst loss, RTO and Lost Retransmissions) that may be a valuable to know @ IETF.

rscheff: Don't quite understand this. This is the after-idle function (thus has no bearing with the…
rrsAuthorUnsubmitted
Done
Inline Actions

Ahh I thought you were pointing out the initialization code. Actually I never
changed this.. you must remember this is what is in the NF tree. I would
have to look historically at where or what zapped those lines.. I will restore
them. It could have been merge damage or??

We would never have noticed this since there is a bug in the way being idle
is calculated in FreeBSD.. and basically it has never worked for servers only
for clients.. I may have fixed that in head can't remember.. but because that
has been the way the NF stack as ran for years I think we keep the option
to do idle down removed :)

rrs: Ahh I thought you were pointing out the initialization code. Actually I never changed this..
rscheffUnsubmitted
Not Done
Inline Actions

The Idle detection for both directions got addressed with D25016, after you hinted me on this :)

rscheff: The Idle detection for both directions got addressed with D25016, after you hinted me on this :)
Context not available.
u_int mss; u_int mss;
cwin = CCV(ccv, snd_cwnd); cwin = CCV(ccv, snd_cwnd);
mss = tcp_maxseg(ccv->ccvc.tcp); mss = CCV(ccv, t_maxseg);
rscheffUnsubmitted
Not Done
Inline Actions

This will miscalculate the mss for variable-length options (e.g. bidir traffic with SACK loss recovery in both directions going on, or when TSopt is used).

rscheff: This will miscalculate the mss for variable-length options (e.g. bidir traffic with SACK loss…
rrsAuthorUnsubmitted
Done
Inline Actions

I wonder if this is a sync-update we missed. Since I only had work on the beta stuff in here and a couple odds and ends. I
do like using tcp_maxseg(), but even at that it will only deal with constant options i.e. in the case of
a loss it will not allow bytes for that.. which as it should be.

rrs: I wonder if this is a sync-update we missed. Since I only had work on the beta stuff in here…
rrsAuthorUnsubmitted
Done
Inline Actions

Actually I think the idea here is to have a mss to set the cwnd to. And in this
case you should be using probably just the fixed option sizes.. not including
if a sack is pending. Reason is that:

a) We don't count options in the cwnd/rwnd ... i.e. sendwin = max(cwnd, rwnd)... and we

compare that to (snd_max - snd_una).. that never has any options space in it.

b) We want to drive the sending of whole mss pieces. We don't want to have

left over 14 bytes or some such.

Yes an initial send if sack is on and has data would be smaller than a mss. But
the cwnd is being set in theory for other sends. If you account for whats in the
current sack blocks then you are shorting yourself and causing small sends in
the network.

Really what is needed here is not tcp_maxseg() but what is in ctf_fixed_maxseg().
What I probably need to do here is promote that into tcp_subr.c to tcp_fixed_maxseg()
and then use that.

rrs: Actually I think the idea here is to have a mss to set the cwnd to. And in this case you…
nreno = ccv->cc_data; nreno = ccv->cc_data;
beta = (nreno == NULL) ? V_newreno_beta : nreno->beta; beta = (nreno == NULL) ? V_newreno_beta : nreno->beta;
beta_ecn = (nreno == NULL) ? V_newreno_beta_ecn : nreno->beta_ecn; beta_ecn = (nreno == NULL) ? V_newreno_beta_ecn : nreno->beta_ecn;
if (V_cc_do_abe && type == CC_ECN)
/*
* Note that we only change the backoff for ECN if the
* global sysctl V_cc_do_abe is set <or> the stack itself
* has set a flag in our newreno_flags (due to pacing) telling
* us to use the lower valued back-off.
*/
if (V_cc_do_abe ||
(nreno && (nreno->newreno_flags & CC_NEWRENO_BETA_ECN) && (type == CC_ECN)))
lstewartUnsubmitted
Not Done
Inline Actions

I noticed this during some unrelated code review: I think the flags test should be (nreno->newreno_flags & CC_NEWRENO_BETA_ECN_ENABLED).

lstewart: I noticed this during some unrelated code review: I think the flags test should be `(nreno…
factor = beta_ecn; factor = beta_ecn;
else else
factor = beta; factor = beta;
Context not available.
V_cc_do_abe && V_cc_abe_frlossreduce)) { V_cc_do_abe && V_cc_abe_frlossreduce)) {
CCV(ccv, snd_ssthresh) = CCV(ccv, snd_ssthresh) =
((uint64_t)CCV(ccv, snd_ssthresh) * ((uint64_t)CCV(ccv, snd_ssthresh) *
(uint64_t)beta) / (uint64_t)beta) / (uint64_t)beta_ecn;
(100ULL * (uint64_t)beta_ecn);
} }
if (!IN_CONGRECOVERY(CCV(ccv, t_flags))) if (!IN_CONGRECOVERY(CCV(ccv, t_flags)))
CCV(ccv, snd_ssthresh) = cwin; CCV(ccv, snd_ssthresh) = cwin;
rscheffUnsubmitted
Not Done
Inline Actions

So, newreno no longer adjusts ssthresh and cwnd on RTO? That doesn't sound right...

rscheff: So, newreno no longer adjusts ssthresh and cwnd on RTO? That doesn't sound right...
rrsAuthorUnsubmitted
Not Done
Inline Actions

Hmm I did not take this out.. but I see it somehow is not
in our tree.. This must be some sort of merge damage.. going
to have to see where in the NF tree this came from...

rrs: Hmm I did not take this out.. but I see it somehow is not in our tree.. This must be some sort…
rrsAuthorUnsubmitted
Done
Inline Actions

Ok I see whats going on here. Looks like rack does these sets
with a bunch of other stuff. So somehow in our tree
the case got removed, when it should not. Since we
don't use anything but rack or BBR it was never noticed :-)

I will fix it in the next patch update with the rest of the fixes

rrs: Ok I see whats going on here. Looks like rack does these sets with a bunch of other stuff. So…
Context not available.
nreno->beta = opt->val; nreno->beta = opt->val;
break; break;
case CC_NEWRENO_BETA_ECN: case CC_NEWRENO_BETA_ECN:
if (!V_cc_do_abe) if ((!V_cc_do_abe) && ((nreno->newreno_flags & CC_NEWRENO_BETA_ECN) == 0))
lstewartUnsubmitted
Not Done
Inline Actions

Ditto

lstewart: Ditto
return (EACCES); return (EACCES);
nreno->beta_ecn = opt->val; nreno->beta_ecn = opt->val;
break; break;
Context not available.