From: Daniel Axtens <dja@axtens•net>
To: Madhavan Srinivasan <maddy@linux•vnet.ibm.com>
Cc: linux-kernel@vger•kernel.org, linuxppc-dev@lists•ozlabs.org,
Stephane Eranian <eranian@google•com>,
Paul Mackerras <paulus@samba•org>,
Sukadev Bhattiprolu <sukadev@linux•vnet.ibm.com>,
Anshuman Khandual <khandual@linux•vnet.ibm.com>
Subject: Re: [PATCH v1 7/9]powerpc/powernv: Event attr creation and PMU registration
Date: Wed, 03 Jun 2015 11:06:32 +1000 [thread overview]
Message-ID: <1433293592.438.74.camel@axtens.net> (raw)
In-Reply-To: <1433260778-26497-8-git-send-email-maddy@linux.vnet.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3485 bytes --]
On Tue, 2015-06-02 at 21:29 +0530, Madhavan Srinivasan wrote:
> Patch adds common event attribute function and Nest pmu registration call.
>
> Cc: Michael Ellerman <mpe@ellerman•id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel•crashing.org>
> Cc: Paul Mackerras <paulus@samba•org>
> Cc: Sukadev Bhattiprolu <sukadev@linux•vnet.ibm.com>
> Cc: Anshuman Khandual <khandual@linux•vnet.ibm.com>
> Cc: Stephane Eranian <eranian@google•com>
> Signed-off-by: Madhavan Srinivasan <maddy@linux•vnet.ibm.com>
> ---
> arch/powerpc/perf/nest-pmu.c | 52 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c
> index 514a0be..dd84fd7 100644
> --- a/arch/powerpc/perf/nest-pmu.c
> +++ b/arch/powerpc/perf/nest-pmu.c
> @@ -244,6 +244,49 @@ static int update_pmu_ops(struct nest_pmu *pmu)
> return 0;
> }
>
> +/*
> + * Populate event name and string in attribute
> + */
> +struct attribute *dev_str_attr(char *name, char *str)
> +{
> + struct perf_pmu_events_attr *attr;
> +
> + attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> +
> + attr->event_str = (const char *)str;
Erk. Two things:
- Is str const or not? If you're treating it as const here, should you
pass that through the function signature?
- Who is responsible for the memory behind it? It looks like a caller
can't construct str dynamically, pass it to this function and then free
it, because that will invalidate attr->event_str. Is this documented?
> + attr->attr.attr.name = name;
> + attr->attr.attr.mode = 0444;
> + attr->attr.show = perf_event_sysfs_show;
> +
> + return &attr->attr.attr;
If you're returning the address of attr->attr.attr, then:
- why don't you just deal directly with struct attribute * in the
function? Why an entire struct perf_pmu_events_attr *?
- with the function as written, if you return just &attr->attr.attr,
don't attr->event_str and attr->attr.show get lost?
> +}
> +
> +int update_events_in_group(
> + struct ppc64_nest_ima_events *p8_events, int idx,
> + struct nest_pmu *pmu)
> +{
> + struct attribute_group *attr_group;
> + struct attribute **attrs;
> + int i;
> +
> + attr_group = kzalloc(((sizeof(struct attribute *) * (idx + 1)) +
> + sizeof(*attr_group)), GFP_KERNEL);
> + if (!attr_group)
> + return -ENOMEM;
> +
> + attrs = (struct attribute **)(attr_group + 1);
> + attr_group->name = "events";
> + attr_group->attrs = attrs;
> +
> + for (i=0; i< idx; i++, p8_events++)
> + attrs[i] = dev_str_attr((char *)p8_events->ev_name,
> + (char *)p8_events->ev_value);
> +
> + pmu->attr_groups[0] = attr_group;
> + return 0;
> +}
I'm very confused by what this function is trying to do. Could you add
some comments? I'm particularly confused by the relationship between
attrs and attr_group.
> +
> +
> static int nest_pmu_create(struct device_node *dev, int pmu_index)
> {
> struct ppc64_nest_ima_events **p8_events_arr;
> @@ -364,6 +407,15 @@ static int nest_pmu_create(struct device_node *dev, int pmu_index)
> }
> }
>
> + update_events_in_group(
> + (struct ppc64_nest_ima_events *)p8_events_arr,
> + idx, pmu_ptr);
> + update_pmu_ops(pmu_ptr);
> +
> + /* Register the pmu */
> + perf_pmu_register(&pmu_ptr->pmu, pmu_ptr->pmu.name, -1);
> + printk(KERN_INFO "Nest PMU %s Registered\n", pmu_ptr->pmu.name);
> +
> return 0;
> }
>
Regards,
Daniel
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]
next prev parent reply other threads:[~2015-06-03 1:07 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-02 15:59 [PATCH v1 0/9]powerpc/powernv: Nest Instrumentation support Madhavan Srinivasan
2015-06-02 15:59 ` [PATCH v1 1/9]powerpc/powernv: Data structure and macros definition Madhavan Srinivasan
2015-06-02 23:11 ` Daniel Axtens
2015-06-04 7:48 ` Madhavan Srinivasan
2015-06-02 15:59 ` [PATCH v1 2/9]powerpc/powernv: nest pmu init function with cpumask attr Madhavan Srinivasan
2015-06-02 23:14 ` Daniel Axtens
2015-06-04 8:06 ` Madhavan Srinivasan
2015-06-02 15:59 ` [PATCH v1 3/9]powerpc/powernv: Add cpu hotplug support Madhavan Srinivasan
2015-06-02 23:38 ` Daniel Axtens
2015-06-04 8:30 ` Madhavan Srinivasan
2015-06-02 15:59 ` [PATCH v1 4/9]powerpc/powernv: Add generic nest pmu ops Madhavan Srinivasan
2015-06-03 0:03 ` Daniel Axtens
2015-06-04 9:06 ` Madhavan Srinivasan
2015-06-04 9:27 ` Madhavan Srinivasan
2015-06-02 15:59 ` [PATCH v1 5/9]powerpc/powernv: nest pmu feature detection support Madhavan Srinivasan
2015-06-03 0:21 ` Daniel Axtens
2015-06-04 9:52 ` Madhavan Srinivasan
2015-06-02 15:59 ` [PATCH v1 6/9]powerpc/powernv: dt parser function for nest pmu and its events Madhavan Srinivasan
2015-06-03 0:46 ` Daniel Axtens
2015-06-04 10:03 ` Madhavan Srinivasan
2015-06-02 15:59 ` [PATCH v1 7/9]powerpc/powernv: Event attr creation and PMU registration Madhavan Srinivasan
2015-06-03 1:06 ` Daniel Axtens [this message]
2015-06-09 11:41 ` Madhavan Srinivasan
2015-06-02 15:59 ` [PATCH v1 8/9] powerpc/powernv: Add OPAL support for Nest PMU Madhavan Srinivasan
2015-06-03 0:54 ` Daniel Axtens
2015-06-04 10:26 ` Madhavan Srinivasan
2015-06-02 15:59 ` [PATCH v1 9/9]powerpc/powernv: Makefile changes to include nest pmu Madhavan Srinivasan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1433293592.438.74.camel@axtens.net \
--to=dja@axtens$(echo .)net \
--cc=eranian@google$(echo .)com \
--cc=khandual@linux$(echo .)vnet.ibm.com \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=maddy@linux$(echo .)vnet.ibm.com \
--cc=paulus@samba$(echo .)org \
--cc=sukadev@linux$(echo .)vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox