Re: [PATCH v2 2/3] perf x86: Add compaction function for uncore attributes

[Sudarikov, Roman]

On 10.12.2019 13:37, Peter Zijlstra wrote:
> On Tue, Dec 10, 2019 at 12:14:50PM +0300, roman.sudarikov@xxxxxxxxxxxxxxx wrote:
> > From: Roman Sudarikov <roman.sudarikov@xxxxxxxxxxxxxxx>
> >
> > In current design, there is an implicit assumption that array of pointers
> > to uncore type attributes is NULL terminated. However, not all attributes
> > are mandatory for each Uncore unit type, e.g. "events" is required for
> > IMC but doesn't exist for CHA. That approach correctly supports only one
> > optional attribute which also must be the last in the row.
> > The patch removes limitation by safely removing embedded NULL elements.
> >
> > Co-developed-by: Alexander Antonov <alexander.antonov@xxxxxxxxx>
> > Signed-off-by: Alexander Antonov <alexander.antonov@xxxxxxxxx>
> > Signed-off-by: Roman Sudarikov <roman.sudarikov@xxxxxxxxxxxxxxx>
> > ---
> >   arch/x86/events/intel/uncore.c | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> >
> > diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> > index 24e120289018..a05352c4fc01 100644
> > --- a/arch/x86/events/intel/uncore.c
> > +++ b/arch/x86/events/intel/uncore.c
> > @@ -923,6 +923,22 @@ static void uncore_types_exit(struct intel_uncore_type **types)
> >   		uncore_type_exit(*types);
> >   }
> >   
> > +static void uncore_type_attrs_compaction(struct intel_uncore_type *type)
> > +{
> > +	int i, j;
> > +	int size = ARRAY_SIZE(type->attr_groups);
> > +
> > +	for (i = 0, j = 0; i < size; i++) {
> > +		if (!type->attr_groups[i])
> > +			continue;
> > +		if (i > j) {
> > +			type->attr_groups[j] = type->attr_groups[i];
> > +			type->attr_groups[i] = NULL;
> > +		}
> > +		j++;
> > +	}
> > +}
> GregKH had objections to us playing silly games like that and made us
> use is_visible() for the regular PMU driver. Also see commit:
>
>    baa0c83363c7 ("perf/x86: Use the new pmu::update_attrs attribute group")

Hi Peter,

if I understand correctly, that commit is intended for graceful handling 
of cases where we need to
probe hardware first before making decision whether to show or not that 
particular event for that particular pmu.
What I'm doing has slightly different context - I'm creating the mapping 
per pmu type.
That mapping is not hardware dependent but implementation dependent 
meaning that if the mapping is not implemented
for some pmu type, then the mapping attribute has no way to show up 
following current implementation, right?
If the mapping is implemented then it shows up for all pmus of that type.

I can rework it following the approach implemented in the commit you 
mentioned but the benefit is not immediately obvious :-)

Thanks,
Roman