[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: asynchronous detach



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]