On Sun, 23 Apr 2017 04:30:01 -0700 tip-bot for Thomas Gleixner <[email protected]> wrote:
> Commit-ID: 24db7a671bd5eea76b17138b976eb9a4072f1b7a > Gitweb: http://git.kernel.org/tip/24db7a671bd5eea76b17138b976eb9a4072f1b7a > Author: Thomas Gleixner <[email protected]> > AuthorDate: Sun, 23 Apr 2017 12:17:13 +0200 > Committer: Thomas Gleixner <[email protected]> > CommitDate: Sun, 23 Apr 2017 13:24:34 +0200 > > trace/perf: Cure hotplug lock ordering issues > > The get_online_cpus() rework unearthed lock ordering issues. > > Reorder hotpluglock and event_mutex() to avoid circular locking > dependencies and convert static_key_slow_dec() to the cpuslocked version to > avoid hotplug lock recursion. > > That also requires to protect the call to perf_trace_destroy() against cpu > hotplug. > > Signed-off-by: Thomas Gleixner <[email protected]> > Cc: Peter Zijlstra <[email protected]> > Cc: Steven Rostedt <[email protected]> BTW, I only got an email with the tip commit. Was this sent out before committing to tip? > > --- > kernel/events/core.c | 2 ++ > kernel/trace/trace_events.c | 6 ++++++ > kernel/tracepoint.c | 4 ++-- > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index b5b4f52f..997123c 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -7783,7 +7783,9 @@ EXPORT_SYMBOL_GPL(perf_tp_event); > > static void tp_perf_event_destroy(struct perf_event *event) > { > + get_online_cpus(); > perf_trace_destroy(event); > + put_online_cpus(); > } OK, so I merged the branch I had Linus pull into the commit just before this one in tip. Then I ran all my tracing tests and not one triggers any lockdep issues. Just to make sure this was still an issue, as soon as I ran: # perf record -e sched:sched_switch -a sleep 1 It triggered: [ 198.228443] ====================================================== [ 198.235892] [ INFO: possible circular locking dependency detected ] [ 198.243406] 4.11.0-test+ #541 Not tainted [ 198.248657] ------------------------------------------------------- [ 198.256144] perf/4027 is trying to acquire lock: [ 198.261972] (event_mutex){+.+.+.}, at: [<ffffffff81195713>] perf_trace_init+0x23/0x2d0 [ 198.271162] [ 198.271162] but task is already holding lock: [ 198.279353] (cpu_hotplug_lock.rw_sem){++++++}, at: [<ffffffff811b8280>] SYSC_perf_event_open+0x3c0/0x10f0 The point being, there's no place that holds event_mutex that actually requires having get_online_cpus(), besides perf. And I don't think perf needs it here either. It probably needs it for the hardware events, but does it really need it for the software ones? Is there a way to separate software event initialization from hardware ones? Or does it really need to have get_online_cpus() when enabling software events. Can we have it as a delayed case? Enable them after put_online_cpus() and disable them before the call to get_online_cpus()? I'm thinking it will be a hell of a lot easier to make perf not need to have get_online_cpus() taken when calling the tp events. Have it separated out if possible. Otherwise, you are going to be playing whack a mole for a long time if you want to inverse event_mutex with get_online_cpus(). -- Steve > > static int perf_tp_event_init(struct perf_event *event) > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c > index 9311654..8156bfd 100644 > --- a/kernel/trace/trace_events.c > +++ b/kernel/trace/trace_events.c > @@ -747,9 +747,11 @@ static int __ftrace_set_clr_event(struct trace_array > *tr, const char *match, > { > int ret; > > + get_online_cpus(); > mutex_lock(&event_mutex); > ret = __ftrace_set_clr_event_nolock(tr, match, sub, event, set); > mutex_unlock(&event_mutex); > + put_online_cpus(); > > return ret; > } > @@ -1039,11 +1041,13 @@ event_enable_write(struct file *filp, const char > __user *ubuf, size_t cnt, > case 0: > case 1: > ret = -ENODEV; > + get_online_cpus(); > mutex_lock(&event_mutex); > file = event_file_data(filp); > if (likely(file)) > ret = ftrace_event_enable_disable(file, val); > mutex_unlock(&event_mutex); > + put_online_cpus(); > break; > > default: > @@ -2902,6 +2906,7 @@ early_event_add_tracer(struct dentry *parent, struct > trace_array *tr) > > int event_trace_del_tracer(struct trace_array *tr) > { > + get_online_cpus(); > mutex_lock(&event_mutex); > > /* Disable any event triggers and associated soft-disabled events */ > @@ -2924,6 +2929,7 @@ int event_trace_del_tracer(struct trace_array *tr) > tr->event_dir = NULL; > > mutex_unlock(&event_mutex); > + put_online_cpus(); > > return 0; > } > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > index 685c50a..10ce910 100644 > --- a/kernel/tracepoint.c > +++ b/kernel/tracepoint.c > @@ -220,7 +220,7 @@ static int tracepoint_add_func(struct tracepoint *tp, > */ > rcu_assign_pointer(tp->funcs, tp_funcs); > if (!static_key_enabled(&tp->key)) > - static_key_slow_inc(&tp->key); > + static_key_slow_inc_cpuslocked(&tp->key); > release_probes(old); > return 0; > } > @@ -250,7 +250,7 @@ static int tracepoint_remove_func(struct tracepoint *tp, > tp->unregfunc(); > > if (static_key_enabled(&tp->key)) > - static_key_slow_dec(&tp->key); > + static_key_slow_dec_cpuslocked(&tp->key); > } > rcu_assign_pointer(tp->funcs, tp_funcs); > release_probes(old);
