On 09/07, Roland McGrath wrote: > > > But more importantly, I think utrace_reap() never needs to synchronize > > with utrace_barrier(). Once we drop utrace->lock, any other caller which > > takes this lock must see both ->exit_state and utrace->reap. This means > > utrace_control() or utrace_set_events() which sets/clears REAP must not > > succeed. > > Right. So this doesn't matter for the primary original purpose of > utrace_barrier, i.e.: > > * A successful return from utrace_barrier() guarantees its ordering > * with respect to utrace_set_events() and utrace_control() calls.
Yes. > But IMHO it would be nice (and the least surprising thing) if this > statement were really true too: > > * A return value of zero means no callback from @target to @engine was > * in progress. I was going to write that this doesn't make real sense, but now I think you are right. For example, something_t *ptr; void my_report_reap(...) { if (ptr) dereference(ptr); } ... void *tmp = ptr; ptr = NULL; utrace_barrier(my_engine); kfree(tmp); With or without recent changes, this doesn't work. > I'm not really sure about the barriers, Me too. In fact, recently I realized I don't really understand the barriers around ->reporting, need to read this code with a fresh head. In particular, I'd like to see 1. Can't we set ->reporting _after_ checking engine->flags ? (unlikely) 2. Is utrace_barrier() correct???? Note the example above, it assumes that utrace_barrier() itself is the barrier wrt reporting. But, is it? > list_for_each_entry_safe(engine, next, &utrace->attached, entry) { > - if (engine->flags & UTRACE_EVENT(REAP)) > + if (engine->flags & UTRACE_EVENT(REAP)) { > + /* > + * These barriers order anything ->report_reap() > + * does to be while ->reporting is set. That way > + * any utrace_barrier() call that returns 0 is > + * guaranteeing that the callback is complete. > + */ > + utrace->reporting = engine; > + smp_mb(); > engine->ops->report_reap(engine, target); > + smp_mb(); > + utrace->reporting = NULL; Agreed. And yes, now it is important to clear ->ops _after_ calling ->report_reap(), otherwise utrace_barrier() can take the lock. Oleg.