From owner-p4-projects@FreeBSD.ORG Sun Nov 14 11:30:10 2004 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 45B1A16A4D1; Sun, 14 Nov 2004 11:30:10 +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 1FED116A4CE for ; Sun, 14 Nov 2004 11:30:10 +0000 (GMT) Received: from repoman.freebsd.org (repoman.freebsd.org [216.136.204.115]) by mx1.FreeBSD.org (Postfix) with ESMTP id 004A943D1F for ; Sun, 14 Nov 2004 11:30:10 +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 iAEBU9iP075090 for ; Sun, 14 Nov 2004 11:30:09 GMT (envelope-from davidxu@freebsd.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.12.11/8.12.11/Submit) id iAEBU9V2075087 for perforce@freebsd.org; Sun, 14 Nov 2004 11:30:09 GMT (envelope-from davidxu@freebsd.org) Date: Sun, 14 Nov 2004 11:30:09 GMT Message-Id: <200411141130.iAEBU9V2075087@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 65088 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 11:30:10 -0000 http://perforce.freebsd.org/chv.cgi?CH=65088 Change 65088 by davidxu@davidxu_alona on 2004/11/14 11:29:43 1. Fix another race in umtx_unlock, after current thread sets UMTX_UNOWNED, a new thread can come in and lock the umtx, this results current thread can not set UMTX_CONTESTED bit, Although current thread will wake a thread on wait queue,but that thread can receive a signal and breaks out of loop, so no contested bit will be set. 2. Macro optimization in umtx_lock, test sucessful before failure case. Affected files ... .. //depot/projects/davidxu_ksedbg/src/sys/kern/kern_umtx.c#7 edit Differences ... ==== //depot/projects/davidxu_ksedbg/src/sys/kern/kern_umtx.c#7 (text+ko) ==== @@ -56,8 +56,8 @@ }; #define UMTX_QUEUES 128 -#define UMTX_HASH(pid, umtx) \ - ((((uintptr_t)pid << 16) + ((uintptr_t)umtx & 65535)) % UMTX_QUEUES) +#define UMTX_HASH(pid, umtx) \ + ((((uintptr_t)pid << 16) + (((uintptr_t)umtx >> 4) & 65535)) % UMTX_QUEUES) static struct umtxq_chain umtxq_chains[UMTX_QUEUES]; static MALLOC_DEFINE(M_UMTX, "umtx", "UMTX queue memory"); @@ -73,14 +73,14 @@ #define UMTX_CONTESTED LONG_MIN -static void umtx_sysinit(void *); +static void umtx_queues_init(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); +SYSINIT(umtx, SI_SUB_LOCK, SI_ORDER_MIDDLE, umtx_queues_init, NULL); static void -umtx_sysinit(void *arg) +umtx_queues_init(void *arg) { int i; @@ -190,26 +190,26 @@ owner = casuptr((intptr_t *)&umtx->u_owner, UMTX_UNOWNED, td->td_tid); + /* The acquire succeeded. */ + if (owner == UMTX_UNOWNED) + return (0); + /* The address was invalid. */ if (owner == -1) return (EFAULT); - /* The acquire succeeded. */ - if (owner == UMTX_UNOWNED) - return (0); - /* If no one owns it but it is contested try to acquire it. */ if (owner == UMTX_CONTESTED) { owner = casuptr((intptr_t *)&umtx->u_owner, UMTX_CONTESTED, td->td_tid | UMTX_CONTESTED); + if (owner == UMTX_CONTESTED) + return (0); + /* The address was invalid. */ if (owner == -1) return (EFAULT); - if (owner == UMTX_CONTESTED) - return (0); - /* If this failed the lock has changed, restart. */ continue; } @@ -305,72 +305,69 @@ /* We should only ever be in here for contested locks */ if ((owner & UMTX_CONTESTED) == 0) return (EINVAL); - blocked = NULL; /* * When unlocking the umtx, it must be marked as unowned if * there is zero or one thread only waiting for it. * Otherwise, it must be marked as contested. */ + old = casuptr((intptr_t *)&umtx->u_owner, owner, UMTX_UNOWNED); + if (old == -1) + return (EFAULT); + if (old != owner) + return (EINVAL); + /* + * At the point, a new thread can lock the umtx before we + * reach here, so contested bit will not be set, if there + * are two or more threads on wait queue, we should set + * contensted bit for them. + */ + blocked = NULL; 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)) { - if (blocked) { - mtx_lock_spin(&sched_lock); - blocked->td_flags |= TDF_UMTXWAKEUP; - mtx_unlock_spin(&sched_lock); - } + if (uq == NULL) { UMTX_UNLOCK(td, umtx); - old = casuptr((intptr_t *)&umtx->u_owner, owner, - UMTX_UNOWNED); - if (old == -1) - return (EFAULT); - if (old != owner) - return (EINVAL); + return (0); + } + blocked2 = NULL; + if ((blocked = TAILQ_FIRST(&uq->uq_tdq)) != NULL) { + mtx_lock_spin(&sched_lock); + blocked->td_flags |= TDF_UMTXWAKEUP; + mtx_unlock_spin(&sched_lock); + blocked2 = TAILQ_NEXT(blocked, td_umtx); + } + UMTX_UNLOCK(td, umtx); + /* + * If there is second thread waiting on umtx, set contested bit, + * if they are resumed before we reach here, it is harmless, + * just a bit nonefficient. + */ + if (blocked2 != NULL) { + owner = UMTX_UNOWNED; + for (;;) { + old = casuptr((intptr_t *)&umtx->u_owner, owner, + owner | UMTX_CONTESTED); + if (old == -1) + return (EFAULT); + if (old == owner) + break; + owner = old; + } /* - * Recheck the umtx queue to make sure another thread - * didn't put itself on it after it was unlocked. + * Another thread locked the umtx before us, so don't bother + * to wake more threads, that thread will do it when it unlocks + * the umtx. */ - UMTX_LOCK(td, umtx); - uq = umtx_lookup(td, umtx); - if (uq != NULL && - ((blocked2 = TAILQ_FIRST(&uq->uq_tdq)) != NULL && - TAILQ_NEXT(blocked2, td_umtx) != NULL)) { - 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(td, umtx); - } - } else { - if (blocked != NULL) { - 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_CONTESTED); - if (old != -1 && old != owner) - return (EINVAL); + if ((owner & ~UMTX_CONTESTED) != 0) + return (0); } - if (old == -1) - return (EFAULT); - /* * If there is a thread waiting on the umtx, wake it up. */ if (blocked != NULL) wakeup(blocked); + return (0); }