On Tue, 2013-03-26 at 16:00 -0400, Rik van Riel wrote: > On Tue, 26 Mar 2013 14:07:14 -0400 > Sasha Levin <[email protected]> wrote: > > > > Not necessarily, we do release everything at the end of the function: > > > out_unlock_free: > > > sem_unlock(sma, locknum); > > > > Ow, there's a rcu_read_unlock() in sem_unlock()? This complicates things > > even > > more I suspect. If un is non-NULL we'll be unlocking rcu lock twice? > > Sasha, this patch should resolve the RCU tangle, by making sure > we only ever take the rcu_read_lock once in semtimedop. > > ---8<--- > > The ipc semaphore code has a nasty RCU locking tangle, with both > find_alloc_undo and semtimedop taking the rcu_read_lock(). The > code can be cleaned up somewhat by only taking the rcu_read_lock > once. > > There are no other callers to find_alloc_undo. > > This should also solve the trinity issue reported by Sasha Levin.
I take it this is on top of the patchlet Sasha submitted? (I hit rcu stall banging on patch set in rt with 60 synchronized core executive model if I let it run long enough, fwtw) > Reported-by: Sasha Levin <[email protected]> > Signed-off-by: Rik van Riel <[email protected]> > --- > ipc/sem.c | 31 +++++++++---------------------- > 1 files changed, 9 insertions(+), 22 deletions(-) > > diff --git a/ipc/sem.c b/ipc/sem.c > index f46441a..2ec2945 100644 > --- a/ipc/sem.c > +++ b/ipc/sem.c > @@ -1646,22 +1646,23 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf > __user *, tsops, > alter = 1; > } > > + INIT_LIST_HEAD(&tasks); > + > if (undos) { > + /* On success, find_alloc_undo takes the rcu_read_lock */ > un = find_alloc_undo(ns, semid); > if (IS_ERR(un)) { > error = PTR_ERR(un); > goto out_free; > } > - } else > + } else { > un = NULL; > + rcu_read_lock(); > + } > > - INIT_LIST_HEAD(&tasks); > - > - rcu_read_lock(); > sma = sem_obtain_object_check(ns, semid); > if (IS_ERR(sma)) { > - if (un) > - rcu_read_unlock(); > + rcu_read_unlock(); > error = PTR_ERR(sma); > goto out_free; > } > @@ -1693,22 +1694,8 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf > __user *, tsops, > */ > error = -EIDRM; > locknum = sem_lock(sma, sops, nsops); > - if (un) { > - if (un->semid == -1) { > - rcu_read_unlock(); > - goto out_unlock_free; > - } else { > - /* > - * rcu lock can be released, "un" cannot disappear: > - * - sem_lock is acquired, thus IPC_RMID is > - * impossible. > - * - exit_sem is impossible, it always operates on > - * current (or a dead task). > - */ > - > - rcu_read_unlock(); > - } > - } > + if (un && un->semid == -1) > + goto out_unlock_free; > > error = try_atomic_semop (sma, sops, nsops, un, task_tgid_vnr(current)); > if (error <= 0) { > -- > 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/ -- 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/
