>From the previous changelog:
>
> Once we drop utrace->lock, any other caller which takes this lock must
> see both ->exit_state and utrace->reap. This means utrace_control() or
> utrace_set_events() which sets/clears REAP must not succeed.
IOW, nobody change change engine->ops or REAP bit in flags. Nobody can
add the new engine or remove it from ->attached. We can do all work
lockless and without barriers.
To simplify the review, utrace_reap() becomes:
static void utrace_reap(struct task_struct *target, struct utrace
*utrace)
__releases(utrace->lock)
{
struct utrace_engine *engine;
splice_attaching(utrace);
spin_unlock(&utrace->lock);
/*
* We were called with utrace->reap set:
* nobody can set/clear UTRACE_EVENT(REAP) in engine->flags or
* change engine->ops, and nobody can change utrace->attached.
*/
list_for_each_entry(engine, &utrace->attached, entry) {
if (engine->flags & UTRACE_EVENT(REAP))
engine->ops->report_reap(engine, target);
engine->ops = NULL;
engine->flags = 0;
}
put_detached_list(&utrace->attached);
}
Note that I changed the logic a bit, we set ->ops = NULL after
->report_reap(). Is it OK? If not, it is trivial to change.
Signed-off-by: Oleg Nesterov <o...@redhat.com>
---
kernel/utrace.c | 39 ++++++++++++---------------------------
1 file changed, 12 insertions(+), 27 deletions(-)
--- __UTRACE/kernel/utrace.c~3_NO_LOCKING 2009-09-06 15:09:59.000000000
+0200
+++ __UTRACE/kernel/utrace.c 2009-09-06 15:06:23.000000000 +0200
@@ -408,40 +408,25 @@ static void put_detached_list(struct lis
static void utrace_reap(struct task_struct *target, struct utrace *utrace)
__releases(utrace->lock)
{
- struct utrace_engine *engine, *next;
- const struct utrace_engine_ops *ops;
- unsigned long flags;
- LIST_HEAD(detached);
+ struct utrace_engine *engine;
-restart:
splice_attaching(utrace);
- list_for_each_entry_safe(engine, next, &utrace->attached, entry) {
- ops = engine->ops;
- flags = engine->flags;
+ spin_unlock(&utrace->lock);
+ /*
+ * We were called with utrace->reap set:
+ * nobody can set/clear UTRACE_EVENT(REAP) in engine->flags or
+ * change engine->ops, and nobody can change utrace->attached.
+ */
+ list_for_each_entry(engine, &utrace->attached, entry) {
+ if (engine->flags & UTRACE_EVENT(REAP))
+ engine->ops->report_reap(engine, target);
+
engine->ops = NULL;
engine->flags = 0;
- list_move(&engine->entry, &detached);
- /*
- * If it didn't need a callback, we don't need to drop
- * the lock. Now nothing else refers to this engine.
- */
- if (!(flags & UTRACE_EVENT(REAP)))
- continue;
-
- spin_unlock(&utrace->lock);
-
- (*ops->report_reap)(engine, target);
-
- put_detached_list(&detached);
-
- spin_lock(&utrace->lock);
- goto restart;
}
- spin_unlock(&utrace->lock);
-
- put_detached_list(&detached);
+ put_detached_list(&utrace->attached);
}
/*
