Possible problem with utrace_control

[Oleg Nesterov]

On 06/22, Roland McGrath wrote:
>
> > OTOH, I am wondering if we can give more power to utrace_control(STOP).
> > Currently debugger can't stop the tracee unless it cooperates. However, if
> > the tracee doesn't have UTRACE_EVENT(QUIESCE), then utrace_control(STOP)
> > works. This is strange.
>
> Why is it strange?  If you don't have any callback, then all your engine
> does is get stopped.  If you have callbacks, then you always get callbacks
> before the thread does anything (including stop).  Since your callback's
> return value always replaces any other resume action

OK. But then perhaps UTRACE_STICKY_STOP makes sense even without the
races we discussed. It can simplify the cooperation in this case.

> > Stupid question: what if we just remove the clear_engine_wants_stop()
> > code in finish_callback_report() ?
>
> That would change the API so that a callback's resume action is always
> overridden by a utrace_control(,,UTRACE_STOP).  Today the practice from a
> third-party debugger thread is to do:
>
> 	ret = utrace_control(task, engine, UTRACE_STOP);
> 	if (ret == 0) {
> 	  // Already stopped, e.g. was in jctl or another engine's stop.
> 	  // This means we can do our asynchronous examination here.
> 	} else if (ret == -EINPROGRESS) {
> 	  // it will get to report_quiesce soonish (modulo blocked in kernel)
> 	}
>
> If the examination you wanted to do was some predetermined sequence of
> fetching and fiddling before resuming, then the report_* callback can just
> do all that itself.  For example, fetch the registers for a sample and keep
> going; insert some breakpoints and keep going; etc.

OK, agreed.

> With this
> change, it would have to explicitly request not stopping by calling
> utrace_control(current, engine, UTRACE_RESUME) inside the callback.

Ah, indeed, this can't be good. Thanks.

> > Btw, before I forgot about this. shouldn't utrace_control(UTRACE_RESUME)
> > remove ENGINE_STOP from ->utrace_flags like DETACH does?
>
> Perhaps so.  My memory is dim on the use of that bit in utrace_flags.

(don't assume I was able to quickly recall why DETACH does this ;)

> It's
> to close certain races, but we'll have to remind each other of the details.

Yes. Let's consider the concrete example. The tracee is going to
stop and calls utrace_stop(). Before it takes utrace->lock and
sets state = TASK_TRACED, the debugger does utrace_control(DETACH).

In this case utrace_stop() shouldn't stop, otherwise nobody will
ever wake it up. That is why we clear this bit in ->utrace_flags
to provoke utrace_reset() which will check carefully the tracee
has an engine with ENGINE_STOP.

The original motivation for this patch was ptrace-utrace, the
implicit DETACH if the tracer exits.

UTRACE_RESUME should probably do the same, otherwise the tracee
can stop right after utrace_control(RESUME). This case is less
important compared to UTRACE_DETACH (and just in case, ptrace
doesn't need this) but still.

> > Hmm. Not sure... at least I don't immediately see something simple.
> >
> > Except, just add wait_queue_head into struct utrace. This way utrace_barrier()
> > can wait longer than needed if it notices utrace->reporting == engine. Or we
> > can add it into utrace_engine, in this case utrace_stop() needs
> > list_for_each(engine, utrace->attached).
>
> You're talking about two different things here.  Whatever implementation
> details change in utrace_barrier() is not directly apropos.

Yes, agreed.

> For the new API idea, I was talking about something like a utrace_wake_up()
> call.

Now I don't understand you again...

The 1st question: should the new API help us to kill ptrace_notify_stop()
in utrace_stop() ?

> What I'm most interested in right now is thinking through the utility of
> particular new API details for engine writers.

Hmm... After the previous email I thought that this utrace_wake_up()
or whatever shouldn't be visible outside of utrace.c.

Ignoring the ptrace_notify_stop() problem, could you give us any
example of how it can be used?

Or. Do you literally mean something like

	utrace_wait_for_event();		// for debugger
	utrace_wake_up();			// for tracee

which should (at least) cover all races with exit/detach/etc ?

Oleg.