Skip site navigation (1)Skip section navigation (2) 
Date:      Wed, 11 Nov 2020 08:50:04 +0000 (UTC)
From:      Mateusz Guzik <mjg@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r367584 - in head/sys: kern sys
Message-ID:  <202011110850.0AB8o4VA062275@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: mjg
Date: Wed Nov 11 08:50:04 2020
New Revision: 367584
URL: https://svnweb.freebsd.org/changeset/base/367584

Log:
  thread: rework tidhash vs proc lock interaction
  
  Apart from minor clean up this gets rid of proc unlock/lock cycle on thread
  exit to work around LOR against tidhash lock.

Modified:
  head/sys/kern/init_main.c
  head/sys/kern/kern_kthread.c
  head/sys/kern/kern_thr.c
  head/sys/kern/kern_thread.c
  head/sys/sys/proc.h

Modified: head/sys/kern/init_main.c
==============================================================================
--- head/sys/kern/init_main.c	Wed Nov 11 08:48:43 2020	(r367583)
+++ head/sys/kern/init_main.c	Wed Nov 11 08:50:04 2020	(r367584)
@@ -497,7 +497,7 @@ proc0_init(void *dummy __unused)
 	STAILQ_INIT(&p->p_ktr);
 	p->p_nice = NZERO;
 	td->td_tid = THREAD0_TID;
-	LIST_INSERT_HEAD(TIDHASH(td->td_tid), td, td_hash);
+	tidhash_add(td);
 	td->td_state = TDS_RUNNING;
 	td->td_pri_class = PRI_TIMESHARE;
 	td->td_user_pri = PUSER;

Modified: head/sys/kern/kern_kthread.c
==============================================================================
--- head/sys/kern/kern_kthread.c	Wed Nov 11 08:48:43 2020	(r367583)
+++ head/sys/kern/kern_kthread.c	Wed Nov 11 08:50:04 2020	(r367584)
@@ -350,15 +350,12 @@ kthread_exit(void)
 	 * The last exiting thread in a kernel process must tear down
 	 * the whole process.
 	 */
-	rw_wlock(&tidhash_lock);
 	PROC_LOCK(p);
 	if (p->p_numthreads == 1) {
 		PROC_UNLOCK(p);
-		rw_wunlock(&tidhash_lock);
 		kproc_exit(0);
 	}
-	LIST_REMOVE(td, td_hash);
-	rw_wunlock(&tidhash_lock);
+	tidhash_remove(td);
 	umtx_thread_exit(td);
 	tdsigcleanup(td);
 	PROC_SLOCK(p);

Modified: head/sys/kern/kern_thr.c
==============================================================================
--- head/sys/kern/kern_thr.c	Wed Nov 11 08:48:43 2020	(r367583)
+++ head/sys/kern/kern_thr.c	Wed Nov 11 08:50:04 2020	(r367584)
@@ -353,14 +353,13 @@ kern_thr_exit(struct thread *td)
 		return (0);
 	}
 
-	p->p_pendingexits++;
 	td->td_dbgflags |= TDB_EXIT;
-	if (p->p_ptevents & PTRACE_LWP)
+	if (p->p_ptevents & PTRACE_LWP) {
+		p->p_pendingexits++;
 		ptracestop(td, SIGTRAP, NULL);
-	PROC_UNLOCK(p);
+		p->p_pendingexits--;
+	}
 	tidhash_remove(td);
