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 <[email protected]>
---
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;
