Personally I dislike the complications introduced by safe/unsafe modes of
utrace_reset().
And I think these complications are not needed. Let's return to the previous
discussion:
> 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.
Yes! But with the recent changes mark_engine_wants_stop() adds ENGINE_STOP
to ->utrace_flags, this means the case when utrace_stop() calls utrace_reset()
is really unlikely.
So. Kill this (imho) awful "bool safe" argument. utrace_reset() must be always
safe. Instead,
- change utrace_control(DETACH) to clear ENGINE_STOP from ->utrace_flags
- change utrace_stop() to call utrace_reset() and re-check when there is
no ENGINE_STOP in ->utrace_flags.
IOW, this partly re-rereverts 00932dbe24bd4273e6d723baa9342e7b3e248911
Signed-off-by: Oleg Nesterov <o...@redhat.com>
---
kernel/utrace.c | 39 +++++++++++++++------------------------
1 file changed, 15 insertions(+), 24 deletions(-)
--- __UTRACE/kernel/utrace.c~2_KILL_SAFE 2009-09-01 20:03:25.000000000
+0200
+++ __UTRACE/kernel/utrace.c 2009-09-01 20:07:45.000000000 +0200
@@ -707,22 +707,17 @@ static void utrace_wakeup(struct task_st
* some stale bits in @task->utrace_flags. Clean them up and recompute the
* flags. Returns true if we're now fully detached.
*
- * @safe is true if this is a safe point to touch the engine list,
- * i.e. @task is either current or known to be stopped.
- *
* Called with @utrace->lock held, returns with it released.
* After this returns, @utrace might be freed if everything detached.
*/
-static bool utrace_reset(struct task_struct *task, struct utrace *utrace,
- bool safe)
+static bool utrace_reset(struct task_struct *task, struct utrace *utrace)
__releases(utrace->lock)
{
struct utrace_engine *engine, *next;
unsigned long flags = 0;
LIST_HEAD(detached);
- if (safe)
- splice_attaching(utrace);
+ splice_attaching(utrace);
/*
* Update the set of events of interest from the union
@@ -731,7 +726,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 (safe && engine->ops == &utrace_detached_ops) {
+ if (engine->ops == &utrace_detached_ops) {
engine->ops = NULL;
list_move(&engine->entry, &detached);
} else {
@@ -789,7 +784,7 @@ static void utrace_stop(struct task_stru
* inside this function. It should never be set on entry.
*/
BUG_ON(utrace->stopped);
-
+relock:
spin_lock(&utrace->lock);
if (action == UTRACE_INTERRUPT) {
@@ -816,7 +811,9 @@ static void utrace_stop(struct task_stru
* steps (this can matter for UTRACE_RESUME but not UTRACE_DETACH).
*/
if (unlikely(!(task->utrace_flags & ENGINE_STOP))) {
- spin_unlock(&utrace->lock);
+ utrace_reset(task, utrace);
+ if (task->utrace_flags & ENGINE_STOP)
+ goto relock;
return;
}
@@ -1037,7 +1034,7 @@ int utrace_control(struct task_struct *t
enum utrace_resume_action action)
{
struct utrace *utrace;
- bool reset, safe = false;
+ bool reset;
int ret;
if (unlikely(action > UTRACE_DETACH))
@@ -1074,7 +1071,7 @@ int utrace_control(struct task_struct *t
spin_unlock(&utrace->lock);
return ret;
}
- reset = safe = true;
+ reset = true;
}
switch (action) {
@@ -1086,6 +1083,9 @@ int utrace_control(struct task_struct *t
break;
case UTRACE_DETACH:
+ if (engine_wants_stop(engine))
+ target->utrace_flags &= ~ENGINE_STOP;
+ mark_engine_detached(engine);
reset = reset || utrace_do_stop(target, utrace);
if (!reset) {
/*
@@ -1100,14 +1100,6 @@ int utrace_control(struct task_struct *t
if (utrace->reporting == engine)
ret = -EINPROGRESS;
}
- if (engine_wants_stop(engine)) {
- /*
- * We need utrace_reset() to check if anyone else
- * still wants this target to stay stopped.
- */
- reset = true;
- }
- mark_engine_detached(engine);
break;
case UTRACE_RESUME:
@@ -1211,8 +1203,7 @@ int utrace_control(struct task_struct *t
* there is nothing more we need to do.
*/
if (reset)
- utrace_reset(target, utrace, safe || utrace->stopped ||
- target == current);
+ utrace_reset(target, utrace);
else
spin_unlock(&utrace->lock);
@@ -1318,7 +1309,7 @@ static inline void finish_report_reset(s
{
if (unlikely(!report->takers || report->detaches)) {
spin_lock(&utrace->lock);
- if (utrace_reset(task, utrace, true))
+ if (utrace_reset(task, utrace))
report->action = UTRACE_RESUME;
}
}
@@ -1741,7 +1732,7 @@ void utrace_report_death(struct task_str
*/
utrace_reap(task, utrace);
else
- utrace_reset(task, utrace, true);
+ utrace_reset(task, utrace);
}
/*
