aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMateusz Guzik <mjg@FreeBSD.org>2020-11-11 08:50:04 +0000
committerMateusz Guzik <mjg@FreeBSD.org>2020-11-11 08:50:04 +0000
commitaae3547be3f9f3072cf41ed1368a488defada7c7 (patch)
treee3bc42746511e6bee745afb643b85ccf7eb360e5
parentcf31cadeb6b96b86cc8619a0a849f8e61b479a9a (diff)
downloadsrc-aae3547be3f9f3072cf41ed1368a488defada7c7.tar.gz
src-aae3547be3f9f3072cf41ed1368a488defada7c7.zip
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.
Notes
Notes: svn path=/head/; revision=367584
-rw-r--r--sys/kern/init_main.c2
-rw-r--r--sys/kern/kern_kthread.c5
-rw-r--r--sys/kern/kern_thr.c9
-rw-r--r--sys/kern/kern_thread.c107
-rw-r--r--sys/sys/proc.h4
5 files changed, 81 insertions, 46 deletions
diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c
index 49e353ddad47..9c81e0c9f909 100644
--- a/sys/kern/init_main.c
+++ b/sys/kern/init_main.c
@@ -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;
diff --git a/sys/kern/kern_kthread.c b/sys/kern/kern_kthread.c
index 092d0b9a0e0f..5cc4bd275bcd 100644
--- a/sys/kern/kern_kthread.c
+++ b/sys/kern/kern_kthread.c
@@ -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);
diff --git a/sys/kern/kern_thr.c b/sys/kern/kern_thr.c
index 399d4d8b869e..f3e4e3649855 100644
--- a/sys/kern/kern_thr.c
+++ b/sys/kern/kern_thr.c
@@ -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
diff --git a/sys/kern/kern_thread.c b/sys/kern/kern_thread.c
index 1408180b8327..42a9641f7b8b 100644
--- a/sys/kern/kern_thread.c
+++ b/sys/kern/kern_thread.c
@@ -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;
+ }
+ if (td->td_proc != p) {
+ PROC_UNLOCK(p);
+ continue;
}
- run++;
+ if (p->p_state == PRS_NEW) {
+ PROC_UNLOCK(p);
+ return (NULL);
+ }
+ return (td);
}
- rw_runlock(&tidhash_lock);
- return (td);
}
void
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 47669d4af23a..474dc0f2c5b7 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -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;