> > + *
> > + * 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);
>
> Agreed.
I put that in.
> > Does that need a barrier pair here and in
>
> No, set_notify_resume()->test_and_set_tsk_thread_flag() implies mb(),
Ah, ok.
> > tracehook_notify_resume()?
>
> Ah. I think you are right, and I think it needs the barrier even without
> this change. Say, UTRACE_REPORT does:
>
> utrace->report = 1;
> set_notify_resume();
>
> Without mb() there is no guarantee that utrace_resume() will notice and
> clear ->report.
Wait, what? You just said that set_notify_resume() already implies an mb().
> smp_mb__after_clear_bit() is enough, but in that case perhaps it is better
> to modify the arch dependent do_notify_resume().
I don't follow this. But we don't want a solution that requires changing
arch code. Why can't tracehook_notify_resume() do whatever is required?
> > + *
> > + * 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) {
>
> This looks misleading, utrace_release_task() is called unconditionally, and
> we could use any unused bit afacis (REAP only makes sense for engine->flags,
> we never check ->utrace_flags & REAP).
It's true that any bit at all would do, but REAP is one that makes some
sense logically and also one that is implicitly reserved in utrace_flags
already (without having to reserve another one from engine.flags).
It's true that utrace_release_task() is called unconditionally now, but it
might not always be so. It seems like a very intuitive and useful
invariant that utrace_flags==0 means "utrace totally empty".
It's unconditional now because the previous code tested the indirect
pointer rather than flags (for reasons we can no longer be very sure of).
If we can convince ourselves about the interlocks, then it would be better
to have it test utrace_flags and not call into utrace.c for the common case
(nor take the utrace lock).
> Also, whatever reason we have to keep ->utrace_flags != 0, the same
> reason applies to ->utrace_flags |= XXX in utrace_add_engine().
Hence the change we agreed to above.
> utrace_reset() also does
>
> if (task->exit_state) {
> flags &= DEAD_FLAGS_MASK;
>
> The comment about DEAD_FLAGS_MASK
>
> /*
> * Only these flags matter any more for a dead task (exit_state set).
> * We use this mask on flags installed in ->utrace_flags after
> * exit_notify (and possibly utrace_report_death) has run.
I think these macros are from when reap did a quiesce callback in a
previous incarnation of the API. It doesn't make much sense to use
the macro for just UTRACE_EVENT(REAP) now.
> Looks a bit confusing to me. Unless exit_notify() calls utrace_report_death()
> we don't change ->utrace_flags.
If it doesn't call utrace_report_death(), that means DEATH_EVENTS were not
in ->utrace_flags.
> Yes. But this means we could do "flags &= ~DEATH_EVENTS" instead. This is
> subjective of course, but looks more clean to me.
>
> Note also that utrace_reset() is the only user of DEAD_FLAGS_MASK and
> LIVE_FLAGS_MASK has no users.
I got rid of those macros and replaced the comment with this:
if (task->exit_state) {
+ /*
+ * Once it's already dead, we never install any flags
+ * except REAP. When ->exit_state is set and events
+ * like DEATH are not set, then they never can be set.
+ * This ensures that utrace_release_task() knows
+ * positively that utrace_report_death() can never run.
+ */
BUG_ON(utrace->death);
- flags &= DEAD_FLAGS_MASK;
+ flags &= UTRACE_EVENT(REAP);
wake = false;
I think it makes sense to use this mask because what we are specifically
concerned with here is that utrace_release_task() is the one and only
utrace entry point that the task might take hereafter.
> Also, it would be better imho to change tracehook_report_death() to use
> DEATH_EVENTS too, it is always good when grep can find the usage.
I made _UTRACE_DEATH_EVENTS that common macro.
Thanks,
Roland
