Q: utrace_reset() && UTRACE_EVENT(REAP)
Roland McGrath
roland at redhat.com
Thu Mar 12 23:16:07 UTC 2009
> 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
More information about the utrace-devel
mailing list