Thirumalai Srinivasan writes: > * In each of ill_trace_ref(), ipif_trace_ref(), ire_trace_ref(), and > nce_trace_ref(), if the call to th_trace_gethash() fails, we set the > appropriate xxx_trace_disable flag. Can we also call the appropriate > cleanup > func like xxx_trace_cleanup() to release any memory being held.
Good catch; thanks. I did that right for the other cases, but not for that one. I'll add those calls. > * ip_if.h comments need updates. Eg. L492/493 L502/503 refer to non-existent > functions ill/ipif/ire_thread_exit. Also L475/476 should be deleted. Oops. Yes, those should instead refer to the callbacks from the destructors. > * ips_thread_lock could be a reader writer lock. The xxx_trace_cleanup would > then grab it as reader while th_trace_gethash and ip_thread_exit would > grab > it as writer. Unlike interfaces, ire creation/deletion activity could > be more > frequent and with a single ips_thread_lock this may be helpful. OK; that makes sense. I originally made it a mutex just for simplicity, but the fact that xxx_trace_cleanup needs to walk the active thread entries means that it ought to be concurrent. I'll make that change. > * One subtle point is that a th_hash_t which in turn maps to a thread can be > present only in one ips_thread_list or a single IP stack. Correct. > Mostly a thread > is tied to a single IP stack via its zone, but if it walks netstacks then > it may hold references to ipif/ill across netstacks, but its per thread > th_hash_t is present in just one IP stack, typically the IP stack > corresponding to the 1st reference it made to any ipif/ill/ire/nce. > > This means that if any ipif/ill/ire/nce in some other stack was freed, the > the xxx_trace_cleanup verification will miss the above thread (th_hash_t) > and any references by the above thread. > > I don't think we need to add more complexity just to cover this edge case, > since all this is about just some extra debugging help and nothing more. > But a comment explaining the above may be helpful. Actually, that case makes me nervous, so I'm going to fix it. We're still just talking about debug support, and about basic system scalability, so there's no particular reason to have the ips_thread_lock and ips_thread_list be per-stack. A simpler answer here is to create a global ip_thread_rwlock and ip_thread_list, and have the *_cleanup function walk that list. On a system with a lot of shared-stack zones, it works out to be the same. On a system with a lot of exclusive-stack zones, we'll end up walking through threads in other zones, and thus take longer to do the work, but we'll always end up doing the right thing. Many thanks for the comments! (I've also gotten back comments from Dan McDonald. I'll update, retest, and publish a new webrev before heading to RTI.) -- James Carlson, Solaris Networking <[EMAIL PROTECTED]> Sun Microsystems / 1 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677 _______________________________________________ networking-discuss mailing list [email protected]
