> > Another follow-up.
> >
> > I tried both approaches out (your suggestion and Meem's timer suggestion),
> > and fount that we get the best performance when combine these two
> together,
> > and the timer has to be set to 1 second. For whoever is interested, here
> is
> > the webrev:
> >
> > http://jurassic.eng/net/aquila.prc/export/home/cathy/clearview/webrev_tcp/
>
> I took a quick look and it seems like what I expected. BTW, AFAIK,
> timeout(9F) won't actually return 0 on failure, so I don't think that code
> is reachable. On a related note, I've always disliked `do_lock' arguments
> -- among other things, it tends to lead to multiple locking orders, and
> eventually deadlock. It also doesn't appear to be necessary in your code
> (e.g., you can always have it grab the lock with some minor code changes).
>
I am still in the process of code optimization so that I am not expecting a
code review yet. But sure, I'll have a look of your suggestions and see
which one applies.
Thanks
- Cathy
> Some other nits:
>
> * I'd prefer ds_tx_qdepth_tid to ds_qdepth_timer. (In particular,
> it should have the "tx_" prefix like the other related fields.)
>
> * 1353: Appears to be an unrelated strcmp() change.
>
> * dld_qdepth() loop seems a bit odd -- why does it calculate the
> first iteration in the initialization to the `for' loop? Would
> seem more natural as:
>
> for (mp = dsp->ds_tx_list_head, mp != NULL; mp = mp->b_next) {
> size = get_mpsize(mp);
> cnt += size;
> msgcnt++;
> if (cnt >= dld_max_q_count || msgcnt > dld_max_q_count) {
> ASSERT(dsp->ds_tx_qbusy);
> dsp->ds_tx_list_tail = mp;
> freemsgchain(mp->b_next);
> mp->b_next = NULL;
> cnt -= size;
> msgcnt--;
> break;
> }
> }
>
> ... or (perhaps slower, but simpler):
>
> for (mp = dsp->ds_tx_list_head, mp != NULL; mp = mp->b_next) {
> size = get_mpsize(mp);
> if (cnt + size > dld_max_q_count ||
> msgcnt == dld_max_q_count) {
> ASSERT(dsp->ds_tx_qbusy);
> dsp->ds_tx_list_tail = mp;
> freemsgchain(mp->b_next);
> mp->b_next = NULL;
> break;
> }
> cnt += size;
> msgcnt++;
> }
>
> Either would also eliminate the need for the check at line 2215.
>
> * Lots of spelling errors need to be corrected (especially
> s/caculate/calculate), and a bunch of wording needs to be
> fixed. Most of that can be worked as part of the bigger
> Clearview code review, though.
>
> * s/inline static/static inline/ for consistency with other
> uses of inline in our source.
>
> * get_mpsize() is a strange function name -- doesn't seem to
> follow any of the existing conventions -- at least mp_getsize()
> would match mmd_getsize().
>
> * 2176-2188: Kinda ugly code. How about something like:
>
> switch (DB_TYPE(mp)) {
> case M_DATA:
> return (mp->b_cont == NULL) ? MBLKL(mp) : msgdsize(mp));
> #ifdef HWIPSEC
> case M_CTL:
> mp = mp->b_cont;
> ASSERT(mp != NULL && DB_TYPE(mp) == M_DATA);
> return (mp->b_cont == NULL) ? MBLKL(mp) : msgdsize(mp));
> #endif
>
> * 2246: Needless cast.
>
