On 08/30, Roland McGrath wrote:
>
> > This case is not unlikely, for example ENGINE_STOP is cleared after the
> > previous wakeup. We are running, if ENGINE_STOP is set, UTRACE_STOP was
> > used and utrace_reset() was called after that. Otherwise, _perhaps_ it
> > was cleared by UTRACE_DETACH.
>
> What we want is to change things so that this case *is* unlikely. That is,
> we really want to avoid any extra O(n) work such as utrace_reset calls. It
> should never be common to do one of those we don't really want to do. And,
> all else being equal, when some extra O(n) work is unavoidably required,
> IMHO it's better that it be in the tracer than in the tracee.
Yes, agreed.
> I took a different tack and committed these:
>
> 96fe3cc Revert "utrace_stop: fix UTRACE_DETACH race"
> d1a14ce utrace: change UTRACE_STOP bookkeeping
> 64a8ca3 utrace_reset cleanup
I think this should work, but I'd like to re-read these changes carefully,
this all is subtle. And, at first glance, we need something like the patch
below.
Because now utrace_control(UTRACE_DETACH) does
case UTRACE_DETACH:
...
if (engine_wants_stop(engine))
resume = true;
OK, we need to call utrace_reset() to recalc ->utrace_flags. But utrace_reset()
assumes it is safe to play with utrace->attached list, now this is not true.
IOW, with this patch utrace_control(UTRACE_DETACH)->utrace_reset() can race
with REPORT_CALLBACKS/etc which does list_for_each_entry(->attached).
(minor, but perhaps resume should be renamed to "bool reset")
"task != current" before utrace_wakeup() doesn't look right too, I think we
should check ->stopped instead. Harmless, but otherwise we can call wakeup()
while it is not needed.
Oleg.
--- a/kernel/utrace.c
+++ b/kernel/utrace.c
@@ -722,11 +722,12 @@ static bool utrace_reset(struct task_str
__releases(utrace->lock)
{
struct utrace_engine *engine, *next;
+ bool xxx = (utrace->stopped || task == current);
unsigned long flags = 0;
LIST_HEAD(detached);
- splice_attaching(utrace);
-
+ if (xxx)
+ splice_attaching(utrace);
/*
* Update the set of events of interest from the union
* of the interests of the remaining tracing engines.
@@ -734,7 +735,7 @@ static bool utrace_reset(struct task_str
* We'll collect them on the detached list.
*/
list_for_each_entry_safe(engine, next, &utrace->attached, entry) {
- if (engine->ops == &utrace_detached_ops) {
+ if (xxx && engine->ops == &utrace_detached_ops) {
engine->ops = NULL;
list_move(&engine->entry, &detached);
} else {
@@ -765,7 +766,7 @@ static bool utrace_reset(struct task_str
*/
utrace->interrupt = utrace->report = utrace->signal_handler = 0;
- if (!(flags & ENGINE_STOP) && task != current)
+ if (!(flags & ENGINE_STOP) && utrace->stopped)
/*
* No more engines want it stopped. Wake it up.
*/
