Re: Q: utrace_reset() && UTRACE_EVENT(REAP)

[Roland McGrath]

> Hmm. But this leads to another question: why does utrace_reset() set
> UTRACE_EVENT(REAP) ?
> 
> This looks as: make sure ->utrace_flags is never 0 unless we detach
> all engines. Perhaps because sometimes, say tracehook_notify_resume(),
> we just check task_utrace_flags() != 0 ?

Right, it's an invariant that utrace_flags != 0 if there is any utrace
stuff to do.  It just fits logically too.  The utrace_flags bits mean "need
to call into utrace", so UTRACE_EVENT(REAP) means that we need to call
utrace_release_task.

> Imho, this needs a comment. Or I missed something obvious.

Sure, better comments are always good.  How's this?

@@ -899,6 +899,10 @@ static void utrace_reset(struct task_struct *task, struct 
utrace *utrace,
         * of the interests of the remaining tracing engines.
         * For any engine marked detached, remove it from the list.
         * We'll collect them on the detached list.
+        *
+        * Any engine that's not detached implies tracking the REAP event,
+        * whether or not that engine wants a report_reap callback.  Any
+        * engine requires attention from utrace_release_task().
         */
        list_for_each_entry_safe(engine, next, &utrace->attached, entry) {
                if (engine->ops == &utrace_detached_ops) {

> Oh. utrace_attach_task()->utrace_add_engine() sets ->report + 
> TIF_NOTIFY_RESUME.
> But tracehook_notify_resume() does nothing because ->utrace_flags == 0 ?

The logic (in the utrace_add_engine comment) is to have ->report just to
make sure splice_attaching() precedes the next reporting pass (start_report).
It doesn't actually care about TIF_NOTIFY_RESUME (i.e. how soon the report
happens), but just wants to keep the invariant that ->report matches
TIF_NOTIFY_RESUME.  But as you point out, this invariant will be violated
later if tracehook_notify_resume() sees ->utrace_flags == 0.

> Perhaps this is not problem per se. But let's suppose we call, say,
> utrace_control(UTRACE_STOP) later. utrace_do_stop() sees ->report == 1
> and doesn't call set_notify_resume(). But TIF_NOTIFY_RESUME was already
> cleared by do_notify_resume().

Right.

So I think we need this:

@@ -181,7 +181,13 @@ static int utrace_add_engine(struct task_struct *target,
                 * also set.  Otherwise utrace_control() or utrace_do_stop()
                 * might skip setting TIF_NOTIFY_RESUME upon seeing ->report
                 * already set, and we'd miss a necessary callback.
+                *
+                * In case we had no engines before, make sure that
+                * utrace_flags is not zero when tracehook_notify_resume()
+                * checks.  That would bypass utrace reporting clearing
+                * TIF_NOTIFY_RESUME, and thus violate the same invariant.
                 */
+               target->utrace_flags |= UTRACE_EVENT(REAP);
                list_add_tail(&engine->entry, &utrace->attaching);
                utrace->report = 1;
                set_notify_resume(target);

Does that need a barrier pair here and in tracehook_notify_resume()?


Thanks,
Roland