Jim, Looks good. Using thread specific data handles the problem neatly.
* 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. * 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. * 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. * 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. 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. Thirumalai James Carlson wrote: >I'd like review comments from Sasha and Thiru (on the To: line above) >if possible. In any event, comments from any interested person are >solicited. It'd be nice to get comments by a week from now (24 August >2007), and I'll wait that long, but when possible will be fine. > >These are changes for these two CRs: > > 6203568 accumulating number of threads behind rw_lock in > ire_walk_ill_tables IRB_REFRELE > 6591083 IP instances sis_check added stray entries to sparc ip > module makefile > >I have webrevs and testing information available both on >opensolaris.org: > > http://cr.opensolaris.org/~carlsonj/webrev-6203568/ > http://cr.opensolaris.org/~carlsonj/testing-6203568/ > >and on the SWAN: > > http://zhadum.east/ws/carlsonj/6203568-fix/webrev/ > http://zhadum.east/ws/carlsonj/6203568-fix/testing/ > >For testing, the "summary.txt" file provides the key information. >(I'd have called it "README," but it seems that the opensolaris.org >web server summarily ignores any files matching that name. Go >figure.) > >For those on the SWAN, I've got a workspace available with cscope >built in $SRC and $SRC/uts. Feel free to browse it: > > /net/zhadum.east/export/ws/carlsonj/6203568-fix/ > > > _______________________________________________ networking-discuss mailing list [email protected]
