Re: Q: utrace_reset() UTRACE_EVENT(REAP)
On 03/18, Roland McGrath wrote:
Yes, but the other side lacks a barrier. UTRACE_REPORT does
utrace-report = 1;
wmb(); // actually mb, but wmb is enough
set _TIF_NOTIFY_RESUME;
do_notify_resume()-utrace_resume()-start_report() path does
if (_TIF_NOTIFY_RESUME)
// !!! we need rmb in between !!!
if (utrace-report)
...
and it can miss -report.
I see. We have a similar problem for (the first) attach, too, right?
utrace_add_engine does:
Yes sure. I just meant the barrier was needed even before you changed
utrace_add_engine() to set -report.
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -616,6 +616,12 @@ static inline void set_notify_resume(struct task_struct
*task)
static inline void tracehook_notify_resume(struct pt_regs *regs)
{
struct task_struct *task = current;
+ /*
+ * This pairs with the barrier implicit in set_notify_resume().
+ * It ensures that we read the nonzero utrace_flags set before
+ * set_notify_resume() was called by utrace setup.
+ */
+ smp_rmb();
smp_mb__after_clear_bit() is enough, but I agree, smp_rmb() is more
understandable.
Oleg.
Re: Q: utrace_reset() UTRACE_EVENT(REAP)
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.
Re: Q: utrace_reset() UTRACE_EVENT(REAP)
I'm afraid I wasn't clear again, On 03/12, Oleg Nesterov wrote: Oh. utrace_attach_task()-utrace_add_engine() sets -report + TIF_NOTIFY_RESUME. But tracehook_notify_resume() does nothing because -utrace_flags == 0 ? Confused. 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(). And again, utrace_control(UTRACE_STOP) does not set -utrace_flags != 0 itself. But even if we called utrace_set_events(XXX) before, without set_notify_resume() we have to wait for that XXX event, this doesn't look right. Oleg.
Re: Q: utrace_reset() UTRACE_EVENT(REAP)
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
