> James Carlson writes:
> > (I've also gotten back comments from Dan McDonald. I'll update,
> > retest, and publish a new webrev before heading to RTI.)
>
> I've updated and retested everything. The updated webrev and testing
> information are here:
>
> http://cr.opensolaris.org/~carlsonj/testing-6203568/
> http://cr.opensolaris.org/~carlsonj/webrev-6203568/
In general, looks good. A few comments on common/inet/ip/ip_if.c:
* The *_untrace_ref() functions seem to assume that the thread
doing the untrace will already be known to the tracing
framework (or we'll panic). I don't see what guarantees this.
* The *_trace_ref() functions ASSERT() that mod_hash_insert()
cannot fail, but I don't see what makes that a safe assertion.
Seems like this will panic under sufficient memory pressure.
* Speaking of the *_trace_ref() functions: seems like a lot of
the code could be shared -- e.g., we could have a common
th_trace_ref() function such as:
boolean_t
th_trace_ref(void *obj, ip_stack_t *ipst)
{
th_trace_t *th_trace;
mod_hash_t *mh;
mod_hash_val_t val;
if ((mh = th_trace_gethash(ipst)) == NULL)
return (B_FALSE);
/*
* Attempt to locate the trace buffer for this obj and thread.
* If it does not exist, then allocate a new trace buffer and
* insert into the hash.
*/
if (mod_hash_find(mh, obj, &val) == MH_ERR_NOTFOUND) {
th_trace = kmem_zalloc(sizeof (th_trace_t), KM_NOSLEEP);
if (th_trace == NULL)
return (B_FALSE);
th_trace->th_id = curthread;
if (mod_hash_insert(mh, obj, (mod_hash_val_t)th_trace)
!= 0) {
kmem_free(th_trace, sizeof (th_trace_t));
return (B_FALSE);
}
} else {
th_trace = (th_trace_t *)val;
}
ASSERT(th_trace->th_refcnt >= 0 &&
th_trace->th_refcnt < TR_BUF_MAX - 1);
th_trace->th_refcnt++;
th_trace_rrecord(th_trace);
return (B_TRUE);
}
... at which point, the various <obj>_trace_ref() functions can
become trivial, like:
void
ill_trace_ref(ill_t *ill)
{
ASSERT(MUTEX_HELD(&ill->ill_lock));
if (!ill->ill_trace_disable)
return;
if (!th_trace_ref(ill, ill->ill_ipst)) {
ill->ill_trace_disable = B_TRUE;
ill_trace_cleanup(ill);
}
}
Similar simplifications are possible for the *_trace_unref()
and *_trace_cleanup() functions, of course.
* I'm not sure why this:
#ifdef DEBUG
ipif_trace_cleanup(ipif);
#endif
... is seen as preferable to:
IPIF_TRACE_CLEANUP(ipif)
The latter seems less obtrusive.
--
meem
_______________________________________________
networking-discuss mailing list
[email protected]