-	PROC_LOCK(p);
-	p->p_pendingexits--;
 
 	/*
 	 * The check above should prevent all other threads from this

Modified: head/sys/kern/kern_thread.c
==============================================================================
--- head/sys/kern/kern_thread.c	Wed Nov 11 08:48:43 2020	(r367583)
+++ head/sys/kern/kern_thread.c	Wed Nov 11 08:50:04 2020	(r367584)
@@ -147,9 +147,10 @@ SYSCTL_INT(_kern, OID_AUTO, maxthread, CTLFLAG_RDTUN,
 
 static int nthreads;
 
-struct	tidhashhead *tidhashtbl;
-u_long	tidhash;
-struct	rwlock tidhash_lock;
+static LIST_HEAD(tidhashhead, thread) *tidhashtbl;
+static u_long	tidhash;
+static struct	rwlock tidhash_lock;
+#define	TIDHASH(tid)	(&tidhashtbl[(tid) & tidhash])
 
 EVENTHANDLER_LIST_DEFINE(thread_ctor);
 EVENTHANDLER_LIST_DEFINE(thread_dtor);
@@ -1329,13 +1330,65 @@ thread_single_end(struct proc *p, int mode)
 		kick_proc0();
 }
 
-/* Locate a thread by number; return with proc lock held. */
+/*
+ * Locate a thread by number and return with proc lock held.
+ *
+ * thread exit establishes proc -> tidhash lock ordering, but lookup
+ * takes tidhash first and needs to return locked proc.
+ *
+ * The problem is worked around by relying on type-safety of both
+ * structures and doing the work in 2 steps:
+ * - tidhash-locked lookup which saves both thread and proc pointers
+ * - proc-locked verification that the found thread still matches
+ */
+static bool
+tdfind_hash(lwpid_t tid, pid_t pid, struct proc **pp, struct thread **tdp)
+{
+#define RUN_THRESH	16
+	struct proc *p;
+	struct thread *td;
+	int run;
+	bool locked;
+
+	run = 0;
+	rw_rlock(&tidhash_lock);
+	locked = true;
+	LIST_FOREACH(td, TIDHASH(tid), td_hash) {
+		if (td->td_tid != tid) {
+			run++;
+			continue;
+		}
+		p = td->td_proc;
+		if (pid != -1 && p->p_pid != pid) {
+			td = NULL;
+			break;
+		}
+		if (run > RUN_THRESH) {
+			if (rw_try_upgrade(&tidhash_lock)) {
+				LIST_REMOVE(td, td_hash);
+				LIST_INSERT_HEAD(TIDHASH(td->td_tid),
+					td, td_hash);
+				rw_wunlock(&tidhash_lock);
+				locked = false;
+				break;
+			}
+		}
+		break;
+	}
+	if (locked)
+		rw_runlock(&tidhash_lock);
+	if (td == NULL)
+		return (false);
+	*pp = p;
+	*tdp = td;
+	return (true);
+}
+
 struct thread *
 tdfind(lwpid_t tid, pid_t pid)
 {
-#define RUN_THRESH	16
+	struct proc *p;
 	struct thread *td;
-	int run = 0;
 
 	td = curthread;
 	if (td->td_tid == tid) {
@@ -1345,34 +1398,24 @@ tdfind(lwpid_t tid, pid_t pid)
 		return (td);
 	}
 
-	rw_rlock(&tidhash_lock);
-	LIST_FOREACH(td, TIDHASH(tid), td_hash) {
-		if (td->td_tid == tid) {
-			if (pid != -1 && td->td_proc->p_pid != pid) {
-				td = NULL;
-				break;
-			}
-			PROC_LOCK(td->td_proc);
-			if (td->td_proc->p_state == PRS_NEW) {
-				PROC_UNLOCK(td->td_proc);
-				td = NULL;
-				break;
-			}
-			if (run > RUN_THRESH) {
-				if (rw_try_upgrade(&tidhash_lock)) {
-					LIST_REMOVE(td, td_hash);
-					LIST_INSERT_HEAD(TIDHASH(td->td_tid),
-						td, td_hash);
-					rw_wunlock(&tidhash_lock);
-					return (td);
-				}
-			}
-			break;
+	for (;;) {
+		if (!tdfind_hash(tid, pid, &p, &td))
+			return (NULL);
+		PROC_LOCK(p);
+		if (td->td_tid != tid) {
+			PROC_UNLOCK(p);
+			continue;
 		}
-		run++;
+		if (td->td_proc != p) {
+			PROC_UNLOCK(p);
+			continue;
+		}
+		if (p->p_state == PRS_NEW) {
+			PROC_UNLOCK(p);
+			return (NULL);
+		}
+		return (td);
 	}
-	rw_runlock(&tidhash_lock);
-	return (td);
 }
 
 void

Modified: head/sys/sys/proc.h
==============================================================================
--- head/sys/sys/proc.h	Wed Nov 11 08:48:43 2020	(r367583)
+++ head/sys/sys/proc.h	Wed Nov 11 08:50:04 2020	(r367584)
@@ -968,10 +968,6 @@ extern LIST_HEAD(pidhashhead, proc) *pidhashtbl;
 extern struct sx *pidhashtbl_lock;
 extern u_long pidhash;
 extern u_long pidhashlock;
-#define	TIDHASH(tid)	(&tidhashtbl[(tid) & tidhash])
-extern LIST_HEAD(tidhashhead, thread) *tidhashtbl;
-extern u_long tidhash;
-extern struct rwlock tidhash_lock;
 
 #define	PGRPHASH(pgid)	(&pgrphashtbl[(pgid) & pgrphash])
 extern LIST_HEAD(pgrphashhead, pgrp) *pgrphashtbl;

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202011110850.0AB8o4VA062275>