I think Roland is right. Let's forget about utrace for the moment,
this code looks like
if (!CONDITION) {
set_task_state(TASK_XXX);
schedule();
}
and it can obviously race with
CONDITION = true;
wake_up_xxx();
This is Linux in-kernel synchronization 101.
The correct blocks of this sort do:
set_current_state(TASK_UNINTERRUPTIBLE);
while (!CONDITION) {
schedule();
}
set_current_state(TASK_RUNNING);
This is entirely orthogonal to anything having to do with utrace per se.
This pattern race-free because wake_up_* will reset the waiter to
TASK_RUNNING after making CONDITION true. So in the race where the
while (!CONDITION) clause lets it get into schedule(), that just
returns immediately, after which the second iteration sees CONDITION true.
This is why I recommended using higher-level things like linux/wait.h
facilities, where the correct usage patterns are more obvious.
But the race we're actually talking about here is on the other side of
the coin.
I am wondering if there is another way to handle such a race...
Suppose we
- add UTRACE_STOP_XXX
- modify utrace_control(UTRACE_STOP_XXX) to do
mark_engine_wants_stop() and nothing more.
utrace_control(current,,UTRACE_STOP) doesn't really do anything more.
So a callback doing this closes the old race: UTRACE_STOP is in force
when the tracer side wakes up, since utrace_control-on-current was done
before wake_up_tracer(). But...
- finish_callback_report(UTRACE_STOP_XXX) does
spin_lock(utrace-lock);
if (engine_wants_stop(engine))
action = UTRACE_STOP;
else
action = UTRACE_REUME;
spin_unlock(utrace-lock);
Let's clarify what this means for the people following the utrace API
but not its implementation internals: currently, the callback's return
value overrides the engine's last-set state such as UTRACE_STOP, whether
that came from a previous utrace_control before the callback, a
utrace_control call inside the callback, or from the return value of a
previous callback.
So now we've moved to a different race (or a different instance of the
original race, if you like): after wake_up_tracer(), the tracer can come
and clear our engine's UTRACE_STOP state with its utrace_control call;
but then our callback return value just reestablishes UTRACE_STOP and we
still stop, having lost the asynchronous resumption from the tracer.
So Oleg has an idea for a new kind of callback return value, which I'd call
perhaps UTRACE_NOP. This would mean to stay in UTRACE_STOP if the last
utrace_control call on this engine used UTRACE_STOP, or the last callback
return value left the engine in that state, otherwise UTRACE_RESUME.
Then, probably producer could do
utrace_control(current, UTRACE_STOP_XXX);
slot = book_message_in_buffer(buf);
if (slot 0)
return UTRACE_STOP_XXX;
...
return UTRACE_RESUME;
Even better, UTRACE_STOP_XXX can live outside of UTRACE_RESUME_MASK.
Right, it doesn't belong in enum utrace_resume_action--it doesn't make
sense as an argument for utrace_control, for example. So it could be
something like a return-value flag bit saying stop if marked for stop,
e.g. UTRACE_SINGLESTEP|UTRACE_STICKY_STOP for single-step, unless marked
to stop, in which case stay stopped.
So this would fix that race. For the scenario we're discussing here,
that's probably just fine. The only reason to stop was for the consumer
to run. If it woke up quickly and ran enough to call utrace_control
while we were still finishing the return of the callback and utrace
bookkeeping, then we just don't stop at all. (But note if there is
another engine that decides to stop anyway, then we'll have used a
second CPU and/or ping-pong scheduled to run the tracer thread in
parallel with a very short amount of work in the tracee before it just
blocks anyway.)
Now let's consider another scenario. Take the normal debugger scenario,
i.e. a ptrace implementation done cleanly on the utrace API without the
magic kludges we have today, or a ptrace replacement built cleanly from
scratch in the ideal way, or the pending gdbstub implementation. Here
what the callback does is wake up the tracer, and return UTRACE_STOP.
What the tracer thread does is block in some fashion until the tracee's
wake_up_tracer(), then do whatever logic (in ptrace, that means return
to user mode and let it make another ptrace system call, but that all
might be quite fast), and decide to examine the thread. To make sure
its own callback is finished and UTRACE_STOP will be in force, it has to
synchronize with utrace_barrier. So in maximum parallelism, this means,
having preempted the tracee just now by waking up, we now yield back to
the tracee to finish our callback, other engines' callbacks, and utrace
bookkeeping.
