On 09/08, Oleg Nesterov wrote: > > I am starting to think we are going to add too much subtle complications > to avoid lock/unlock in release path :/
Or not, I dunno. up to you. But if we do this change, I'd like to re-factor utrace_add_engine(). Mostly because "if (utrace->reap)" check should be near ->exit_state check, they both are the same thing: protect against release. OTOH, if we change utrace_reap() to clear ->utrace_flags, then utrace_add_engine() do not need to check ->reap at all. If you don't like it - just ignore, I don't have a strong feeling. To simplify the review: static int utrace_add_engine(struct task_struct *target, struct utrace *utrace, struct utrace_engine *engine, int flags, const struct utrace_engine_ops *ops, void *data) { int ret; spin_lock(&utrace->lock); ret = -EEXIST; if ((flags & UTRACE_ATTACH_EXCLUSIVE) && unlikely(matching_engine(utrace, flags, ops, data))) goto unlock; ret = -ESRCH; /* can't attach after utrace_release_task() */ if (utrace->reap) goto unlock; /* * In case we had no engines before, make sure that * utrace_flags is not zero. */ if (!target->utrace_flags) { target->utrace_flags = UTRACE_EVENT(REAP); /* * If we race with tracehook_prepare_release_task() * make sure that either it sees utrace_flags != 0 * or we see exit_state == EXIT_DEAD. */ smp_mb(); if (unlikely(target->exit_state == EXIT_DEAD)) { target->utrace_flags = 0; goto unlock; } } /* * Put the new engine on the pending ->attaching list. * Make sure it gets onto the ->attached list by the next * time it's examined. Setting ->pending_attach ensures * that start_report() takes the lock and splices the lists * before the next new reporting pass. * * When target == current, it would be safe just to call * splice_attaching() right here. But if we're inside a * callback, that would mean the new engine also gets * notified about the event that precipitated its own * creation. This is not what the user wants. */ list_add_tail(&engine->entry, &utrace->attaching); utrace->pending_attach = 1; ret = 0; unlock: spin_unlock(&utrace->lock); return ret; } Signed-off-by: Oleg Nesterov <o...@redhat.com> --- kernel/utrace.c | 81 +++++++++++++++++++++++++++----------------------------- 1 file changed, 40 insertions(+), 41 deletions(-) --- __UTRACE/kernel/utrace.c~4_CLEANUP_ADD_ENGINE 2009-09-08 22:32:44.000000000 +0200 +++ __UTRACE/kernel/utrace.c 2009-09-08 23:46:10.000000000 +0200 @@ -139,55 +139,54 @@ static int utrace_add_engine(struct task const struct utrace_engine_ops *ops, void *data) { - int ret = 0; + int ret; spin_lock(&utrace->lock); - if (utrace->reap) { - /* - * Already entered utrace_release_task(), cannot attach now. - */ - ret = -ESRCH; - } else if ((flags & UTRACE_ATTACH_EXCLUSIVE) && - unlikely(matching_engine(utrace, flags, ops, data))) { - ret = -EEXIST; - } else { + ret = -EEXIST; + if ((flags & UTRACE_ATTACH_EXCLUSIVE) && + unlikely(matching_engine(utrace, flags, ops, data))) + goto unlock; + + ret = -ESRCH; + /* can't attach after utrace_release_task() */ + if (utrace->reap) + goto unlock; + /* + * In case we had no engines before, make sure that + * utrace_flags is not zero. + */ + if (!target->utrace_flags) { + target->utrace_flags = UTRACE_EVENT(REAP); /* - * Put the new engine on the pending ->attaching list. - * Make sure it gets onto the ->attached list by the next - * time it's examined. Setting ->pending_attach ensures - * that start_report() takes the lock and splices the lists - * before the next new reporting pass. - * - * When target == current, it would be safe just to call - * splice_attaching() right here. But if we're inside a - * callback, that would mean the new engine also gets - * notified about the event that precipitated its own - * creation. This is not what the user wants. - * - * In case we had no engines before, make sure that - * utrace_flags is not zero. + * If we race with tracehook_prepare_release_task() + * make sure that either it sees utrace_flags != 0 + * or we see exit_state == EXIT_DEAD. */ - if (!target->utrace_flags) { - target->utrace_flags = UTRACE_EVENT(REAP); - /* - * If we race with tracehook_prepare_release_task() - * make sure that either it sees utrace_flags != 0 - * or we see exit_state == EXIT_DEAD. - */ - smp_mb(); - if (unlikely(target->exit_state == EXIT_DEAD)) { - target->utrace_flags = 0; - ret = -ESRCH; - } - } - - if (!ret) { - list_add_tail(&engine->entry, &utrace->attaching); - utrace->pending_attach = 1; + smp_mb(); + if (unlikely(target->exit_state == EXIT_DEAD)) { + target->utrace_flags = 0; + goto unlock; } } + /* + * Put the new engine on the pending ->attaching list. + * Make sure it gets onto the ->attached list by the next + * time it's examined. Setting ->pending_attach ensures + * that start_report() takes the lock and splices the lists + * before the next new reporting pass. + * + * When target == current, it would be safe just to call + * splice_attaching() right here. But if we're inside a + * callback, that would mean the new engine also gets + * notified about the event that precipitated its own + * creation. This is not what the user wants. + */ + list_add_tail(&engine->entry, &utrace->attaching); + utrace->pending_attach = 1; + ret = 0; +unlock: spin_unlock(&utrace->lock); return ret;