Messages in this thread | | | Subject | Re: [PATCH] CKRM: 8/10 CKRM: Resource controller for prioritizing inbound network requests | From | Dave Hansen <> | Date | Mon, 29 Nov 2004 12:28:58 -0800 |
| |
You sure do like C++ comments, don't you :)
On Mon, 2004-11-29 at 10:51, Gerrit Huizenga wrote: > +#ifdef CONFIG_ACCEPT_QUEUES > +#define TCP_ACCEPTQ_SHARE 13 /* Set accept queue share */ > +#endif
Why does a #define need an #ifdef? It's not like there's object size bloat from it.
> +#ifdef CONFIG_ACCEPT_QUEUES > + > +#define NUM_ACCEPT_QUEUES 8 /* Must be power of 2 */ > + > +struct tcp_acceptq_info { > + unsigned char acceptq_shares; > + unsigned long acceptq_wait_time; > + unsigned int acceptq_qcount; > + unsigned int acceptq_count; > +}; > +#endif
Same here. Are you just trying to make sure that uses of those variables don't happen outside of your other #ifdef'd code?
> +struct tcp_listen_opt > +{ > + u8 max_qlen_log; /* log_2 of maximal queued SYNs */ > + int qlen; > +#ifdef CONFIG_ACCEPT_QUEUES > + int qlen_young[NUM_ACCEPT_QUEUES]; > +#else > + int qlen_young; > +#endif > + int clock_hand; > + u32 hash_rnd; > + struct open_request *syn_table[TCP_SYNQ_HSIZE]; > +};
Icky. How about just making NUM_ACCEPT_QUEUES=1 when your option is off? Should all compile down to the same thing.
> + if (prev_class < 0) { > + req->dl_next = tp->accept_queue; > + tp->accept_queue = req; > + } > + else { > + req->dl_next = tp->acceptq[prev_class].aq_tail->dl_next; > + tp->acceptq[prev_class].aq_tail->dl_next = req; > + } > + }
Documentation/CodingStyle. else and brakets on one line, please.
> +static void *laq_res_alloc(struct ckrm_core_class *core, > + struct ckrm_core_class *parent) > +{ ... > + res = kmalloc(sizeof(ckrm_laq_res_t), GFP_ATOMIC); > + if (res) { > + memset(res, 0, sizeof(res)); > + spin_lock_init(&res->reslock); > + laq_res_hold(res); > + res->my_depth = pdepth; > + if (pdepth == 2) // listen class > + res->my_id = 0; > + else if (pdepth == 3) > + res->my_id = atoi(laq_get_name(core)); > + res->core = core; > + res->pcore = parent; > + > + // rescls in place, now initialize contents other than > + // hierarchy pointers > + laq_res_initcls(res); // acts as initialising value > + } > + > + return res; > +}
Generally, these look nicer if you do this:
res = kmalloc() if (!res) return res; ... stuff from old if() here ... -- Dave
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |