> 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.
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.
Since commit ac6e19c (my equivalent to your 3/4), utrace_barrier will no
longer return -ESRCH while your report_reap callback might be running.
I think its doing so before was not really ideal, because it still gave
no positive indication that the callback *couldn't* still be running,
which is what a caller of utrace_barrier is really there for.
It seems best to me that utrace_barrier block while the engine's
report_reap callback could be running, just like for all other
callbacks. Then a 0 return means unequivocally that no callback is in
progress, and so does an -ESRCH return.
So maybe we want this? I'm not really sure about the barriers, maybe we
don't need both or any at all. My thinking about that is just to keep
uniform across all callbacks what kind of guarantees the caller of
utrace_barrier has about the ordering of any effects its report_reap
callback can have. Other callbacks always have an smp_mb() between
setting ->reporting and calling the engine's code.
Thanks,
Roland
diff --git a/kernel/utrace.c b/kernel/utrace.c
index 6036764..3bec3be 100644
--- a/kernel/utrace.c
+++ b/kernel/utrace.c
@@ -426,8 +426,19 @@ static void utrace_reap(struct task_stru
spin_unlock(&utrace->lock);
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;
+ }
engine->ops = NULL;
engine->flags = 0;
