Re: [PATCH 2/2] UTRACE_STOP: nesting engine management (updated)

[Roland McGrath]

 When a report function of an engine returns UTRACE_STOP, it means (may mean)
 that it wants to change the status of the process before resuming it.
 VM monitors often change the status, sometimes debugger users want to set
 some variables too.

Yes.  In ideal cases, it can decide up front quickly what it wants to do,
and change the user state right in the callback without stopping.  But when
it needs another agent to decide what to do, it uses UTRACE_STOP.

 IMHO, utrace should stop it *before* calling the report function of the 
 next engine, 

No, we'll never want to do it this way.  One engine doesn't get to
arbitrarily delay the reporting to other engines of the thread's events.
This is both an efficiency point and a robustness point.  It's important to
remember that utrace is about the primitive events: the user thread had an
event ... the user thread is about to run again.  The high-level notion of
what did the other engine do? is built from examining the state at these
events, and knowing about the delays that other engines are imposing via
UTRACE_STOP.

 otherwise we need to set up another structure to synchronize
 the engines (that may even be unknown one to the other).
 If there is a tracer/debugger among the engines, it is not even possible to 
 know
 which snapshot it gets, after or before the modification created by the VM
 monitor?

This is where the broader discussion of callback order comes in.

When a previous engine has decided to use UTRACE_STOP, your callback's
@action argument reflects this.  You know that another engine is going to
do something asynchronous before it lets the user thread run.  If your own
engine doesn't especially want it stopped now but wants to see what it
looks like when other engines are done fiddling with it, then you can use
UTRACE_REPORT.  That ensures that you'll get a report_quiesce callback
after those other engines have done their thing.


Thanks,
Roland

Re: [PATCH 2/2] UTRACE_STOP: nesting engine management (updated)

[Oleg Nesterov]

On 03/12, Renzo Davoli wrote:

 I have update also the second patch. Please note that now this patch
 must be applied after the first one.
 This patch implements a consistent nesting model for utrace machines.
 (There is a full description in the messages I sent on Feb. 14 and Mar. 6)

This patch does 2 completely different things. I think you should
make separate patches.

Again, we need Roland's opinion, but could you explain why it would
be better to use _reverse in utrace_report_syscall_entry() ?

As for another change,

 --- linux-2.6.29-rc7-git5-utrace-p1/kernel/utrace.c   2009-03-12 
 11:05:50.0 +0100
 +++ linux-2.6.29-rc7-git5-utrace-p2/kernel/utrace.c   2009-03-12 
 13:37:27.0 +0100
 @@ -1405,6 +1405,7 @@
  static bool finish_callback(struct utrace *utrace,
   struct utrace_report *report,
   struct utrace_engine *engine,
 + struct task_struct *task,
   u32 ret)
  {
   enum utrace_resume_action action = utrace_resume_action(ret);
 @@ -1426,6 +1427,7 @@
   spin_lock(utrace-lock);
   mark_engine_wants_stop(engine);
   spin_unlock(utrace-lock);
 + utrace_stop(task, utrace);

I don't think this is safe. If we do utrace_stop() here, the next engine
can be detached before we return (UTRACE_DETACH assumes it it safe to
unlink the engine when the target is stopped). This means we can't
continue list_for_each_entry(engine, utrace-attached, entry) after
return from finish_callback().

Oleg.