Attachment 206036 Details for Bug 239351 – proposed patch

[patch] proposed patch

diff (text/plain), 3.81 KB, created by Mark Johnston on 2019-07-24 13:46:49 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Johnston

Created: 2019-07-24 13:46:49 UTC

Size: 3.81 KB

patch

obsolete

>commit 69eacd1bd8d34f0644f8fb91ce94f35f50738d63
>Author: Mark Johnston <markjdb@gmail.com>
>Date:   Tue Jul 23 21:30:53 2019 -0400
>
>    Fix epoch's use of turnstile_lock().
>    
>    turnstile_lock() was added specifically for use in epoch.  While waiting
>    for readers to drain, if the blocking thread discovers a reader blocked
>    on a turnstile, it will also block on the turnstile.  It previously
>    returned the value of the ts_owner field to indicate that the turnstile
>    was successfully locked, and NULL to indicate that the race was lost.
>    But, ts_owner may be NULL if the turnstile corresponds to a rw lock
>    owned by one or more readers, in which case the epoch code leaked the
>    turnstile lock and chain lock.
>    
>    Apply a minimal fix: simply return the owner as a separate value.
>
>diff --git a/sys/kern/subr_epoch.c b/sys/kern/subr_epoch.c
>index 6bbe07aaa330..aec3a70c27d2 100644
>--- a/sys/kern/subr_epoch.c
>+++ b/sys/kern/subr_epoch.c
>@@ -438,26 +438,20 @@ epoch_block_handler_preempt(struct ck_epoch *global __unused,
> 			 */
> 			critical_enter();
> 			thread_unlock(td);
>-			owner = turnstile_lock(ts, &lock);
>-			/*
>-			 * The owner pointer indicates that the lock succeeded.
>-			 * Only in case we hold the lock and the turnstile we
>-			 * locked is still the one that curwaittd is blocked on
>-			 * can we continue. Otherwise the turnstile pointer has
>-			 * been changed out from underneath us, as in the case
>-			 * where the lock holder has signalled curwaittd,
>-			 * and we need to continue.
>-			 */
>-			if (owner != NULL && ts == curwaittd->td_blocked) {
>-				MPASS(TD_IS_INHIBITED(curwaittd) &&
>-				    TD_ON_LOCK(curwaittd));
>-				critical_exit();
>-				turnstile_wait(ts, owner, curwaittd->td_tsqueue);
>-				counter_u64_add(turnstile_count, 1);
>-				thread_lock(td);
>-				return;
>-			} else if (owner != NULL)
>+
>+			if (turnstile_lock(ts, &lock, &owner)) {
>+				if (ts == curwaittd->td_blocked) {
>+					MPASS(TD_IS_INHIBITED(curwaittd) &&
>+					    TD_ON_LOCK(curwaittd));
>+					critical_exit();
>+					turnstile_wait(ts, owner,
>+					    curwaittd->td_tsqueue);
>+					counter_u64_add(turnstile_count, 1);
>+					thread_lock(td);
>+					return;
>+				}
> 				turnstile_unlock(ts, lock);
>+			}
> 			thread_lock(td);
> 			critical_exit();
> 			KASSERT(td->td_locks == locksheld,
>diff --git a/sys/kern/subr_turnstile.c b/sys/kern/subr_turnstile.c
>index c95120a3d561..12272e39d45d 100644
>--- a/sys/kern/subr_turnstile.c
>+++ b/sys/kern/subr_turnstile.c
>@@ -566,24 +566,26 @@ turnstile_trywait(struct lock_object *lock)
> 	return (ts);
> }
> 
>-struct thread *
>-turnstile_lock(struct turnstile *ts, struct lock_object **lockp)
>+bool
>+turnstile_lock(struct turnstile *ts, struct lock_object **lockp,
>+    struct thread **tdp)
> {
> 	struct turnstile_chain *tc;
> 	struct lock_object *lock;
> 
> 	if ((lock = ts->ts_lockobj) == NULL)
>-		return (NULL);
>+		return (false);
> 	tc = TC_LOOKUP(lock);
> 	mtx_lock_spin(&tc->tc_lock);
> 	mtx_lock_spin(&ts->ts_lock);
> 	if (__predict_false(lock != ts->ts_lockobj)) {
> 		mtx_unlock_spin(&tc->tc_lock);
> 		mtx_unlock_spin(&ts->ts_lock);
>-		return (NULL);
>+		return (false);
> 	}
> 	*lockp = lock;
>-	return (ts->ts_owner);
>+	*tdp = ts->ts_owner;
>+	return (true);
> }
> 
> void
>diff --git a/sys/sys/turnstile.h b/sys/sys/turnstile.h
>index 975781ae9170..c74d069041ce 100644
>--- a/sys/sys/turnstile.h
>+++ b/sys/sys/turnstile.h
>@@ -99,7 +99,8 @@ int	turnstile_signal(struct turnstile *, int);
> struct turnstile *turnstile_trywait(struct lock_object *);
> void	turnstile_unpend(struct turnstile *);
> void	turnstile_wait(struct turnstile *, struct thread *, int);
>-struct thread *turnstile_lock(struct turnstile *, struct lock_object **);
>+bool	turnstile_lock(struct turnstile *, struct lock_object **,
>+	    struct thread **);
> void	turnstile_unlock(struct turnstile *, struct lock_object *);
> void	turnstile_assert(struct turnstile *);
> #endif	/* _KERNEL */

Actions: View | Diff

Attachments on bug 239351: 205949 | 206031 | 206032 | 206036 | 206043 | 206045 | 206046 | 206050 | 206051 | 206056 | 206061 | 206063