Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1287193 > unrolled thread
| Started by | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| First post | 2015-12-09 09:40 +0100 |
| Last post | 2015-12-09 16:30 +0100 |
| Articles | 4 — 2 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.
Re: [tip:perf/core] perf: Add pmu specific data for perf task context Peter Zijlstra <peterz@infradead.org> - 2015-12-09 09:40 +0100
RE: [tip:perf/core] perf: Add pmu specific data for perf task context "Liang, Kan" <kan.liang@intel.com> - 2015-12-09 16:00 +0100
Re: [tip:perf/core] perf: Add pmu specific data for perf task context Peter Zijlstra <peterz@infradead.org> - 2015-12-09 16:20 +0100
RE: [tip:perf/core] perf: Add pmu specific data for perf task context "Liang, Kan" <kan.liang@intel.com> - 2015-12-09 16:30 +0100
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-12-09 09:40 +0100 |
| Subject | Re: [tip:perf/core] perf: Add pmu specific data for perf task context |
| Message-ID | <qDIs1-5V9-9@gated-at.bofh.it> |
On Wed, Feb 18, 2015 at 09:15:06AM -0800, tip-bot for Yan, Zheng wrote:
> +find_get_context(struct pmu *pmu, struct task_struct *task,
> + struct perf_event *event)
> {
> struct perf_event_context *ctx, *clone_ctx = NULL;
> struct perf_cpu_context *cpuctx;
> + void *task_ctx_data = NULL;
> unsigned long flags;
> int ctxn, err;
> + int cpu = event->cpu;
>
> if (!task) {
> /* Must be root to operate on a CPU event: */
> @@ -3342,11 +3354,24 @@ find_get_context(struct pmu *pmu, struct task_struct *task, int cpu)
> if (ctxn < 0)
> goto errout;
>
> + if (event->attach_state & PERF_ATTACH_TASK_DATA) {
> + task_ctx_data = kzalloc(pmu->task_ctx_size, GFP_KERNEL);
> + if (!task_ctx_data) {
> + err = -ENOMEM;
> + goto errout;
> + }
> + }
> +
> retry:
> ctx = perf_lock_task_context(task, ctxn, &flags);
> if (ctx) {
> clone_ctx = unclone_ctx(ctx);
> ++ctx->pin_count;
> +
> + if (task_ctx_data && !ctx->task_ctx_data) {
> + ctx->task_ctx_data = task_ctx_data;
> + task_ctx_data = NULL;
> + }
> raw_spin_unlock_irqrestore(&ctx->lock, flags);
>
> if (clone_ctx)
> @@ -3357,6 +3382,11 @@ retry:
> if (!ctx)
> goto errout;
>
> + if (task_ctx_data) {
> + ctx->task_ctx_data = task_ctx_data;
> + task_ctx_data = NULL;
> + }
> +
> err = 0;
> mutex_lock(&task->perf_event_mutex);
> /*
> @@ -3383,9 +3413,11 @@ retry:
> }
> }
>
> + kfree(task_ctx_data);
> return ctx;
>
> errout:
> + kfree(task_ctx_data);
> return ERR_PTR(err);
> }
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 36babfd..97aa610 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3508,11 +3515,6 @@ retry:
if (!ctx)
goto errout;
- if (task_ctx_data) {
- ctx->task_ctx_data = task_ctx_data;
- task_ctx_data = NULL;
- }
-
err = 0;
mutex_lock(&task->perf_event_mutex);
/*
@@ -3526,6 +3528,10 @@ retry:
else {
get_ctx(ctx);
++ctx->pin_count;
+ if (task_ctx_data) {
+ ctx->task_ctx_data = task_ctx_data;
+ task_ctx_data = NULL;
+ }
rcu_assign_pointer(task->perf_event_ctxp[ctxn], ctx);
}
mutex_unlock(&task->perf_event_mutex);
Does that make sense? No point in setting task_ctx_data if we're going
to free the ctx and try again.
--
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/
[toc] | [next] | [standalone]
| From | "Liang, Kan" <kan.liang@intel.com> |
|---|---|
| Date | 2015-12-09 16:00 +0100 |
| Subject | RE: [tip:perf/core] perf: Add pmu specific data for perf task context |
| Message-ID | <qDOnL-1h9-5@gated-at.bofh.it> |
| In reply to | #1287193 |
>
> On Wed, Feb 18, 2015 at 09:15:06AM -0800, tip-bot for Yan, Zheng wrote:
> > +find_get_context(struct pmu *pmu, struct task_struct *task,
> > + struct perf_event *event)
> > {
> > struct perf_event_context *ctx, *clone_ctx = NULL;
> > struct perf_cpu_context *cpuctx;
> > + void *task_ctx_data = NULL;
> > unsigned long flags;
> > int ctxn, err;
> > + int cpu = event->cpu;
> >
> > if (!task) {
> > /* Must be root to operate on a CPU event: */ @@ -
> 3342,11 +3354,24
> > @@ find_get_context(struct pmu *pmu, struct task_struct *task, int cpu)
> > if (ctxn < 0)
> > goto errout;
> >
> > + if (event->attach_state & PERF_ATTACH_TASK_DATA) {
> > + task_ctx_data = kzalloc(pmu->task_ctx_size, GFP_KERNEL);
> > + if (!task_ctx_data) {
> > + err = -ENOMEM;
> > + goto errout;
> > + }
> > + }
> > +
> > retry:
> > ctx = perf_lock_task_context(task, ctxn, &flags);
> > if (ctx) {
> > clone_ctx = unclone_ctx(ctx);
> > ++ctx->pin_count;
> > +
> > + if (task_ctx_data && !ctx->task_ctx_data) {
> > + ctx->task_ctx_data = task_ctx_data;
> > + task_ctx_data = NULL;
> > + }
> > raw_spin_unlock_irqrestore(&ctx->lock, flags);
> >
> > if (clone_ctx)
> > @@ -3357,6 +3382,11 @@ retry:
> > if (!ctx)
> > goto errout;
> >
> > + if (task_ctx_data) {
> > + ctx->task_ctx_data = task_ctx_data;
> > + task_ctx_data = NULL;
> > + }
> > +
> > err = 0;
> > mutex_lock(&task->perf_event_mutex);
> > /*
> > @@ -3383,9 +3413,11 @@ retry:
> > }
> > }
> >
> > + kfree(task_ctx_data);
> > return ctx;
> >
> > errout:
> > + kfree(task_ctx_data);
> > return ERR_PTR(err);
> > }
>
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c index
> 36babfd..97aa610 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3508,11 +3515,6 @@ retry:
> if (!ctx)
> goto errout;
>
> - if (task_ctx_data) {
> - ctx->task_ctx_data = task_ctx_data;
> - task_ctx_data = NULL;
> - }
> -
> err = 0;
> mutex_lock(&task->perf_event_mutex);
> /*
> @@ -3526,6 +3528,10 @@ retry:
> else {
> get_ctx(ctx);
> ++ctx->pin_count;
> + if (task_ctx_data) {
> + ctx->task_ctx_data = task_ctx_data;
> + task_ctx_data = NULL;
> + }
> rcu_assign_pointer(task->perf_event_ctxp[ctxn],
> ctx);
> }
> mutex_unlock(&task->perf_event_mutex);
>
>
> Does that make sense? No point in setting task_ctx_data if we're going to
> free the ctx and try again.
The task_ctx_data will be checked before use. So it wouldn't crash the
system if it's NULL.
The problem is that LBR stack info will not be save/store on context switch
anymore. The user probably get wrong call stack information.
May I know why you want to do that?
Thanks,
Kan
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-12-09 16:20 +0100 |
| Message-ID | <qDOH8-1CS-13@gated-at.bofh.it> |
| In reply to | #1287537 |
On Wed, Dec 09, 2015 at 02:59:21PM +0000, Liang, Kan wrote:
> > diff --git a/kernel/events/core.c b/kernel/events/core.c index
> > 36babfd..97aa610 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -3508,11 +3515,6 @@ retry:
> > if (!ctx)
> > goto errout;
> >
> > - if (task_ctx_data) {
> > - ctx->task_ctx_data = task_ctx_data;
> > - task_ctx_data = NULL;
> > - }
> > -
> > err = 0;
> > mutex_lock(&task->perf_event_mutex);
> > /*
> > @@ -3526,6 +3528,10 @@ retry:
> > else {
> > get_ctx(ctx);
> > ++ctx->pin_count;
> > + if (task_ctx_data) {
> > + ctx->task_ctx_data = task_ctx_data;
> > + task_ctx_data = NULL;
> > + }
> > rcu_assign_pointer(task->perf_event_ctxp[ctxn],
> > ctx);
> > }
> > mutex_unlock(&task->perf_event_mutex);
> >
> >
> > Does that make sense? No point in setting task_ctx_data if we're going to
> > free the ctx and try again.
>
> The task_ctx_data will be checked before use. So it wouldn't crash the
> system if it's NULL.
Yeah, I know, I checked :-)
> The problem is that LBR stack info will not be save/store on context
> switch anymore. The user probably get wrong call stack information.
Yep
> May I know why you want to do that?
Because this seemed like a less fragile construct. When there's multiple
event creations racing it seems possible (ableit entirely unlikely) to
assign the allocated task_ctx_data to a ctx that we'll delete, and on
the second go around re-allocate a ctx, but are left wihtout
task_ctx_data to assign to it.
So by only assigning the task_ctx_data when we _know_ we've succeeded,
we'll avoid this scenario.
--
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/
[toc] | [prev] | [next] | [standalone]
| From | "Liang, Kan" <kan.liang@intel.com> |
|---|---|
| Date | 2015-12-09 16:30 +0100 |
| Subject | RE: [tip:perf/core] perf: Add pmu specific data for perf task context |
| Message-ID | <qDOQO-1FV-27@gated-at.bofh.it> |
| In reply to | #1287554 |
>
> On Wed, Dec 09, 2015 at 02:59:21PM +0000, Liang, Kan wrote:
> > > diff --git a/kernel/events/core.c b/kernel/events/core.c index
> > > 36babfd..97aa610 100644
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -3508,11 +3515,6 @@ retry:
> > > if (!ctx)
> > > goto errout;
> > >
> > > - if (task_ctx_data) {
> > > - ctx->task_ctx_data = task_ctx_data;
> > > - task_ctx_data = NULL;
> > > - }
> > > -
> > > err = 0;
> > > mutex_lock(&task->perf_event_mutex);
> > > /*
> > > @@ -3526,6 +3528,10 @@ retry:
> > > else {
> > > get_ctx(ctx);
> > > ++ctx->pin_count;
> > > + if (task_ctx_data) {
> > > + ctx->task_ctx_data = task_ctx_data;
> > > + task_ctx_data = NULL;
> > > + }
> > > rcu_assign_pointer(task->perf_event_ctxp[ctxn],
> > > ctx);
> > > }
> > > mutex_unlock(&task->perf_event_mutex);
> > >
> > >
> > > Does that make sense? No point in setting task_ctx_data if we're
> > > going to free the ctx and try again.
> >
> > The task_ctx_data will be checked before use. So it wouldn't crash the
> > system if it's NULL.
>
> Yeah, I know, I checked :-)
>
> > The problem is that LBR stack info will not be save/store on context
> > switch anymore. The user probably get wrong call stack information.
>
> Yep
>
> > May I know why you want to do that?
>
> Because this seemed like a less fragile construct. When there's multiple
> event creations racing it seems possible (ableit entirely unlikely) to assign
> the allocated task_ctx_data to a ctx that we'll delete, and on the second go
> around re-allocate a ctx, but are left wihtout task_ctx_data to assign to it.
>
> So by only assigning the task_ctx_data when we _know_ we've succeeded,
> we'll avoid this scenario.
Yes, I think it make sense to that.
Thanks,
Kan
--
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/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web