On 03/12, Roland McGrath wrote:
>
> 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);
Agreed.
> Does that need a barrier pair here and in
No, set_notify_resume()->test_and_set_tsk_thread_flag() implies mb(),
> 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.
smp_mb__after_clear_bit() is enough, but in that case perhaps it is better
to modify the arch dependent do_notify_resume().
A couple of minor nits, but please remember I often misread the comments.
> 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) {
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). Also, whatever reason we have to keep
->utrace_flags != 0, the same reason applies to ->utrace_flags |= XXX in
utrace_add_engine().
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.
Looks a bit confusing to me. Unless exit_notify() calls utrace_report_death()
we don't change ->utrace_flags.
* This ensures that utrace_release_task knows positively that
* utrace_report_death will not run later.
*/
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.
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.
Oleg.
