On Thu, Jul 31, 2008 at 03:38:02PM -0700, Roland McGrath wrote:
...
> > I think this can be a good model to use for non-perturbing quiesce for
> > cases as breakpoint insertion and removal or any applicaton text
> > modification, where one needs to ensure no thread is executing in the
> > vicinity of the said modification. So, even on an -EINPROGRESS on a
> > utrace_control, if the thread is in kernel, manipulating application
> > text is still safe, right?
>
> For this particular case, maybe "safe enough". I wouldn't like to
> encourage thinking of it as safe in general. Whatever the kernel is
> doing might be touching user memory, or unmapping it or whatever. For
> breakpoint insertion/removal, you are pretty much assuming nothing else
> would ever touch the text anyway. You still need an interlock against
> munmap text + mmap reusing addrs in quick succession. I suppose you
> could hold mmap_sem or something.
>
> > Not sure if a similar model can be used to address the teardown races
> > problem.
>
> Hmm. I mentioned before having a non-synchronizing record of the
> callback in progress. This lets UTRACE_DETACH return -EINPROGRESS,
> which is after the horse has left the barn, as it were. There is a
> related extension of this idea I had vaguely in mind but didn't mention.
> A utrace_set_events call that disables event bits could also return
> -EINPROGRESS when some callback might be in progress. As with detach,
> that means a zero return tells you all is well. Unlike detach, if you
> get -EINPROGRESS from utrace_set_events, you still have the engine.
So, the intention is that -EINPROGRESS can be returned only on a
utrace_set_events(task, engine, 0), right?
Actually, I think it would also be useful to just have a
utrace_clear_events_sync() that encapsulates the above call. I can see
use for it outside of an asynchronous detach, for cases when one needs
to just turn tracing events off on a thread. (It is definitely not
recommended for debugger/probes usage with breakpoints already set in
user code, but would be useful for someone doing non-invasive syscall
and process lifetime tracing).
> If your callback never blocks, that is enough to know that it's safe to
> free your data, unload your module, etc. (Or if your callback can block
> but what you want to synchronize with is just what it does before it
> blocks. That's when you don't care about being able to unload the
> module, just juggling your own data structures.)
>
> For the general case of some unknown callback code that might include
> blocking, you could instead do:
>
> ret = utrace_set_events(task, engine, 0);
> while (ret == -EINPROGRESS) {
> ret = utrace_prepare_examine(task, engine, &exam);
> if (ret == -EAGAIN)
> yield();
> ret = utrace_set_events(task, engine, 0);
> if (ret == -EINPROGRESS)
> yield();
> }
>
> That is: when it is blocked, check again if a callback still might be in
> progress, and loop. Assuming your callback is well-behaved and only
> ever blocks for a short time (like kmalloc), this will return soon.
This can cetainly work!
> yield(); should actually be schedule_timeout_interruptible(1); probably,
> and it should be able to return -ERESTARTSYS.
>
> If this is a good plan, then we should encapsulate it in a new call
> utrace_set_events_sync() or something like that. So then the safe
> detach procedure would be just:
>
> ret = utrace_set_events_sync(task, engine, UTRACE_EVENT(REAP));
> if (!ret)
> ret = utrace_control(task, engine, UTRACE_DETACH);
>
> This can only return zero or -ESRCH. Zero says that all callbacks are
> definitely finished. -ESRCH says that your report_reap callback is
> definitely happening (right now, modulo preemption).
>
> We could encapsulate that into utrace_detach_sync() too.
Yes, this does take care of the thread about to die case too.
Ananth
