> 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).
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.
--
meem
