On Wed, Apr 04, 2007 at 10:55:01PM -0700, Paul Menage wrote: > >@@ -1257,8 +1260,8 @@ static int attach_task(struct cpuset *cs > > > > put_task_struct(tsk); > > synchronize_rcu(); > >- if (atomic_dec_and_test(&oldcs->count)) > >- check_for_release(oldcs, ppathbuf); > >+ if (oldcs_to_be_released) > >+ check_for_release(oldcs_to_be_released, ppathbuf); > > return 0; > > } > > Is this part of the patch necessary? If we're adding a task_lock() in > cpuset_exit(), then the problem that Vatsa described (both > cpuset_attach_task() and cpuset_exit() decrementing the same cpuset > count, and cpuset_attach_task() incrementing the count on a cpuset > that the task doesn't eventually end up in) go away, since only one > thread will retrieve the old value of the task's cpuset in order to > decrement its count.
You *have* to drop/inc the refcount inside the task_lock, otherwise it is racy. task_lock(T1); old_cs = T1->cputset (C1) atomic_inc(&C2->count); T1->cputset = C2; task_unlock(); ... synchronize_rcu(); if (atomic_dec_and_test(&C1->count)) check_for_release(..) is incorrect. For ex: T1's refcount on C1 may have already been dropped by now in cpuset_exit() and dropping the refcount again can lead to negative refcounts. . > > void cpuset_exit(struct task_struct *tsk) > > { > > struct cpuset *cs; > >+ struct cpuset *oldcs_to_be_released = NULL; > > > >+ task_lock(tsk); > > cs = tsk->cpuset; > > tsk->cpuset = &top_cpuset; /* the_top_cpuset_hack - see above > > */ > >+ if (atomic_dec_and_test(&cs->count)) > >+ oldcs_to_be_released = cs; > >+ task_unlock(tsk); > > > > I think this is still racy - at this point we're holding a reference > on a cpuset that could have a zero count, How's that possible? That you have a zero-refcount cpuset with non empty tasks in it? > and we don't hold > manage_mutex or callback_mutex. So a concurrent rmdir could zap the > directory and free the cpuset. I don't think that is possible. Can you explain? > Shouldn't we just put a task_lock()/task_unlock() around these lines > and leave everything else as-is? > > task_lock(tsk); > cs = tsk->cpuset; > tsk->cpuset = &top_cpuset; /* the_top_cpuset_hack - see above */ > task_unlock(tsk) If we don't drop refcount inside task_lock() it makes it racy with attach_task(). 'cs' derived above may not be the right cpuset to drop refcount on later in cpuset_exit. -- Regards, vatsa - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/