Hi Eric,
The patch didn't improve lock contention.
PerfTop: 48925 irqs/sec kernel:95.6% exact: 100.0% lost: 0/0
drop: 0/0 [4000Hz cycles], (all, 104 CPUs)
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
69.66% [kernel] [k]
native_queued_spin_lock_slowpath
1.93% [kernel] [k]
_raw_spin_lock
1.24% [kernel] [k]
page_counter_cancel
0.70% [kernel] [k]
do_syscall_64
0.62% [kernel] [k]
find_idlest_group.isra.96
0.57% [kernel] [k]
queued_write_lock_slowpath
0.56% [kernel] [k] d_walk
0.45% [kernel] [k]
clear_page_erms
0.44% [kernel] [k]
syscall_return_via_sysret
0.40% [kernel] [k]
entry_SYSCALL_64
0.38% [kernel] [k]
refcount_dec_not_one
0.37% [kernel] [k]
propagate_protected_usage
0.33% [kernel] [k]
unmap_page_range
0.33% [kernel] [k]
select_collect
0.32% [kernel] [k] memcpy_erms
0.30% [kernel] [k]
proc_task_readdir
0.27% [kernel] [k]
_raw_spin_lock_irqsave
Thanks,
Junxiao.
On 6/19/20 7:09 AM, ebiederm@xxxxxxxxxxxx wrote:
Junxiao Bi <junxiao.bi@xxxxxxxxxx> reported:
When debugging some performance issue, i found that thousands of threads exit
around same time could cause a severe spin lock contention on proc dentry
"/proc/$parent_process_pid/task/", that's because threads needs to clean up
their pid file from that dir when exit.
Matthew Wilcox <willy@xxxxxxxxxxxxx> reported:
We've looked at a few different ways of fixing this problem.
The flushing of the proc dentries from the dcache is an optmization,
and is not necessary for correctness. Eventually cache pressure will
cause the dentries to be freed even if no flushing happens. Some
light testing when I refactored the proc flushg[1] indicated that at
least the memory footprint is easily measurable.
An optimization that causes a performance problem due to a thundering
herd of threads is no real optimization.
Modify the code to only flush the /proc/<tgid>/ directory when all
threads in a process are killed at once. This continues to flush
practically everything when the process is reaped as the threads live
under /proc/<tgid>/task/<tid>.
There is a rare possibility that a debugger will access /proc/<tid>/,
which this change will no longer flush, but I believe such accesses
are sufficiently rare to not be observed in practice.
[1] 7bc3e6e55acf ("proc: Use a list of inodes to flush from proc")
Link: https://lkml.kernel.org/r/54091fc0-ca46-2186-97a8-d1f3c4f3877b@xxxxxxxxxx
Reported-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
Reported-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
---
I am still waiting for word on how this affects performance, but this is
a clean version that should avoid the thundering herd problem in
general.
kernel/exit.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index cebae77a9664..567354550d62 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -151,8 +151,8 @@ void put_task_struct_rcu_user(struct task_struct *task)
void release_task(struct task_struct *p)
{
+ struct pid *flush_pid = NULL;
struct task_struct *leader;
- struct pid *thread_pid;
int zap_leader;
repeat:
/* don't need to get the RCU readlock here - the process is dead and
@@ -165,7 +165,16 @@ void release_task(struct task_struct *p)
write_lock_irq(&tasklist_lock);
ptrace_release_task(p);
- thread_pid = get_pid(p->thread_pid);
+
+ /*
+ * When all of the threads are exiting wait until the end
+ * and flush everything.
+ */
+ if (thread_group_leader(p))
+ flush_pid = get_pid(task_tgid(p));
+ else if (!(p->signal->flags & SIGNAL_GROUP_EXIT))
+ flush_pid = get_pid(task_pid(p));
+
__exit_signal(p);
/*
@@ -188,8 +197,10 @@ void release_task(struct task_struct *p)
}
write_unlock_irq(&tasklist_lock);
- proc_flush_pid(thread_pid);
- put_pid(thread_pid);
+ if (flush_pid) {
+ proc_flush_pid(flush_pid);
+ put_pid(flush_pid);
+ }
release_thread(p);
put_task_struct_rcu_user(p);
