Messages in this thread | | | From | Roland McGrath <> | Subject | Re: [RFC,PATCH 14/14] utrace core | Date | Sun, 13 Dec 2009 12:48:47 -0800 (PST) |
| |
> All that seems to do is call ->release() and kmem_cache_free()s the > utrace_engine thing, why can't that be done with utrace->lock held?
Calling ->release with a lock held is clearly insane, sorry. It is true that any engine-writer who does anything like utrace_* calls inside their release callback is doing things the wrong way. But guaranteeing that simple mistakes result in spin-lock deadlocks just seems clearly wrong to me. A main point of the utrace API is to make it easier to work in this space and help you avoid writing the pathological bugs. Adding picayune gotchas like this just does not help anyone. No other API callback is made holding some internal implementation lock, and making this one the sole exception seems just obviously ill-advised on its face. I can't really imagine what a justification for such an obfuscation would be.
> But yeah, passing that list along does seem like a better solution.
So you find it cleaner to have each caller of utrace_reset take another output parameter and be followed with the same exact source code duplicated in each call site, than to have utrace_reset() do the unlock and then the common code itself. I guess there is no accounting for taste.
We try not to get excited about trivia, so on matters like this one we will do whatever the consensus of gate-keeping reviewers wants. My patch to implement your suggestion adds 13 lines of source and 134 bytes of compiled text (x86-64). Is that what you prefer?
I'll note that the code as it stands uses the __releases annotation for sparse, as well as thoroughly documenting the locking details in comments. I gather that clear explanation of the code is in your eyes no excuse for ever writing code that requires one to actually read the comments. I'm not sure that attitude can ever be satisfied by any code that is nontrivial or makes any attempts at optimization.
Thanks, Roland
| |