From owner-p4-projects@FreeBSD.ORG Sun Nov 14 05:13:25 2004 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 43E4F16A4D1; Sun, 14 Nov 2004 05:13:25 +0000 (GMT) Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 1619616A4CE for ; Sun, 14 Nov 2004 05:13:25 +0000 (GMT) Received: from repoman.freebsd.org (repoman.freebsd.org [216.136.204.115]) by mx1.FreeBSD.org (Postfix) with ESMTP id E919D43D46 for ; Sun, 14 Nov 2004 05:13:24 +0000 (GMT) (envelope-from davidxu@freebsd.org) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.12.11/8.12.11) with ESMTP id iAE5DOFN056481 for ; Sun, 14 Nov 2004 05:13:24 GMT (envelope-from davidxu@freebsd.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.12.11/8.12.11/Submit) id iAE5DOTv056478 for perforce@freebsd.org; Sun, 14 Nov 2004 05:13:24 GMT (envelope-from davidxu@freebsd.org) Date: Sun, 14 Nov 2004 05:13:24 GMT Message-Id: <200411140513.iAE5DOTv056478@repoman.freebsd.org> X-Authentication-Warning: repoman.freebsd.org: perforce set sender to davidxu@freebsd.org using -f From: David Xu To: Perforce Change Reviews Subject: PERFORCE change 65074 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 14 Nov 2004 05:13:25 -0000 http://perforce.freebsd.org/chv.cgi?CH=65074 Change 65074 by davidxu@davidxu_alona on 2004/11/14 05:12:40 1. Fix a race between signal and umtx_unlock. a waiter may be resumed by signal and left or exited, heavily loaded test causes kernel to crash. 2. Use distributed queue locks instead of single giant lock. Affected files ... .. //depot/projects/davidxu_ksedbg/src/sys/kern/kern_umtx.c#4 edit Differences ... ==== //depot/projects/davidxu_ksedbg/src/sys/kern/kern_umtx.c#4 (text+ko) ==== @@ -49,25 +49,48 @@ pid_t uq_pid; /* Pid key component. */ }; +LIST_HEAD(umtx_head, umtx_q); +struct umtxq_chain { + struct umtx_head uc_queues; /* List of sleep queues. */ + struct mtx uc_lock; /* lock for this chain. */ +}; + #define UMTX_QUEUES 128 #define UMTX_HASH(pid, umtx) \ - (((uintptr_t)pid + ((uintptr_t)umtx & ~65535)) % UMTX_QUEUES) + ((((uintptr_t)pid << 16) + ((uintptr_t)umtx & 65535)) % UMTX_QUEUES) -LIST_HEAD(umtx_head, umtx_q); -static struct umtx_head queues[UMTX_QUEUES]; +static struct umtxq_chain umtxq_chains[UMTX_QUEUES]; static MALLOC_DEFINE(M_UMTX, "umtx", "UMTX queue memory"); -static struct mtx umtx_lock; -MTX_SYSINIT(umtx, &umtx_lock, "umtx", MTX_DEF); - -#define UMTX_LOCK() mtx_lock(&umtx_lock); -#define UMTX_UNLOCK() mtx_unlock(&umtx_lock); +#define UMTX_LOCK(td, umtx) \ + mtx_lock(&umtxq_chains[UMTX_HASH((td)->td_proc->p_pid, \ + (umtx))].uc_lock) +#define UMTX_UNLOCK(td, umtx) \ + mtx_unlock(&umtxq_chains[UMTX_HASH((td)->td_proc->p_pid, \ + (umtx))].uc_lock); +#define UMTX_MUTEX(td, umtx) \ + (&umtxq_chains[UMTX_HASH(td->td_proc->p_pid, umtx)].uc_lock) #define UMTX_CONTESTED LONG_MIN +static void umtx_sysinit(void *); static struct umtx_q *umtx_lookup(struct thread *, struct umtx *umtx); static struct umtx_q *umtx_insert(struct thread *, struct umtx *umtx); +SYSINIT(umtx, SI_SUB_LOCK, SI_ORDER_MIDDLE, umtx_sysinit, NULL); + +static void +umtx_sysinit(void *arg) +{ + int i; + + for (i = 0; i < UMTX_QUEUES; ++i) { + LIST_INIT(&umtxq_chains[i].uc_queues); + mtx_init(&umtxq_chains[i].uc_lock, "umtxq_lock", NULL, + MTX_DEF | MTX_DUPOK); + } +} + static struct umtx_q * umtx_lookup(struct thread *td, struct umtx *umtx) { @@ -75,9 +98,11 @@ struct umtx_q *uq; pid_t pid; + mtx_assert(UMTXQ_MUTEX(td, umtx), MA_OWNED); + pid = td->td_proc->p_pid; - head = &queues[UMTX_HASH(td->td_proc->p_pid, umtx)]; + head = &umtxq_chains[UMTX_HASH(td->td_proc->p_pid, umtx)].uc_queues; LIST_FOREACH(uq, head, uq_next) { if (uq->uq_pid == pid && uq->uq_umtx == umtx) @@ -102,16 +127,16 @@ if ((uq = umtx_lookup(td, umtx)) == NULL) { struct umtx_q *ins; - UMTX_UNLOCK(); + UMTX_UNLOCK(td, umtx); ins = malloc(sizeof(*uq), M_UMTX, M_ZERO | M_WAITOK); - UMTX_LOCK(); + UMTX_LOCK(td, umtx); /* * Some one else could have succeeded while we were blocked * waiting on memory. */ if ((uq = umtx_lookup(td, umtx)) == NULL) { - head = &queues[UMTX_HASH(pid, umtx)]; + head = &umtxq_chains[UMTX_HASH(pid, umtx)].uc_queues; uq = ins; uq->uq_pid = pid; uq->uq_umtx = umtx; @@ -148,7 +173,7 @@ struct umtx *umtx; intptr_t owner; intptr_t old; - int error; + int error = 0; uq = NULL; @@ -189,10 +214,16 @@ continue; } + /* + * If we caught a signal, we have retried and now + * exit immediately. + */ + if (error) + return (error); - UMTX_LOCK(); + UMTX_LOCK(td, umtx); uq = umtx_insert(td, umtx); - UMTX_UNLOCK(); + UMTX_UNLOCK(td, umtx); /* * Set the contested bit so that a release in user space @@ -205,9 +236,9 @@ /* The address was invalid. */ if (old == -1) { - UMTX_LOCK(); + UMTX_LOCK(td, umtx); umtx_remove(uq, td); - UMTX_UNLOCK(); + UMTX_UNLOCK(td, umtx); return (EFAULT); } @@ -216,24 +247,29 @@ * and we need to retry or we lost a race to the thread * unlocking the umtx. */ - PROC_LOCK(td->td_proc); + UMTX_LOCK(td, umtx); if (old == owner && (td->td_flags & TDF_UMTXWAKEUP) == 0) - error = msleep(td, &td->td_proc->p_mtx, + error = msleep(td, UMTX_MUTEX(td, umtx), td->td_priority | PCATCH, "umtx", 0); else error = 0; - mtx_lock_spin(&sched_lock); - td->td_flags &= ~TDF_UMTXWAKEUP; - mtx_unlock_spin(&sched_lock); - PROC_UNLOCK(td->td_proc); + umtx_remove(uq, td); + UMTX_UNLOCK(td, umtx); - UMTX_LOCK(); - umtx_remove(uq, td); - UMTX_UNLOCK(); + if (td->td_flags & TDF_UMTXWAKEUP) { + /* + * if we were resumed by umtx_unlock, we should retry + * to avoid a race. + */ + mtx_lock_spin(&sched_lock); + td->td_flags &= ~TDF_UMTXWAKEUP; + mtx_unlock_spin(&sched_lock); + continue; + } /* - * If we caught a signal we might have to retry or exit - * immediately. + * If we caught a signal without resumed by umtx_unlock, + * exit immediately. */ if (error) return (error); @@ -246,7 +282,7 @@ _umtx_unlock(struct thread *td, struct _umtx_unlock_args *uap) /* struct umtx *umtx */ { - struct thread *blocked; + struct thread *blocked, *blocked2; struct umtx *umtx; struct umtx_q *uq; intptr_t owner; @@ -276,12 +312,17 @@ * there is zero or one thread only waiting for it. * Otherwise, it must be marked as contested. */ - UMTX_LOCK(); + UMTX_LOCK(td, umtx); uq = umtx_lookup(td, umtx); if (uq == NULL || (uq != NULL && (blocked = TAILQ_FIRST(&uq->uq_tdq)) != NULL && TAILQ_NEXT(blocked, td_umtx) == NULL)) { - UMTX_UNLOCK(); + if (blocked) { + mtx_lock_spin(&sched_lock); + blocked->td_flags |= TDF_UMTXWAKEUP; + mtx_unlock_spin(&sched_lock); + } + UMTX_UNLOCK(td, umtx); old = casuptr((intptr_t *)&umtx->u_owner, owner, UMTX_UNOWNED); if (old == -1) @@ -293,19 +334,25 @@ * Recheck the umtx queue to make sure another thread * didn't put itself on it after it was unlocked. */ - UMTX_LOCK(); + UMTX_LOCK(td, umtx); uq = umtx_lookup(td, umtx); if (uq != NULL && - ((blocked = TAILQ_FIRST(&uq->uq_tdq)) != NULL && + ((blocked2 = TAILQ_FIRST(&uq->uq_tdq)) != NULL && TAILQ_NEXT(blocked, td_umtx) != NULL)) { - UMTX_UNLOCK(); + if (blocked == NULL) { + mtx_lock_spin(&sched_lock); + blocked2->td_flags |= TDF_UMTXWAKEUP; + mtx_unlock_spin(&sched_lock); + blocked = blocked2; + } + UMTX_UNLOCK(td, umtx); old = casuptr((intptr_t *)&umtx->u_owner, UMTX_UNOWNED, UMTX_CONTESTED); } else { - UMTX_UNLOCK(); + UMTX_UNLOCK(td, umtx); } } else { - UMTX_UNLOCK(); + UMTX_UNLOCK(td, umtx); old = casuptr((intptr_t *)&umtx->u_owner, owner, UMTX_CONTESTED); if (old != -1 && old != owner) @@ -318,14 +365,7 @@ /* * If there is a thread waiting on the umtx, wake it up. */ - if (blocked != NULL) { - PROC_LOCK(blocked->td_proc); - mtx_lock_spin(&sched_lock); - blocked->td_flags |= TDF_UMTXWAKEUP; - mtx_unlock_spin(&sched_lock); - PROC_UNLOCK(blocked->td_proc); + if (blocked != NULL) wakeup(blocked); - } - return (0); }