Re: [PATCH v3 1/2] perf x86: Infrastructure for exposing an Uncore unit to PMON mapping

[Jiri Olsa]

On Tue, Jan 14, 2020 at 04:24:34PM +0300, Sudarikov, Roman wrote:

SNIP

> > >   {
> > >   	struct intel_uncore_pmu *pmus;
> > > @@ -950,10 +976,19 @@ static int __init uncore_type_init(struct intel_uncore_type *type, bool setid)
> > >   			attr_group->attrs[j] = &type->event_descs[j].attr.attr;
> > >   		type->events_group = &attr_group->group;
> > > -	}
> > > +	} else
> > > +		type->events_group = &empty_group;
> > Why???
> Hi Greg,
> 
> Technically, what I'm trying to do is to add an attribute which depends on
> the uncore pmu type and BIOS support. New attribute is added to the end of
> the attribute groups array. It appears that the events attribute group is
> optional for most of the uncore pmus for x86/intel, i.e. events_group =
> NULL.
> 
> NULL element in the middle of the attribute groups array "hides" all others
> attribute groups which follows that element.
> 
> To work around it, embedded NULL elements should be either removed from
> the attribute groups array [1] or replaced with empty attribute; see
> implementation above.
> 
> If both approaches are incorrect then please advice what would be correct
> solution for that case.

hi,
I think Greg is reffering to the recent cleanup where we used attribute
groups with is_vissible callbacks, you can check changes below:

b7c9b3927337 perf/x86/intel: Use ->is_visible callback for default group
6a9f4efe78af perf/x86: Use update attribute groups for default attributes
b657688069a2 perf/x86/intel: Use update attributes for skylake format
3ea40ac77261 perf/x86: Use update attribute groups for extra format
1f157286829c perf/x86: Use update attribute groups for caps
baa0c83363c7 perf/x86: Use the new pmu::update_attrs attribute group

jirka

> 
> [1] https://lore.kernel.org/lkml/20191210091451.6054-3-roman.sudarikov@xxxxxxxxxxxxxxx/
> 
> Thanks,
> Roman
> > Didn't we fix up the x86 attributes to work properly and not mess around
> > with trying to merge groups and the like?  Please don't perpetuate that
> > more...
> > 
> > thanks,
> > 
> > greg k-h
> 
>