Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1649230 > unrolled thread
| Started by | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| First post | 2017-05-24 10:30 +0200 |
| Last post | 2017-05-26 10:50 +0200 |
| Articles | 7 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[patch V3 23/32] perf/tracing/cpuhotplug: Fix locking order Thomas Gleixner <tglx@linutronix.de> - 2017-05-24 10:30 +0200
Re: [patch V3 23/32] perf/tracing/cpuhotplug: Fix locking order "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-24 20:40 +0200
Re: [patch V3 23/32] perf/tracing/cpuhotplug: Fix locking order Thomas Gleixner <tglx@linutronix.de> - 2017-05-24 20:50 +0200
Re: [patch V3 23/32] perf/tracing/cpuhotplug: Fix locking order "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-24 23:20 +0200
Re: [patch V3 23/32] perf/tracing/cpuhotplug: Fix locking order Peter Zijlstra <peterz@infradead.org> - 2017-05-30 13:30 +0200
Re: [patch V3 23/32] perf/tracing/cpuhotplug: Fix locking order "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-30 18:30 +0200
[tip:smp/hotplug] perf/tracing/cpuhotplug: Fix locking order tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-05-26 10:50 +0200
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-05-24 10:30 +0200 |
| Subject | [patch V3 23/32] perf/tracing/cpuhotplug: Fix locking order |
| Message-ID | <tKzQ6-8hN-5@gated-at.bofh.it> |
perf, tracing, kprobes and jump_labels have a gazillion of ways to create
dependency lock chains. Some of those involve nested invocations of
get_online_cpus().
The conversion of the hotplug locking to a percpu rwsem requires to avoid
such nested calls. sys_perf_event_open() protects most of the syscall logic
against cpu hotplug. This causes nested calls and lock inversions versus
ftrace and kprobes in various interesting ways.
It's impossible to move the hotplug locking to the outer end of all call
chains in the involved facilities, so the hotplug protection in
sys_perf_event_open() needs to be solved differently.
Introduce 'pmus_mutex' which protects a perf private online cpumask. This
mutex is taken when the mask is updated in the cpu hotplug callbacks and
can be taken in sys_perf_event_open() to protect the swhash setup/teardown
code and when the final judgement about a valid event has to be made.
[ tglx: Produced changelog and fixed the swhash interaction ]
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
include/linux/perf_event.h | 2
kernel/events/core.c | 106 ++++++++++++++++++++++++++++++++-------------
2 files changed, 78 insertions(+), 30 deletions(-)
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -801,6 +801,8 @@ struct perf_cpu_context {
struct list_head sched_cb_entry;
int sched_cb_usage;
+
+ int online;
};
struct perf_output_handle {
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -389,6 +389,7 @@ static atomic_t nr_switch_events __read_
static LIST_HEAD(pmus);
static DEFINE_MUTEX(pmus_lock);
static struct srcu_struct pmus_srcu;
+static cpumask_var_t perf_online_mask;
/*
* perf event paranoia level:
@@ -3812,14 +3813,6 @@ find_get_context(struct pmu *pmu, struct
if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
return ERR_PTR(-EACCES);
- /*
- * We could be clever and allow to attach a event to an
- * offline CPU and activate it when the CPU comes up, but
- * that's for later.
- */
- if (!cpu_online(cpu))
- return ERR_PTR(-ENODEV);
-
cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
ctx = &cpuctx->ctx;
get_ctx(ctx);
@@ -7703,7 +7696,8 @@ static int swevent_hlist_get_cpu(int cpu
int err = 0;
mutex_lock(&swhash->hlist_mutex);
- if (!swevent_hlist_deref(swhash) && cpu_online(cpu)) {
+ if (!swevent_hlist_deref(swhash) &&
+ cpumask_test_cpu(cpu, perf_online_mask)) {
struct swevent_hlist *hlist;
hlist = kzalloc(sizeof(*hlist), GFP_KERNEL);
@@ -7724,7 +7718,7 @@ static int swevent_hlist_get(void)
{
int err, cpu, failed_cpu;
- get_online_cpus();
+ mutex_lock(&pmus_lock);
for_each_possible_cpu(cpu) {
err = swevent_hlist_get_cpu(cpu);
if (err) {
@@ -7732,8 +7726,7 @@ static int swevent_hlist_get(void)
goto fail;
}
}
- put_online_cpus();
-
+ mutex_unlock(&pmus_lock);
return 0;
fail:
for_each_possible_cpu(cpu) {
@@ -7741,8 +7734,7 @@ static int swevent_hlist_get(void)
break;
swevent_hlist_put_cpu(cpu);
}
-
- put_online_cpus();
+ mutex_unlock(&pmus_lock);
return err;
}
@@ -8920,7 +8912,7 @@ perf_event_mux_interval_ms_store(struct
pmu->hrtimer_interval_ms = timer;
/* update all cpuctx for this PMU */
- get_online_cpus();
+ cpus_read_lock();
for_each_online_cpu(cpu) {
struct perf_cpu_context *cpuctx;
cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
@@ -8929,7 +8921,7 @@ perf_event_mux_interval_ms_store(struct
cpu_function_call(cpu,
(remote_function_f)perf_mux_hrtimer_restart, cpuctx);
}
- put_online_cpus();
+ cpus_read_unlock();
mutex_unlock(&mux_interval_mutex);
return count;
@@ -9059,6 +9051,7 @@ int perf_pmu_register(struct pmu *pmu, c
lockdep_set_class(&cpuctx->ctx.mutex, &cpuctx_mutex);
lockdep_set_class(&cpuctx->ctx.lock, &cpuctx_lock);
cpuctx->ctx.pmu = pmu;
+ cpuctx->online = cpumask_test_cpu(cpu, perf_online_mask);
__perf_mux_hrtimer_init(cpuctx, cpu);
}
@@ -9882,12 +9875,10 @@ SYSCALL_DEFINE5(perf_event_open,
goto err_task;
}
- get_online_cpus();
-
if (task) {
err = mutex_lock_interruptible(&task->signal->cred_guard_mutex);
if (err)
- goto err_cpus;
+ goto err_cred;
/*
* Reuse ptrace permission checks for now.
@@ -10073,6 +10064,23 @@ SYSCALL_DEFINE5(perf_event_open,
goto err_locked;
}
+ if (!task) {
+ /*
+ * Check if the @cpu we're creating an event for is online.
+ *
+ * We use the perf_cpu_context::ctx::mutex to serialize against
+ * the hotplug notifiers. See perf_event_{init,exit}_cpu().
+ */
+ struct perf_cpu_context *cpuctx =
+ container_of(ctx, struct perf_cpu_context, ctx);
+
+ if (!cpuctx->online) {
+ err = -ENODEV;
+ goto err_locked;
+ }
+ }
+
+
/*
* Must be under the same ctx::mutex as perf_install_in_context(),
* because we need to serialize with concurrent event creation.
@@ -10162,8 +10170,6 @@ SYSCALL_DEFINE5(perf_event_open,
put_task_struct(task);
}
- put_online_cpus();
-
mutex_lock(¤t->perf_event_mutex);
list_add_tail(&event->owner_entry, ¤t->perf_event_list);
mutex_unlock(¤t->perf_event_mutex);
@@ -10197,8 +10203,6 @@ SYSCALL_DEFINE5(perf_event_open,
err_cred:
if (task)
mutex_unlock(&task->signal->cred_guard_mutex);
-err_cpus:
- put_online_cpus();
err_task:
if (task)
put_task_struct(task);
@@ -10253,6 +10257,21 @@ perf_event_create_kernel_counter(struct
goto err_unlock;
}
+ if (!task) {
+ /*
+ * Check if the @cpu we're creating an event for is online.
+ *
+ * We use the perf_cpu_context::ctx::mutex to serialize against
+ * the hotplug notifiers. See perf_event_{init,exit}_cpu().
+ */
+ struct perf_cpu_context *cpuctx =
+ container_of(ctx, struct perf_cpu_context, ctx);
+ if (!cpuctx->online) {
+ err = -ENODEV;
+ goto err_unlock;
+ }
+ }
+
if (!exclusive_event_installable(event, ctx)) {
err = -EBUSY;
goto err_unlock;
@@ -10920,6 +10939,8 @@ static void __init perf_event_init_all_c
struct swevent_htable *swhash;
int cpu;
+ zalloc_cpumask_var(&perf_online_mask, GFP_KERNEL);
+
for_each_possible_cpu(cpu) {
swhash = &per_cpu(swevent_htable, cpu);
mutex_init(&swhash->hlist_mutex);
@@ -10935,7 +10956,7 @@ static void __init perf_event_init_all_c
}
}
-int perf_event_init_cpu(unsigned int cpu)
+void perf_swevent_init_cpu(unsigned int cpu)
{
struct swevent_htable *swhash = &per_cpu(swevent_htable, cpu);
@@ -10948,7 +10969,6 @@ int perf_event_init_cpu(unsigned int cpu
rcu_assign_pointer(swhash->swevent_hlist, hlist);
}
mutex_unlock(&swhash->hlist_mutex);
- return 0;
}
#if defined CONFIG_HOTPLUG_CPU || defined CONFIG_KEXEC_CORE
@@ -10966,19 +10986,22 @@ static void __perf_event_exit_context(vo
static void perf_event_exit_cpu_context(int cpu)
{
+ struct perf_cpu_context *cpuctx;
struct perf_event_context *ctx;
struct pmu *pmu;
- int idx;
- idx = srcu_read_lock(&pmus_srcu);
- list_for_each_entry_rcu(pmu, &pmus, entry) {
- ctx = &per_cpu_ptr(pmu->pmu_cpu_context, cpu)->ctx;
+ mutex_lock(&pmus_lock);
+ list_for_each_entry(pmu, &pmus, entry) {
+ cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
+ ctx = &cpuctx->ctx;
mutex_lock(&ctx->mutex);
smp_call_function_single(cpu, __perf_event_exit_context, ctx, 1);
+ cpuctx->online = 0;
mutex_unlock(&ctx->mutex);
}
- srcu_read_unlock(&pmus_srcu, idx);
+ cpumask_clear_cpu(cpu, perf_online_mask);
+ mutex_unlock(&pmus_lock);
}
#else
@@ -10986,6 +11009,29 @@ static void perf_event_exit_cpu_context(
#endif
+int perf_event_init_cpu(unsigned int cpu)
+{
+ struct perf_cpu_context *cpuctx;
+ struct perf_event_context *ctx;
+ struct pmu *pmu;
+
+ perf_swevent_init_cpu(cpu);
+
+ mutex_lock(&pmus_lock);
+ cpumask_set_cpu(cpu, perf_online_mask);
+ list_for_each_entry(pmu, &pmus, entry) {
+ cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
+ ctx = &cpuctx->ctx;
+
+ mutex_lock(&ctx->mutex);
+ cpuctx->online = 1;
+ mutex_unlock(&ctx->mutex);
+ }
+ mutex_unlock(&pmus_lock);
+
+ return 0;
+}
+
int perf_event_exit_cpu(unsigned int cpu)
{
perf_event_exit_cpu_context(cpu);
[toc] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-05-24 20:40 +0200 |
| Message-ID | <tKJmp-5SS-9@gated-at.bofh.it> |
| In reply to | #1649230 |
On Wed, May 24, 2017 at 10:15:34AM +0200, Thomas Gleixner wrote:
> perf, tracing, kprobes and jump_labels have a gazillion of ways to create
> dependency lock chains. Some of those involve nested invocations of
> get_online_cpus().
>
> The conversion of the hotplug locking to a percpu rwsem requires to avoid
> such nested calls. sys_perf_event_open() protects most of the syscall logic
> against cpu hotplug. This causes nested calls and lock inversions versus
> ftrace and kprobes in various interesting ways.
>
> It's impossible to move the hotplug locking to the outer end of all call
> chains in the involved facilities, so the hotplug protection in
> sys_perf_event_open() needs to be solved differently.
>
> Introduce 'pmus_mutex' which protects a perf private online cpumask. This
> mutex is taken when the mask is updated in the cpu hotplug callbacks and
> can be taken in sys_perf_event_open() to protect the swhash setup/teardown
> code and when the final judgement about a valid event has to be made.
>
> [ tglx: Produced changelog and fixed the swhash interaction ]
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
One question below about use of cpus_read_lock().
Thanx, Paul
> ---
> include/linux/perf_event.h | 2
> kernel/events/core.c | 106 ++++++++++++++++++++++++++++++++-------------
> 2 files changed, 78 insertions(+), 30 deletions(-)
>
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -801,6 +801,8 @@ struct perf_cpu_context {
>
> struct list_head sched_cb_entry;
> int sched_cb_usage;
> +
> + int online;
> };
>
> struct perf_output_handle {
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -389,6 +389,7 @@ static atomic_t nr_switch_events __read_
> static LIST_HEAD(pmus);
> static DEFINE_MUTEX(pmus_lock);
> static struct srcu_struct pmus_srcu;
> +static cpumask_var_t perf_online_mask;
>
> /*
> * perf event paranoia level:
> @@ -3812,14 +3813,6 @@ find_get_context(struct pmu *pmu, struct
> if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
> return ERR_PTR(-EACCES);
>
> - /*
> - * We could be clever and allow to attach a event to an
> - * offline CPU and activate it when the CPU comes up, but
> - * that's for later.
> - */
> - if (!cpu_online(cpu))
> - return ERR_PTR(-ENODEV);
> -
> cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
> ctx = &cpuctx->ctx;
> get_ctx(ctx);
> @@ -7703,7 +7696,8 @@ static int swevent_hlist_get_cpu(int cpu
> int err = 0;
>
> mutex_lock(&swhash->hlist_mutex);
> - if (!swevent_hlist_deref(swhash) && cpu_online(cpu)) {
> + if (!swevent_hlist_deref(swhash) &&
> + cpumask_test_cpu(cpu, perf_online_mask)) {
> struct swevent_hlist *hlist;
>
> hlist = kzalloc(sizeof(*hlist), GFP_KERNEL);
> @@ -7724,7 +7718,7 @@ static int swevent_hlist_get(void)
> {
> int err, cpu, failed_cpu;
>
> - get_online_cpus();
> + mutex_lock(&pmus_lock);
> for_each_possible_cpu(cpu) {
> err = swevent_hlist_get_cpu(cpu);
> if (err) {
> @@ -7732,8 +7726,7 @@ static int swevent_hlist_get(void)
> goto fail;
> }
> }
> - put_online_cpus();
> -
> + mutex_unlock(&pmus_lock);
> return 0;
> fail:
> for_each_possible_cpu(cpu) {
> @@ -7741,8 +7734,7 @@ static int swevent_hlist_get(void)
> break;
> swevent_hlist_put_cpu(cpu);
> }
> -
> - put_online_cpus();
> + mutex_unlock(&pmus_lock);
> return err;
> }
>
> @@ -8920,7 +8912,7 @@ perf_event_mux_interval_ms_store(struct
> pmu->hrtimer_interval_ms = timer;
>
> /* update all cpuctx for this PMU */
> - get_online_cpus();
> + cpus_read_lock();
OK, I'll bite...
Why is this piece using cpus_read_lock() instead of pmus_lock?
My guess is for the benefit of the cpu_function_call() below, but if
the code instead cycled through the perf_online_mask, wouldn't any
CPU selected be guaranteed to be online?
Or is there some reason that it would be necessary to specially handle
CPUs that perf does not consider to be active, but that are still at
least partway online?
> for_each_online_cpu(cpu) {
> struct perf_cpu_context *cpuctx;
> cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
> @@ -8929,7 +8921,7 @@ perf_event_mux_interval_ms_store(struct
> cpu_function_call(cpu,
> (remote_function_f)perf_mux_hrtimer_restart, cpuctx);
> }
> - put_online_cpus();
> + cpus_read_unlock();
> mutex_unlock(&mux_interval_mutex);
>
> return count;
> @@ -9059,6 +9051,7 @@ int perf_pmu_register(struct pmu *pmu, c
> lockdep_set_class(&cpuctx->ctx.mutex, &cpuctx_mutex);
> lockdep_set_class(&cpuctx->ctx.lock, &cpuctx_lock);
> cpuctx->ctx.pmu = pmu;
> + cpuctx->online = cpumask_test_cpu(cpu, perf_online_mask);
>
> __perf_mux_hrtimer_init(cpuctx, cpu);
> }
> @@ -9882,12 +9875,10 @@ SYSCALL_DEFINE5(perf_event_open,
> goto err_task;
> }
>
> - get_online_cpus();
> -
> if (task) {
> err = mutex_lock_interruptible(&task->signal->cred_guard_mutex);
> if (err)
> - goto err_cpus;
> + goto err_cred;
>
> /*
> * Reuse ptrace permission checks for now.
> @@ -10073,6 +10064,23 @@ SYSCALL_DEFINE5(perf_event_open,
> goto err_locked;
> }
>
> + if (!task) {
> + /*
> + * Check if the @cpu we're creating an event for is online.
> + *
> + * We use the perf_cpu_context::ctx::mutex to serialize against
> + * the hotplug notifiers. See perf_event_{init,exit}_cpu().
> + */
> + struct perf_cpu_context *cpuctx =
> + container_of(ctx, struct perf_cpu_context, ctx);
> +
> + if (!cpuctx->online) {
> + err = -ENODEV;
> + goto err_locked;
> + }
> + }
> +
> +
> /*
> * Must be under the same ctx::mutex as perf_install_in_context(),
> * because we need to serialize with concurrent event creation.
> @@ -10162,8 +10170,6 @@ SYSCALL_DEFINE5(perf_event_open,
> put_task_struct(task);
> }
>
> - put_online_cpus();
> -
> mutex_lock(¤t->perf_event_mutex);
> list_add_tail(&event->owner_entry, ¤t->perf_event_list);
> mutex_unlock(¤t->perf_event_mutex);
> @@ -10197,8 +10203,6 @@ SYSCALL_DEFINE5(perf_event_open,
> err_cred:
> if (task)
> mutex_unlock(&task->signal->cred_guard_mutex);
> -err_cpus:
> - put_online_cpus();
> err_task:
> if (task)
> put_task_struct(task);
> @@ -10253,6 +10257,21 @@ perf_event_create_kernel_counter(struct
> goto err_unlock;
> }
>
> + if (!task) {
> + /*
> + * Check if the @cpu we're creating an event for is online.
> + *
> + * We use the perf_cpu_context::ctx::mutex to serialize against
> + * the hotplug notifiers. See perf_event_{init,exit}_cpu().
> + */
> + struct perf_cpu_context *cpuctx =
> + container_of(ctx, struct perf_cpu_context, ctx);
> + if (!cpuctx->online) {
> + err = -ENODEV;
> + goto err_unlock;
> + }
> + }
> +
> if (!exclusive_event_installable(event, ctx)) {
> err = -EBUSY;
> goto err_unlock;
> @@ -10920,6 +10939,8 @@ static void __init perf_event_init_all_c
> struct swevent_htable *swhash;
> int cpu;
>
> + zalloc_cpumask_var(&perf_online_mask, GFP_KERNEL);
> +
> for_each_possible_cpu(cpu) {
> swhash = &per_cpu(swevent_htable, cpu);
> mutex_init(&swhash->hlist_mutex);
> @@ -10935,7 +10956,7 @@ static void __init perf_event_init_all_c
> }
> }
>
> -int perf_event_init_cpu(unsigned int cpu)
> +void perf_swevent_init_cpu(unsigned int cpu)
> {
> struct swevent_htable *swhash = &per_cpu(swevent_htable, cpu);
>
> @@ -10948,7 +10969,6 @@ int perf_event_init_cpu(unsigned int cpu
> rcu_assign_pointer(swhash->swevent_hlist, hlist);
> }
> mutex_unlock(&swhash->hlist_mutex);
> - return 0;
> }
>
> #if defined CONFIG_HOTPLUG_CPU || defined CONFIG_KEXEC_CORE
> @@ -10966,19 +10986,22 @@ static void __perf_event_exit_context(vo
>
> static void perf_event_exit_cpu_context(int cpu)
> {
> + struct perf_cpu_context *cpuctx;
> struct perf_event_context *ctx;
> struct pmu *pmu;
> - int idx;
>
> - idx = srcu_read_lock(&pmus_srcu);
> - list_for_each_entry_rcu(pmu, &pmus, entry) {
> - ctx = &per_cpu_ptr(pmu->pmu_cpu_context, cpu)->ctx;
> + mutex_lock(&pmus_lock);
> + list_for_each_entry(pmu, &pmus, entry) {
> + cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
> + ctx = &cpuctx->ctx;
>
> mutex_lock(&ctx->mutex);
> smp_call_function_single(cpu, __perf_event_exit_context, ctx, 1);
> + cpuctx->online = 0;
> mutex_unlock(&ctx->mutex);
> }
> - srcu_read_unlock(&pmus_srcu, idx);
> + cpumask_clear_cpu(cpu, perf_online_mask);
> + mutex_unlock(&pmus_lock);
> }
> #else
>
> @@ -10986,6 +11009,29 @@ static void perf_event_exit_cpu_context(
>
> #endif
>
> +int perf_event_init_cpu(unsigned int cpu)
> +{
> + struct perf_cpu_context *cpuctx;
> + struct perf_event_context *ctx;
> + struct pmu *pmu;
> +
> + perf_swevent_init_cpu(cpu);
> +
> + mutex_lock(&pmus_lock);
> + cpumask_set_cpu(cpu, perf_online_mask);
> + list_for_each_entry(pmu, &pmus, entry) {
> + cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
> + ctx = &cpuctx->ctx;
> +
> + mutex_lock(&ctx->mutex);
> + cpuctx->online = 1;
> + mutex_unlock(&ctx->mutex);
> + }
> + mutex_unlock(&pmus_lock);
> +
> + return 0;
> +}
> +
> int perf_event_exit_cpu(unsigned int cpu)
> {
> perf_event_exit_cpu_context(cpu);
>
>
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-05-24 20:50 +0200 |
| Message-ID | <tKJw6-5Wg-13@gated-at.bofh.it> |
| In reply to | #1649858 |
On Wed, 24 May 2017, Paul E. McKenney wrote: > > @@ -8920,7 +8912,7 @@ perf_event_mux_interval_ms_store(struct > > pmu->hrtimer_interval_ms = timer; > > > > /* update all cpuctx for this PMU */ > > - get_online_cpus(); > > + cpus_read_lock(); > > OK, I'll bite... > > Why is this piece using cpus_read_lock() instead of pmus_lock? > > My guess is for the benefit of the cpu_function_call() below, but if > the code instead cycled through the perf_online_mask, wouldn't any > CPU selected be guaranteed to be online? Indeed. > Or is there some reason that it would be necessary to specially handle > CPUs that perf does not consider to be active, but that are still at > least partway online? I have to delegate that question to Peter :) Thanks, tglx
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-05-24 23:20 +0200 |
| Message-ID | <tKLRf-7y3-7@gated-at.bofh.it> |
| In reply to | #1649861 |
On Wed, May 24, 2017 at 08:47:00PM +0200, Thomas Gleixner wrote: > On Wed, 24 May 2017, Paul E. McKenney wrote: > > > @@ -8920,7 +8912,7 @@ perf_event_mux_interval_ms_store(struct > > > pmu->hrtimer_interval_ms = timer; > > > > > > /* update all cpuctx for this PMU */ > > > - get_online_cpus(); > > > + cpus_read_lock(); > > > > OK, I'll bite... > > > > Why is this piece using cpus_read_lock() instead of pmus_lock? > > > > My guess is for the benefit of the cpu_function_call() below, but if > > the code instead cycled through the perf_online_mask, wouldn't any > > CPU selected be guaranteed to be online? > > Indeed. > > > Or is there some reason that it would be necessary to specially handle > > CPUs that perf does not consider to be active, but that are still at > > least partway online? > > I have to delegate that question to Peter :) Another reason might be a desire to avoid contention on pmus_lock, if this function is called often. If that is the case, I cannot resist suggesting percpu_rw_sem. ;-) Thanx, Paul
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2017-05-30 13:30 +0200 |
| Message-ID | <tMNvA-6Gp-3@gated-at.bofh.it> |
| In reply to | #1649858 |
On Wed, May 24, 2017 at 11:30:18AM -0700, Paul E. McKenney wrote:
> > @@ -8920,7 +8912,7 @@ perf_event_mux_interval_ms_store(struct
> > pmu->hrtimer_interval_ms = timer;
> >
> > /* update all cpuctx for this PMU */
> > - get_online_cpus();
> > + cpus_read_lock();
>
> OK, I'll bite...
>
> Why is this piece using cpus_read_lock() instead of pmus_lock?
>
> My guess is for the benefit of the cpu_function_call() below, but if
> the code instead cycled through the perf_online_mask, wouldn't any
> CPU selected be guaranteed to be online?
>
> Or is there some reason that it would be necessary to specially handle
> CPUs that perf does not consider to be active, but that are still at
> least partway online?
Mostly just lazy. This code path didn't present a problem with the lock
ordering. Find the conversion below.
>
> > for_each_online_cpu(cpu) {
> > struct perf_cpu_context *cpuctx;
> > cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
> > @@ -8929,7 +8921,7 @@ perf_event_mux_interval_ms_store(struct
> > cpu_function_call(cpu,
> > (remote_function_f)perf_mux_hrtimer_restart, cpuctx);
> > }
> > - put_online_cpus();
> > + cpus_read_unlock();
> > mutex_unlock(&mux_interval_mutex);
> >
> > return count;
---
Subject: perf: Complete CPU hotplug conversion
Remove the last cpuc_read_lock() user in perf in favour of our internal
state. This conversion is non critical as the lock ordering wasn't
problematic but its nice to be consistent.
Reported-by: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 8d6acaeeea17..ad4f7f03b519 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -389,6 +389,16 @@ static atomic_t nr_switch_events __read_mostly;
static LIST_HEAD(pmus);
static DEFINE_MUTEX(pmus_lock);
static struct srcu_struct pmus_srcu;
+
+/*
+ * CPU hotplug handling, also see perf_event_{exit,init}_cpu().
+ *
+ * We use @pmus_lock to serialize PMU (un)registration against CPU hotplug,
+ * tracking the online state in @perf_online_mask and
+ * pmu->pmu_cpu_context->online. That latter is set while holding ctx->mutex
+ * and therefore holding ctx->mutex is sufficient to serialize against
+ * hotplug wrt cpuctx->online.
+ */
static cpumask_var_t perf_online_mask;
/*
@@ -8887,8 +8897,6 @@ perf_event_mux_interval_ms_show(struct device *dev,
return snprintf(page, PAGE_SIZE-1, "%d\n", pmu->hrtimer_interval_ms);
}
-static DEFINE_MUTEX(mux_interval_mutex);
-
static ssize_t
perf_event_mux_interval_ms_store(struct device *dev,
struct device_attribute *attr,
@@ -8908,12 +8916,12 @@ perf_event_mux_interval_ms_store(struct device *dev,
if (timer == pmu->hrtimer_interval_ms)
return count;
- mutex_lock(&mux_interval_mutex);
+ /* use pmus_lock to order against hotplug and self serialize */
+ mutex_lock(&pmus_lock);
pmu->hrtimer_interval_ms = timer;
/* update all cpuctx for this PMU */
- cpus_read_lock();
- for_each_online_cpu(cpu) {
+ for_each_cpu(cpu, perf_online_mask) {
struct perf_cpu_context *cpuctx;
cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
cpuctx->hrtimer_interval = ns_to_ktime(NSEC_PER_MSEC * timer);
@@ -8921,8 +8929,7 @@ perf_event_mux_interval_ms_store(struct device *dev,
cpu_function_call(cpu,
(remote_function_f)perf_mux_hrtimer_restart, cpuctx);
}
- cpus_read_unlock();
- mutex_unlock(&mux_interval_mutex);
+ mutex_unlock(&pmus_lock);
return count;
}
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-05-30 18:30 +0200 |
| Message-ID | <tMSbU-1dJ-13@gated-at.bofh.it> |
| In reply to | #1653134 |
On Tue, May 30, 2017 at 01:22:35PM +0200, Peter Zijlstra wrote:
> On Wed, May 24, 2017 at 11:30:18AM -0700, Paul E. McKenney wrote:
> > > @@ -8920,7 +8912,7 @@ perf_event_mux_interval_ms_store(struct
> > > pmu->hrtimer_interval_ms = timer;
> > >
> > > /* update all cpuctx for this PMU */
> > > - get_online_cpus();
> > > + cpus_read_lock();
> >
> > OK, I'll bite...
> >
> > Why is this piece using cpus_read_lock() instead of pmus_lock?
> >
> > My guess is for the benefit of the cpu_function_call() below, but if
> > the code instead cycled through the perf_online_mask, wouldn't any
> > CPU selected be guaranteed to be online?
> >
> > Or is there some reason that it would be necessary to specially handle
> > CPUs that perf does not consider to be active, but that are still at
> > least partway online?
>
> Mostly just lazy. This code path didn't present a problem with the lock
> ordering. Find the conversion below.
I know that feeling! With this addition:
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > for_each_online_cpu(cpu) {
> > > struct perf_cpu_context *cpuctx;
> > > cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
> > > @@ -8929,7 +8921,7 @@ perf_event_mux_interval_ms_store(struct
> > > cpu_function_call(cpu,
> > > (remote_function_f)perf_mux_hrtimer_restart, cpuctx);
> > > }
> > > - put_online_cpus();
> > > + cpus_read_unlock();
> > > mutex_unlock(&mux_interval_mutex);
> > >
> > > return count;
>
>
> ---
> Subject: perf: Complete CPU hotplug conversion
>
> Remove the last cpuc_read_lock() user in perf in favour of our internal
> state. This conversion is non critical as the lock ordering wasn't
> problematic but its nice to be consistent.
>
> Reported-by: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> kernel/events/core.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 8d6acaeeea17..ad4f7f03b519 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -389,6 +389,16 @@ static atomic_t nr_switch_events __read_mostly;
> static LIST_HEAD(pmus);
> static DEFINE_MUTEX(pmus_lock);
> static struct srcu_struct pmus_srcu;
> +
> +/*
> + * CPU hotplug handling, also see perf_event_{exit,init}_cpu().
> + *
> + * We use @pmus_lock to serialize PMU (un)registration against CPU hotplug,
> + * tracking the online state in @perf_online_mask and
> + * pmu->pmu_cpu_context->online. That latter is set while holding ctx->mutex
> + * and therefore holding ctx->mutex is sufficient to serialize against
> + * hotplug wrt cpuctx->online.
> + */
> static cpumask_var_t perf_online_mask;
>
> /*
> @@ -8887,8 +8897,6 @@ perf_event_mux_interval_ms_show(struct device *dev,
> return snprintf(page, PAGE_SIZE-1, "%d\n", pmu->hrtimer_interval_ms);
> }
>
> -static DEFINE_MUTEX(mux_interval_mutex);
> -
> static ssize_t
> perf_event_mux_interval_ms_store(struct device *dev,
> struct device_attribute *attr,
> @@ -8908,12 +8916,12 @@ perf_event_mux_interval_ms_store(struct device *dev,
> if (timer == pmu->hrtimer_interval_ms)
> return count;
>
> - mutex_lock(&mux_interval_mutex);
> + /* use pmus_lock to order against hotplug and self serialize */
> + mutex_lock(&pmus_lock);
> pmu->hrtimer_interval_ms = timer;
>
> /* update all cpuctx for this PMU */
> - cpus_read_lock();
> - for_each_online_cpu(cpu) {
> + for_each_cpu(cpu, perf_online_mask) {
> struct perf_cpu_context *cpuctx;
> cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
> cpuctx->hrtimer_interval = ns_to_ktime(NSEC_PER_MSEC * timer);
> @@ -8921,8 +8929,7 @@ perf_event_mux_interval_ms_store(struct device *dev,
> cpu_function_call(cpu,
> (remote_function_f)perf_mux_hrtimer_restart, cpuctx);
> }
> - cpus_read_unlock();
> - mutex_unlock(&mux_interval_mutex);
> + mutex_unlock(&pmus_lock);
>
> return count;
> }
>
[toc] | [prev] | [next] | [standalone]
| From | tip-bot for Thomas Gleixner <tipbot@zytor.com> |
|---|---|
| Date | 2017-05-26 10:50 +0200 |
| Subject | [tip:smp/hotplug] perf/tracing/cpuhotplug: Fix locking order |
| Message-ID | <tLj6y-3Cw-9@gated-at.bofh.it> |
| In reply to | #1649230 |
Commit-ID: a63fbed776c7124ce9f606234267c3c095b2680e
Gitweb: http://git.kernel.org/tip/a63fbed776c7124ce9f606234267c3c095b2680e
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 24 May 2017 10:15:34 +0200
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 26 May 2017 10:10:44 +0200
perf/tracing/cpuhotplug: Fix locking order
perf, tracing, kprobes and jump_labels have a gazillion of ways to create
dependency lock chains. Some of those involve nested invocations of
get_online_cpus().
The conversion of the hotplug locking to a percpu rwsem requires to avoid
such nested calls. sys_perf_event_open() protects most of the syscall logic
against cpu hotplug. This causes nested calls and lock inversions versus
ftrace and kprobes in various interesting ways.
It's impossible to move the hotplug locking to the outer end of all call
chains in the involved facilities, so the hotplug protection in
sys_perf_event_open() needs to be solved differently.
Introduce 'pmus_mutex' which protects a perf private online cpumask. This
mutex is taken when the mask is updated in the cpu hotplug callbacks and
can be taken in sys_perf_event_open() to protect the swhash setup/teardown
code and when the final judgement about a valid event has to be made.
[ tglx: Produced changelog and fixed the swhash interaction ]
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Ingo Molnar <mingo@kernel.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Link: http://lkml.kernel.org/r/20170524081548.930941109@linutronix.de
---
include/linux/perf_event.h | 2 +
kernel/events/core.c | 106 ++++++++++++++++++++++++++++++++-------------
2 files changed, 78 insertions(+), 30 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 24a6358..7d6aa29 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -801,6 +801,8 @@ struct perf_cpu_context {
struct list_head sched_cb_entry;
int sched_cb_usage;
+
+ int online;
};
struct perf_output_handle {
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 6e75a5c..b97cda4 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -389,6 +389,7 @@ static atomic_t nr_switch_events __read_mostly;
static LIST_HEAD(pmus);
static DEFINE_MUTEX(pmus_lock);
static struct srcu_struct pmus_srcu;
+static cpumask_var_t perf_online_mask;
/*
* perf event paranoia level:
@@ -3812,14 +3813,6 @@ find_get_context(struct pmu *pmu, struct task_struct *task,
if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
return ERR_PTR(-EACCES);
- /*
- * We could be clever and allow to attach a event to an
- * offline CPU and activate it when the CPU comes up, but
- * that's for later.
- */
- if (!cpu_online(cpu))
- return ERR_PTR(-ENODEV);
-
cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
ctx = &cpuctx->ctx;
get_ctx(ctx);
@@ -7703,7 +7696,8 @@ static int swevent_hlist_get_cpu(int cpu)
int err = 0;
mutex_lock(&swhash->hlist_mutex);
- if (!swevent_hlist_deref(swhash) && cpu_online(cpu)) {
+ if (!swevent_hlist_deref(swhash) &&
+ cpumask_test_cpu(cpu, perf_online_mask)) {
struct swevent_hlist *hlist;
hlist = kzalloc(sizeof(*hlist), GFP_KERNEL);
@@ -7724,7 +7718,7 @@ static int swevent_hlist_get(void)
{
int err, cpu, failed_cpu;
- get_online_cpus();
+ mutex_lock(&pmus_lock);
for_each_possible_cpu(cpu) {
err = swevent_hlist_get_cpu(cpu);
if (err) {
@@ -7732,8 +7726,7 @@ static int swevent_hlist_get(void)
goto fail;
}
}
- put_online_cpus();
-
+ mutex_unlock(&pmus_lock);
return 0;
fail:
for_each_possible_cpu(cpu) {
@@ -7741,8 +7734,7 @@ fail:
break;
swevent_hlist_put_cpu(cpu);
}
-
- put_online_cpus();
+ mutex_unlock(&pmus_lock);
return err;
}
@@ -8920,7 +8912,7 @@ perf_event_mux_interval_ms_store(struct device *dev,
pmu->hrtimer_interval_ms = timer;
/* update all cpuctx for this PMU */
- get_online_cpus();
+ cpus_read_lock();
for_each_online_cpu(cpu) {
struct perf_cpu_context *cpuctx;
cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
@@ -8929,7 +8921,7 @@ perf_event_mux_interval_ms_store(struct device *dev,
cpu_function_call(cpu,
(remote_function_f)perf_mux_hrtimer_restart, cpuctx);
}
- put_online_cpus();
+ cpus_read_unlock();
mutex_unlock(&mux_interval_mutex);
return count;
@@ -9059,6 +9051,7 @@ skip_type:
lockdep_set_class(&cpuctx->ctx.mutex, &cpuctx_mutex);
lockdep_set_class(&cpuctx->ctx.lock, &cpuctx_lock);
cpuctx->ctx.pmu = pmu;
+ cpuctx->online = cpumask_test_cpu(cpu, perf_online_mask);
__perf_mux_hrtimer_init(cpuctx, cpu);
}
@@ -9882,12 +9875,10 @@ SYSCALL_DEFINE5(perf_event_open,
goto err_task;
}
- get_online_cpus();
-
if (task) {
err = mutex_lock_interruptible(&task->signal->cred_guard_mutex);
if (err)
- goto err_cpus;
+ goto err_cred;
/*
* Reuse ptrace permission checks for now.
@@ -10073,6 +10064,23 @@ SYSCALL_DEFINE5(perf_event_open,
goto err_locked;
}
+ if (!task) {
+ /*
+ * Check if the @cpu we're creating an event for is online.
+ *
+ * We use the perf_cpu_context::ctx::mutex to serialize against
+ * the hotplug notifiers. See perf_event_{init,exit}_cpu().
+ */
+ struct perf_cpu_context *cpuctx =
+ container_of(ctx, struct perf_cpu_context, ctx);
+
+ if (!cpuctx->online) {
+ err = -ENODEV;
+ goto err_locked;
+ }
+ }
+
+
/*
* Must be under the same ctx::mutex as perf_install_in_context(),
* because we need to serialize with concurrent event creation.
@@ -10162,8 +10170,6 @@ SYSCALL_DEFINE5(perf_event_open,
put_task_struct(task);
}
- put_online_cpus();
-
mutex_lock(¤t->perf_event_mutex);
list_add_tail(&event->owner_entry, ¤t->perf_event_list);
mutex_unlock(¤t->perf_event_mutex);
@@ -10197,8 +10203,6 @@ err_alloc:
err_cred:
if (task)
mutex_unlock(&task->signal->cred_guard_mutex);
-err_cpus:
- put_online_cpus();
err_task:
if (task)
put_task_struct(task);
@@ -10253,6 +10257,21 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
goto err_unlock;
}
+ if (!task) {
+ /*
+ * Check if the @cpu we're creating an event for is online.
+ *
+ * We use the perf_cpu_context::ctx::mutex to serialize against
+ * the hotplug notifiers. See perf_event_{init,exit}_cpu().
+ */
+ struct perf_cpu_context *cpuctx =
+ container_of(ctx, struct perf_cpu_context, ctx);
+ if (!cpuctx->online) {
+ err = -ENODEV;
+ goto err_unlock;
+ }
+ }
+
if (!exclusive_event_installable(event, ctx)) {
err = -EBUSY;
goto err_unlock;
@@ -10920,6 +10939,8 @@ static void __init perf_event_init_all_cpus(void)
struct swevent_htable *swhash;
int cpu;
+ zalloc_cpumask_var(&perf_online_mask, GFP_KERNEL);
+
for_each_possible_cpu(cpu) {
swhash = &per_cpu(swevent_htable, cpu);
mutex_init(&swhash->hlist_mutex);
@@ -10935,7 +10956,7 @@ static void __init perf_event_init_all_cpus(void)
}
}
-int perf_event_init_cpu(unsigned int cpu)
+void perf_swevent_init_cpu(unsigned int cpu)
{
struct swevent_htable *swhash = &per_cpu(swevent_htable, cpu);
@@ -10948,7 +10969,6 @@ int perf_event_init_cpu(unsigned int cpu)
rcu_assign_pointer(swhash->swevent_hlist, hlist);
}
mutex_unlock(&swhash->hlist_mutex);
- return 0;
}
#if defined CONFIG_HOTPLUG_CPU || defined CONFIG_KEXEC_CORE
@@ -10966,19 +10986,22 @@ static void __perf_event_exit_context(void *__info)
static void perf_event_exit_cpu_context(int cpu)
{
+ struct perf_cpu_context *cpuctx;
struct perf_event_context *ctx;
struct pmu *pmu;
- int idx;
- idx = srcu_read_lock(&pmus_srcu);
- list_for_each_entry_rcu(pmu, &pmus, entry) {
- ctx = &per_cpu_ptr(pmu->pmu_cpu_context, cpu)->ctx;
+ mutex_lock(&pmus_lock);
+ list_for_each_entry(pmu, &pmus, entry) {
+ cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
+ ctx = &cpuctx->ctx;
mutex_lock(&ctx->mutex);
smp_call_function_single(cpu, __perf_event_exit_context, ctx, 1);
+ cpuctx->online = 0;
mutex_unlock(&ctx->mutex);
}
- srcu_read_unlock(&pmus_srcu, idx);
+ cpumask_clear_cpu(cpu, perf_online_mask);
+ mutex_unlock(&pmus_lock);
}
#else
@@ -10986,6 +11009,29 @@ static void perf_event_exit_cpu_context(int cpu) { }
#endif
+int perf_event_init_cpu(unsigned int cpu)
+{
+ struct perf_cpu_context *cpuctx;
+ struct perf_event_context *ctx;
+ struct pmu *pmu;
+
+ perf_swevent_init_cpu(cpu);
+
+ mutex_lock(&pmus_lock);
+ cpumask_set_cpu(cpu, perf_online_mask);
+ list_for_each_entry(pmu, &pmus, entry) {
+ cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
+ ctx = &cpuctx->ctx;
+
+ mutex_lock(&ctx->mutex);
+ cpuctx->online = 1;
+ mutex_unlock(&ctx->mutex);
+ }
+ mutex_unlock(&pmus_lock);
+
+ return 0;
+}
+
int perf_event_exit_cpu(unsigned int cpu)
{
perf_event_exit_cpu_context(cpu);
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web