Messages in this thread | | | Date | Thu, 4 Jul 2013 14:41:48 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH v2 3/7] perf, x86: Introduce x86 special perf event context |
| |
On Mon, Jul 01, 2013 at 03:23:03PM +0800, Yan, Zheng wrote: > From: "Yan, Zheng" <zheng.z.yan@intel.com> > > The x86 special perf event context is named x86_perf_event_context, > We can enlarge it later to store PMU special data.
This changelog is completely inadequate. It fails to state what and why we do things.
I hate doing this; but I can't see another way around it either. That said:
> @@ -274,6 +274,11 @@ struct pmu { > * flush branch stack on context-switches (needed in cpu-wide mode) > */ > void (*flush_branch_stack) (void); > + > + /* > + * Allocate PMU special perf event context > + */ > + void *(*event_context_alloc) (struct perf_event_context *parent_ctx); > };
It should be *optional*, also wtf is that parent_ctx thing for?
> +++ b/kernel/events/core.c > @@ -2961,13 +2961,20 @@ static void __perf_event_init_context(struct perf_event_context *ctx) > } > > static struct perf_event_context * > -alloc_perf_context(struct pmu *pmu, struct task_struct *task) > +alloc_perf_context(struct pmu *pmu, struct task_struct *task, > + struct perf_event_context *parent_ctx) > { > struct perf_event_context *ctx; > > - ctx = kzalloc(sizeof(struct perf_event_context), GFP_KERNEL); > - if (!ctx) > - return NULL; > + if (pmu->event_context_alloc) { > + ctx = pmu->event_context_alloc(parent_ctx); > + if (IS_ERR(ctx)) > + return ctx; > + } else { > + ctx = kzalloc(sizeof(struct perf_event_context), GFP_KERNEL); > + if (!ctx) > + return ERR_PTR(-ENOMEM); > + } > > __perf_event_init_context(ctx); > if (task) {
I'm not at all sure we want to do it like this; why not simply query the size. Something like:
alloc_perf_context(struct pmu *pmu, struct task_struct *task) { size_t ctx_size = sizeof(struct perf_event_context);
if (pmu->task_context_size) size = pmu->task_context_size();
ctx = kzalloc(size, GFP_KERNEL); if (!ctx) return ERR_PTR(-ENOMEM);
...
}
| |