Re: [PATCH v5 5/7] powerpc/powernv: add event attribute and group to nest pmu

[Daniel Axtens <dja@axtens.net>]

[-- Attachment #1: Type: text/plain, Size: 3577 bytes --]

On Thu, 2015-07-16 at 16:43 +0530, Madhavan Srinivasan wrote:
> Add code to create event/format attributes and attribute groups for
> each nest pmu.
> 
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Anton Blanchard <anton@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 | 65 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c
> index c4c08e4dee55..f3418bdec1cd 100644
> --- a/arch/powerpc/perf/nest-pmu.c
> +++ b/arch/powerpc/perf/nest-pmu.c
> @@ -13,6 +13,17 @@
>  static struct perchip_nest_info p8_nest_perchip_info[P8_NEST_MAX_CHIPS];
>  static struct nest_pmu *per_nest_pmu_arr[P8_NEST_MAX_PMUS];
>  
> +PMU_FORMAT_ATTR(event, "config:0-20");
> +static struct attribute *p8_nest_format_attrs[] = {
> +	&format_attr_event.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group p8_nest_format_group = {
> +	.name = "format",
> +	.attrs = p8_nest_format_attrs,
> +};
> +
>  static int nest_event_info(struct property *pp, char *name,
>  			struct nest_ima_events *p8_events, int string, u32 val)
>  {
> @@ -46,6 +57,56 @@ static int nest_event_info(struct property *pp, char *name,
>  	return 0;
>  }
>  
> +/*
> + * Populate event name and string in attribute
> + */
> +static struct attribute *dev_str_attr(const char *name, const char *str)
> +{
> +	struct perf_pmu_events_attr *attr;
> +
> +	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> +
> +	sysfs_attr_init(&attr->attr.attr);
> +
> +	attr->event_str = str;
> +	attr->attr.attr.name = name;
> +	attr->attr.attr.mode = 0444;
> +	attr->attr.show = perf_event_sysfs_show;
> +
> +	return &attr->attr.attr;
So I asked you about this before, and you pointed me to
perf_event_sysfs_show. Looking at that in kernel/events/core.c, it looks
like that uses container_of to pull out the perf_pmu_events_attr. So I
guess that is at least mostly correct.

I'm hoping something else uses container_of to pull out attr->attr, so
that they can actually grab the attr->attr.show function pointer, so
that perf_event_sysfs_show actually gets called. Where would that be?

> +}
> +
> +static int update_events_in_group(
> +	struct nest_ima_events *p8_events, int idx, struct nest_pmu *pmu)
> +{
> +	struct attribute_group *attr_group;
> +	struct attribute **attrs;
> +	int i;
> +
> +	/*
> +	 * Allocate memory for both event attribute group and for
> +	 * event attributes array.
> +	 */
> +	attr_group = kzalloc(((sizeof(struct attribute *) * (idx + 1)) +
> +				sizeof(*attr_group)), GFP_KERNEL);
> +	if (!attr_group)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Assign memory for event attribute array
> +	 */
> +	attrs = (struct attribute **)(attr_group + 1);
> +	attr_group->name = "events";
> +	attr_group->attrs = attrs;
I am super uncomfortable with this block, especially the assignment to
attrs. I *think* you're trying to allocate an attribute group and a set
of attributes, but you've combined the allocation into one big
contiguous chunk, and then you're trying to tease them apart. Is that
necessary? Could it be two allocs, one for the attribute_group and one
for the attribute?

-- 
Regards,
Daniel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]