Before 04852f35ff95f2a48cf8c38faf365b7d61f4370a we had invariant:
->utrace_flags != 0 if and only if we have attached engines, now
we don't because utrace_add_engine() does not set REAP.
I believe this invariant is very nice, and it is better to keep it.
Note also that if utrace_add_engine() does not set REAP, then we
should also change utrace_reset() which preserves this bit.
I think this change is dangerous. For example, I sent
"[PATCH 4/4] utrace_release_task: check REAP before utrace_reap()"
https://www.redhat.com/archives/utrace-devel/2009-September/msg00049.html
before I reviewed this commit. Again, I am not sure this patch is
really needed, but it looks like an obviously correct optimization.
Now it is not.
Or, suppose we decide to optimize tracehook_force_sigpending(). It
does 2 checks why only one is needed, it could be just
static inline int tracehook_force_sigpending(void)
{
return utrace_interrupt_pending();
}
Now we can't do this, because
engine = utrace_attach_task(task);
utrace_control(task, engine, UTRACE_INTERRUPT);
causes lockup.
Another example. utrace_control() does the sanity check before
it proceeds. No! now it is not a sanity check, it is needed for
correctness. Otherwise
engine = utrace_attach_task(task);
utrace_control(task, engine, UTRACE_REPORT);
Now, do_notify_resume() calls tracehook_notify_resume() which
does nothing because task_utrace_flags() == 0 and clears
TIF_NOTIFY_RESUME. But we didn't clear utrace->report! This means
we can never set TIF_NOTIFY_RESUME again, we check !->report first.
Heh, and given that utrace_control checks "action >= UTRACE_REPORT",
this commit is slightly wrong. Because utrace_control(INTERRUPT) after
attach sets ->interrupt + TIF_SIGPENDING, utrace_get_signal() won't be
called, recalc_sigpending() clears TIF_SIGPENDING and we have the
dangling ->interrupt set.
Please revert this change ;)
Signed-off-by: Oleg Nesterov <o...@redhat.com>
---
kernel/utrace.c | 4 ++++
1 file changed, 4 insertions(+)
--- __UTRACE/kernel/utrace.c~5_ADD_MUST_SET_REAP 2009-09-06
16:44:07.000000000 +0200
+++ __UTRACE/kernel/utrace.c 2009-09-06 18:07:14.000000000 +0200
@@ -164,7 +164,11 @@ static int utrace_add_engine(struct task
* callback, that would mean the new engine also gets
* notified about the event that precipitated its own
* creation. This is not what the user wants.
+ *
+ * In case we had no engines before, make sure that
+ * utrace_flags is not zero.
*/
+ target->utrace_flags |= UTRACE_EVENT(REAP);
list_add_tail(&engine->entry, &utrace->attaching);
utrace->slow_path = 1;
ret = 0;
