On Sun, Mar 23, 2003 at 05:30:39PM +0000, Manfred Spraul wrote:
> __unhash_process acquires the dcache_lock while holding the tasklist_lock
> for writing. AFAICS this can deadlock:
>
> CPU1:
> [ in release_task() ]
> write_lock_irq(&tasklist_lock);
> __unhash_process()
>
> CPU2:
> spin_lock(&dcache_lock);
> <interrupt>
> read_lock(&tasklist_lock);, // e.g. for signal delivery
> ** spins, lock held by cpu 1 for writing.
>
> CPU1:
> [ within __unhash_process() ]
> spin_lock(&dcache_lock);
> ** spins, lock held by CPU2
>
> Probably the callers of __unhash_process must acquire the dcache_lock
> before write_lock_irq(&tasklist_lock).
>
> Any other ideas how to fix the deadlock?
> --
> Manfred
Hello Manfred, Andrew
How is the patch below?.. just some re-arrangement of code. I don't think
we need task_list lock while cleaning up the proc_dentry.
Same problem is there in fs/exec.c:de_thread(). So this is a common solution.
The patch does following things:
0. brings cleaning up of proc_dentry outside tasklist_lock protection.
1. gets rid of code duplication in __unhash_process() and clean_proc_dentry()
2. combines clean_proc_dentry() and put_proc_dentry()
3. makes clean_proc_dentry() extern
fs/exec.c | 31 +++++++++++++------------------
include/linux/proc_fs.h | 1 +
kernel/exit.c | 28 ++++++++--------------------
3 files changed, 22 insertions(+), 38 deletions(-)
diff -urN linux-2.5.65-base/fs/exec.c linux-2.5.65-unhash_process/fs/exec.c
--- linux-2.5.65-base/fs/exec.c 2003-03-18 03:14:03.000000000 +0530
+++ linux-2.5.65-unhash_process/fs/exec.c 2003-03-24 16:49:30.000000000 +0530
@@ -529,27 +529,20 @@
return 0;
}
-static struct dentry *clean_proc_dentry(struct task_struct *p)
+void clean_proc_dentry(struct dentry * proc_dentry)
{
- struct dentry *proc_dentry = p->proc_dentry;
if (proc_dentry) {
spin_lock(&dcache_lock);
if (!d_unhashed(proc_dentry)) {
dget_locked(proc_dentry);
__d_drop(proc_dentry);
- } else
- proc_dentry = NULL;
- spin_unlock(&dcache_lock);
- }
- return proc_dentry;
-}
-
-static inline void put_proc_dentry(struct dentry *dentry)
-{
- if (dentry) {
- shrink_dcache_parent(dentry);
- dput(dentry);
+ spin_unlock(&dcache_lock);
+ shrink_dcache_parent(proc_dentry);
+ dput(proc_dentry);
+ }
+ else
+ spin_unlock(&dcache_lock);
}
}
@@ -661,8 +654,8 @@
yield();
write_lock_irq(&tasklist_lock);
- proc_dentry1 = clean_proc_dentry(current);
- proc_dentry2 = clean_proc_dentry(leader);
+ proc_dentry1 = current->proc_dentry;
+ proc_dentry2 = leader->proc_dentry;
if (leader->tgid != current->tgid)
BUG();
@@ -703,8 +696,10 @@
write_unlock_irq(&tasklist_lock);
- put_proc_dentry(proc_dentry1);
- put_proc_dentry(proc_dentry2);
+ if (proc_dentry1)
+ clean_proc_dentry(proc_dentry1);
+ if (proc_dentry2)
+ clean_proc_dentry(proc_dentry2);
if (state != TASK_ZOMBIE)
BUG();
diff -urN linux-2.5.65-base/include/linux/proc_fs.h linux-2.5.65-unhash_process/include/linux/proc_fs.h
--- linux-2.5.65-base/include/linux/proc_fs.h 2003-03-18 03:13:37.000000000 +0530
+++ linux-2.5.65-unhash_process/include/linux/proc_fs.h 2003-03-24 16:47:10.000000000 +0530
@@ -224,4 +224,5 @@
return PROC_I(inode)->pde;
}
+extern void clean_proc_dentry(struct dentry *proc_dentry);
#endif /* _LINUX_PROC_FS_H */
diff -urN linux-2.5.65-base/kernel/exit.c linux-2.5.65-unhash_process/kernel/exit.c
--- linux-2.5.65-base/kernel/exit.c 2003-03-18 03:14:43.000000000 +0530
+++ linux-2.5.65-unhash_process/kernel/exit.c 2003-03-24 16:46:40.000000000 +0530
@@ -21,6 +21,7 @@
#include <linux/ptrace.h>
#include <linux/profile.h>
#include <linux/mount.h>
+#include <linux/proc_fs.h>
#include <asm/uaccess.h>
#include <asm/pgtable.h>
@@ -33,7 +34,6 @@
static struct dentry * __unhash_process(struct task_struct *p)
{
- struct dentry *proc_dentry;
nr_threads--;
detach_pid(p, PIDTYPE_PID);
@@ -46,17 +46,8 @@
}
REMOVE_LINKS(p);
- proc_dentry = p->proc_dentry;
- if (unlikely(proc_dentry != NULL)) {
- spin_lock(&dcache_lock);
- if (!d_unhashed(proc_dentry)) {
- dget_locked(proc_dentry);
- __d_drop(proc_dentry);
- } else
- proc_dentry = NULL;
- spin_unlock(&dcache_lock);
- }
- return proc_dentry;
+
+ return p->proc_dentry;
}
void release_task(struct task_struct * p)
@@ -93,10 +84,9 @@
sched_exit(p);
write_unlock_irq(&tasklist_lock);
- if (unlikely(proc_dentry != NULL)) {
- shrink_dcache_parent(proc_dentry);
- dput(proc_dentry);
- }
+ if (unlikely(proc_dentry != NULL))
+ clean_proc_dentry(proc_dentry);
+
release_thread(p);
put_task_struct(p);
}
@@ -111,10 +101,8 @@
proc_dentry = __unhash_process(p);
write_unlock_irq(&tasklist_lock);
- if (unlikely(proc_dentry != NULL)) {
- shrink_dcache_parent(proc_dentry);
- dput(proc_dentry);
- }
+ if (unlikely(proc_dentry != NULL))
+ clean_proc_dentry(proc_dentry);
}
/*
-- Maneesh Soni IBM Linux Technology Center, IBM India Software Lab, Bangalore. Phone: +91-80-5044999 email: maneesh@in.ibm.com http://lse.sourceforge.net/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Mon Mar 31 2003 - 22:00:15 EST