From fc8c4c4840e0181daf89df75894137a8fb722a85 Mon Sep 17 00:00:00 2001 From: Suzuki K Poulose Date: Thu, 20 Sep 2018 13:17:48 -0600 Subject: [PATCH] UPSTREAM: coresight: perf: Avoid unncessary CPU hotplug read lock (Upstream commit c48fb3bbe912a295e5b75eaabaf39874d5b9b773). We hold the read lock on CPU hotplug to simply copy the online mask, which is not really needed. And this can cause a lockdep warning, like : [ 54.632093] ====================================================== [ 54.638207] WARNING: possible circular locking dependency detected [ 54.644322] 4.18.0-rc3-00042-g2d39e6356bb7-dirty #309 Not tainted [ 54.650350] ------------------------------------------------------ [ 54.656464] perf/2862 is trying to acquire lock: [ 54.661031] 000000007e21d170 (&event->mmap_mutex){+.+.}, at: perf_event_set_output+0x98/0x138 [ 54.669486] [ 54.669486] but task is already holding lock: [ 54.675256] 000000001080eb1b (&cpuctx_mutex){+.+.}, at: perf_event_ctx_lock_nested+0xf8/0x1f0 [ 54.683704] [ 54.683704] which lock already depends on the new lock. [ 54.683704] [ 54.691797] [ 54.691797] the existing dependency chain (in reverse order) is: [ 54.699201] [ 54.699201] -> #3 (&cpuctx_mutex){+.+.}: [ 54.704556] __mutex_lock+0x70/0x808 [ 54.708608] mutex_lock_nested+0x1c/0x28 [ 54.713005] perf_event_init_cpu+0x8c/0xd8 [ 54.717574] perf_event_init+0x194/0x1d4 [ 54.721971] start_kernel+0x2b8/0x42c [ 54.726107] [ 54.726107] -> #2 (pmus_lock){+.+.}: [ 54.731114] __mutex_lock+0x70/0x808 [ 54.735165] mutex_lock_nested+0x1c/0x28 [ 54.739560] perf_event_init_cpu+0x30/0xd8 [ 54.744129] cpuhp_invoke_callback+0x84/0x248 [ 54.748954] _cpu_up+0xe8/0x1c8 [ 54.752576] do_cpu_up+0xa8/0xc8 [ 54.756283] cpu_up+0x10/0x18 [ 54.759731] smp_init+0xa0/0x114 [ 54.763438] kernel_init_freeable+0x120/0x288 [ 54.768264] kernel_init+0x10/0x108 [ 54.772230] ret_from_fork+0x10/0x18 [ 54.776279] [ 54.776279] -> #1 (cpu_hotplug_lock.rw_sem){++++}: [ 54.782492] cpus_read_lock+0x34/0xb0 [ 54.786631] etm_setup_aux+0x5c/0x308 [ 54.790769] rb_alloc_aux+0x1ec/0x300 [ 54.794906] perf_mmap+0x284/0x610 [ 54.798787] mmap_region+0x388/0x570 [ 54.802838] do_mmap+0x344/0x4f8 [ 54.806544] vm_mmap_pgoff+0xe4/0x110 [ 54.810682] ksys_mmap_pgoff+0xa8/0x240 [ 54.814992] sys_mmap+0x18/0x28 [ 54.818613] el0_svc_naked+0x30/0x34 [ 54.822661] [ 54.822661] -> #0 (&event->mmap_mutex){+.+.}: [ 54.828445] lock_acquire+0x48/0x68 [ 54.832409] __mutex_lock+0x70/0x808 [ 54.836459] mutex_lock_nested+0x1c/0x28 [ 54.840855] perf_event_set_output+0x98/0x138 [ 54.845680] _perf_ioctl+0x2a0/0x6a0 [ 54.849731] perf_ioctl+0x3c/0x68 [ 54.853526] do_vfs_ioctl+0xb8/0xa20 [ 54.857577] ksys_ioctl+0x80/0xb8 [ 54.861370] sys_ioctl+0xc/0x18 [ 54.864990] el0_svc_naked+0x30/0x34 [ 54.869039] [ 54.869039] other info that might help us debug this: [ 54.869039] [ 54.876960] Chain exists of: [ 54.876960] &event->mmap_mutex --> pmus_lock --> &cpuctx_mutex [ 54.876960] [ 54.887217] Possible unsafe locking scenario: [ 54.887217] [ 54.893073] CPU0 CPU1 [ 54.897552] ---- ---- [ 54.902030] lock(&cpuctx_mutex); [ 54.905396] lock(pmus_lock); [ 54.910911] lock(&cpuctx_mutex); [ 54.916770] lock(&event->mmap_mutex); [ 54.920566] [ 54.920566] *** DEADLOCK *** [ 54.920566] [ 54.926424] 1 lock held by perf/2862: [ 54.930042] #0: 000000001080eb1b (&cpuctx_mutex){+.+.}, at: perf_event_ctx_lock_nested+0xf8/0x1f0 Since we have per-cpu array for the paths, we simply don't care about the number of online CPUs. This patch gets rid of the {get/put}_online_cpus(). Reported-by: Mathieu Poirier Signed-off-by: Suzuki K Poulose Signed-off-by: Mathieu Poirier Signed-off-by: Greg Kroah-Hartman Bug: 140266694 Change-Id: I10f697b77051b511c6b3af1219e5835fb0cf4d28 Signed-off-by: Yabin Cui --- drivers/hwtracing/coresight/coresight-etm-perf.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 4b53d55788a0..2f23dc6e2e04 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -161,15 +161,12 @@ static void *alloc_event_data(int cpu) if (!event_data) return NULL; - /* Make sure nothing disappears under us */ - get_online_cpus(); mask = &event_data->mask; if (cpu != -1) cpumask_set_cpu(cpu, mask); else cpumask_copy(mask, cpu_online_mask); - put_online_cpus(); /* * Each CPU has a single path between source and destination. As such