> utrace_set_events() checks the "setting _UTRACE_DEATH_EVENTS" case twice, > and it is not immediately obvious why the first check is needed, and why > it is not racy (we are checking exit_state without tasklist). The code is > correct, but looks confusing.
More comments are always good. > In short: this double check allows to avoid tasklist when utrace_flags > already has these bits while engine->flags doesn't. > > But multitracing is unlikely case, in the likely case if we add > _UTRACE_DEATH_EVENTS to engine->flags we set these bits in utrace_flags > too. I think it makes sense to consolidate these checks to make the > code a bit more understandable. I don't really agree about "unlikely case". In many uses, the systemtap task-finder will have a utrace engine on every task in the system, for example. Moreover, this is an importantly distinct particular kind of micro-optimization to be doing here: avoiding a system-wide lock. Any place that we take tasklist_lock at all, we are introducing a system-wide slowdown or limitation on scaling, just because of our tracing of one task. So optimizing the minimize that is qualitatively different and really much more important than avoiding taking the utrace lock, for example. But with that note in your mind, I am happy to take this patch now as a simplification and let reoptimization come back later. Thanks, Roland
