Page MenuHomeFreeBSD
Differential D15358 Diff 42284 sys/netinet/cc/cc_newreno.c

Changeset View

Standalone View

View Options

sys/netinet/cc/cc_newreno.c

Show First 20 Lines • Show All 76 Lines • ▼ Show 20 Lines
#include <netinet/cc/cc_newreno.h> #include <netinet/cc/cc_newreno.h>
static MALLOC_DEFINE(M_NEWRENO, "newreno data", static MALLOC_DEFINE(M_NEWRENO, "newreno data",
"newreno beta values"); "newreno beta values");
#define CAST_PTR_INT(X) (*((int*)(X))) #define CAST_PTR_INT(X) (*((int*)(X)))
static int newreno_cb_init(struct cc_var *ccv); static int newreno_cb_init(struct cc_var *ccv);
static void newreno_cb_destroy(struct cc_var *ccv);
static void newreno_ack_received(struct cc_var *ccv, uint16_t type); static void newreno_ack_received(struct cc_var *ccv, uint16_t type);
static void newreno_after_idle(struct cc_var *ccv); static void newreno_after_idle(struct cc_var *ccv);
static void newreno_cong_signal(struct cc_var *ccv, uint32_t type); static void newreno_cong_signal(struct cc_var *ccv, uint32_t type);
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);
static VNET_DEFINE(uint32_t, newreno_beta) = 50; static VNET_DEFINE(uint32_t, newreno_beta) = 50;
static VNET_DEFINE(uint32_t, newreno_beta_ecn) = 80; static 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)
struct cc_algo newreno_cc_algo = { struct cc_algo newreno_cc_algo = {
.name = "newreno", .name = "newreno",
.cb_init = newreno_cb_init, .cb_init = newreno_cb_init,
.cb_destroy = newreno_cb_destroy,
.ack_received = newreno_ack_received, .ack_received = newreno_ack_received,
.after_idle = newreno_after_idle, .after_idle = newreno_after_idle,
.cong_signal = newreno_cong_signal, .cong_signal = newreno_cong_signal,
.post_recovery = newreno_post_recovery, .post_recovery = newreno_post_recovery,
.ctl_output = newreno_ctl_output, .ctl_output = newreno_ctl_output,
}; };
struct newreno { struct newreno {
uint32_t beta; uint32_t beta;
uint32_t beta_ecn; uint32_t beta_ecn;
}; };
int int
newreno_cb_init(struct cc_var *ccv) newreno_cb_init(struct cc_var *ccv)
{ {
struct newreno *nreno; struct newreno *nreno;
nreno = malloc(sizeof(struct newreno), M_NEWRENO, M_NOWAIT|M_ZERO); nreno = malloc(sizeof(struct newreno), M_NEWRENO, M_NOWAIT|M_ZERO);
if (nreno != NULL) { if (nreno != NULL) {
nreno->beta = V_newreno_beta; nreno->beta = V_newreno_beta;
nreno->beta_ecn = V_newreno_beta_ecn; nreno->beta_ecn = V_newreno_beta_ecn;
ccv->cc_data = nreno;
jhbUnsubmitted
Not Done
Inline Actions

This seems to be an important bug fix. Several places in this file assume ccv->cc_data is not NULL. newreno_cong_signal() checks it for NULL but then later assumes it is not NULL for the CC_NDUPACK case. newreno_ctl_output() doesn't even check but just assumes non-NULL. It seems like this should probably fail with ENOMEM if the memory allocation fails?

jhb: This seems to be an important bug fix. Several places in this file assume ccv->cc_data is not…
jtlUnsubmitted
Not Done
Inline Actions

Yeah, this is clearly a good catch. We're allocating memory and not doing anything with it, so clearly that's wrong.

I had initially wondered if cc_data ccv->cc_data pointed to bogus memory; however, it should be initialized to NULL by tcp_newtcpcb(), which would cause a page fault if accessed. So, it must just be that no one is actually using the ABE features that this structure supports.

My opinion is that the features which cc_data supports in the newreno module are "optional" (as evidenced by the fact that no one has noticed they don't work). So, we should probably let the connection succeed and simply fix the places that are brokenly relying on cc_data being non-NULL.

In newreno_cong_signal() , you could use the default beta/beta_ecn values if cc_data is NULL.

In newreno_ctl_output(), you could try to allocate and initialize cc_data, if it is NULL. If that fails, return ENOBUFS.

Actually, an even better change may be to simply do the above two changes, but then delay allocating cc_data until someone tries to change the defaults. That would avoid unnecessary allocations for a feature which evidently is not widely used. (Otherwise, it seems like we should be seeing many more kernel panics.)

In any case, very good catch @thj!

jtl: Yeah, this is clearly a good catch. We're allocating memory and not doing anything with it, so…
jhbUnsubmitted
Not Done
Inline Actions

Yes, I think this is too new to say that it is not widely used. However, if cc_newreno is the default, I do see some value in it not failing in cb_init for a malloc(). I think your suggestions of deferring the allocation to the setsockopt() callback and using the defaults as a fallback if it is NULL seems sensible. Arguably if you do that then the 'ABE' check should always be there even if newreno is NULL and use the global ecn value for CC_ECN?

jhb: Yes, I think this is too new to say that it is not widely used. However, if cc_newreno is the…
} }
return (0); return (0);
} }
void
newreno_cb_destroy(struct cc_var *ccv)
{
if (ccv->cc_data != NULL)
free(ccv->cc_data, M_NEWRENO);
}
static void static void
newreno_ack_received(struct cc_var *ccv, uint16_t type) newreno_ack_received(struct cc_var *ccv, uint16_t type)
{ {
if (type == CC_ACK && !IN_RECOVERY(CCV(ccv, t_flags)) && if (type == CC_ACK && !IN_RECOVERY(CCV(ccv, t_flags)) &&
(ccv->flags & CCF_CWND_LIMITED)) { (ccv->flags & CCF_CWND_LIMITED)) {
u_int cw = CCV(ccv, snd_cwnd); u_int cw = CCV(ccv, snd_cwnd);
u_int incr = CCV(ccv, t_maxseg); u_int incr = CCV(ccv, t_maxseg);
▲ Show 20 Lines • Show All 180 Lines • ▼ Show 20 Lines if (sopt->sopt_valsize != sizeof(struct cc_newreno_opts))
return (EMSGSIZE); return (EMSGSIZE);
nreno = ccv->cc_data; nreno = ccv->cc_data;
opt = buf; opt = buf;
switch (sopt->sopt_dir) { switch (sopt->sopt_dir) {
case SOPT_SET: case SOPT_SET:
switch (opt->name) { switch (opt->name) {
case CC_NEWRENO_BETA: case CC_NEWRENO_BETA:
jhbUnsubmitted
Not Done
Inline Actions

I do think it would be simpler to rename 'newreno_condmalloc' to 'newreno_malloc' and remove the conditional logic from it (so it just always calls malloc) and call it here if newreno is NULL, so something like:

nreno = ccv->cc_data;
opt = buf;
switch (sopt->sopt_dir) {
case SOPT_SET:
    /* We cannot set without cc_data memory. */
    if (nreno == NULL) {
        nreno = newreno_malloc(ccv);
        if (nreno == NULL)
            return (ENOMEM);
    }
    ...
jhb: I do think it would be simpler to rename 'newreno_condmalloc' to 'newreno_malloc' and remove…
lstewartAuthorUnsubmitted
Not Done
Inline Actions

Not sure I agree your suggestion is "simpler"... just different, and it increases nesting which I have a mild dislike of. I was anticipating potential future behaviour whereby other code paths might also want to conditionally allocate memory for on-demand feature activation. That being said, no such thing exists currently, so I'm open to adopting your suggestion, but unconvinced "simpler" is a good argument for doing so :)

Are there other arguments/opinions on the matter? I'd like to commit this in the next 24h.

lstewart: Not sure I agree your suggestion is "simpler"... just different, and it increases nesting which…
rgrimesUnsubmitted
Not Done
Inline Actions

To me it would appear the overall code would be easier to understand if the change from the conditional logic was removed from the function and the function renamed. It would remove nesting from inside that code, which would migrate to here though. But it would remove the testing of sopt->sopt_dir == SOPT_SET twice, once in the call to newreno_condmalloc and once in the switch statement on sopt->sopt_dir

rgrimes: To me it would appear the overall code would be easier to understand if the change from the…
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)
return (EACCES); return (EACCES);
nreno->beta_ecn = opt->val; nreno->beta_ecn = opt->val;
break; break;
default: default:
▲ Show 20 Lines • Show All 48 Lines • Show Last 20 Lines