Re: [PATCH v15 19/32] x86/resctrl: Find and enable usable telemetry events

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 12/4/25 12:53 PM, Tony Luck wrote:
> Every event group has a private copy of the data of all telemetry event
> aggregators (aka "telemetry regions") tracking its feature type. Included
> may be regions that have the same feature type but tracking different guid
> from the event group's.
> 
> Traverse the event group's telemetry region data and mark all regions that
> are not usable by the event group as unusable by clearing those regions'
> MMIO addresses. A region is considered unusable if:
> 1) guid does not match the guid of the event group.
> 2) Package ID is invalid.
> 3) The enumerated size of the MMIO region does not match the expected
>    value from the XML description file.
> 
> Hereafter any telemetry region with an MMIO address is considered valid for
> the event group it is associated with.
> 
> Enable all the event group's events as long as there is at least one usable
> region from where data for its events can be read. Enabling of events
> can fail. Warn the user if none of the events in an event group can be enabled.

"Warn the user ..." is clear from the patch. Please add explanation why it is ok to
proceed instead of fail when some of the events cannot be enabled.

> 
> Note that it is architecturally possible that some telemetry events are
> only supported by a subset of the packages in the system. It is not expected
> that systems will ever do this. If they do the user will see event files in
> resctrl that always return "Unavailable".
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---


...

> +
> +static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_PERF_PKG].r_resctrl;
> +	int skipped_events = 0;
> +
> +	if (!group_has_usable_regions(e, p))
> +		return false;
> +
> +	for (int j = 0; j < e->num_events; j++) {
> +		if (!resctrl_enable_mon_event(e->evts[j].id, true,
> +					      e->evts[j].bin_bits, &e->evts[j]))
> +			skipped_events++;
> +	}
> +	if (e->num_events == skipped_events) {
> +		pr_info("No events enabled in %s %s:0x%x\n", r->name, e->pfname, e->guid);
> +		return false;
> +	}
> +
> +	return skipped_events < e->num_events;

Now with the "if (e->num_events == skipped_events)" snippet in this patch this can just
return "true" here, no? Doing so avoids the unnecessary churn of switching to "return true"
in patch 25.

Reinette