Re: [PATCH V6 07/14] perf/x86/intel: Generic support for hardware TopDown metrics

[Liang, Kan]

On 7/21/2020 5:43 AM, Peter Zijlstra wrote:
> On Fri, Jul 17, 2020 at 07:05:47AM -0700, kan.liang@xxxxxxxxxxxxxxx wrote:
> > @@ -1031,6 +1034,35 @@ int x86_schedule_events(struct cpu_hw_events *cpuc, int n, int *assign)
> >   	return unsched ? -EINVAL : 0;
> >   }
> >   
> > +static int add_nr_metric_event(struct cpu_hw_events *cpuc,
> > +			       struct perf_event *event,
> > +			       int *max_count, bool sibling)
> > +{
> > +	/* The TopDown metrics events cannot be shared. */
> > +	if (is_metric_event(event) &&
> > +	    (++cpuc->n_metric_event > INTEL_TD_METRIC_NUM)) {
> > +		cpuc->n_metric_event--;
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * Take the accepted metrics events into account for leader event.
> > +	 */
> > +	if (!sibling)
> > +		*max_count += cpuc->n_metric_event;
> > +	else if (is_metric_event(event))
> > +		(*max_count)++;
> > +
> > +	return 0;
> > +}
> > +
> > +static void del_nr_metric_event(struct cpu_hw_events *cpuc,
> > +				struct perf_event *event)
> > +{
> > +	if (is_metric_event(event))
> > +		cpuc->n_metric_event--;
> > +}
> > +
> >   /*
> >    * dogrp: true if must collect siblings events (group)
> >    * returns total number of events and error code
> > @@ -1066,6 +1098,10 @@ static int collect_events(struct cpu_hw_events *cpuc, struct perf_event *leader,
> >   		cpuc->pebs_output = is_pebs_pt(leader) + 1;
> >   	}
> >   
> > +	if (x86_pmu.intel_cap.perf_metrics &&
> > +	    add_nr_metric_event(cpuc, leader, &max_count, false))
> > +		return -EINVAL;
> > +
> >   	if (is_x86_event(leader)) {
> >   		if (n >= max_count)
> >   			return -EINVAL;
> > @@ -1082,6 +1118,10 @@ static int collect_events(struct cpu_hw_events *cpuc, struct perf_event *leader,
> >   		    event->state <= PERF_EVENT_STATE_OFF)
> >   			continue;
> >   
> > +		if (x86_pmu.intel_cap.perf_metrics &&
> > +		    add_nr_metric_event(cpuc, event, &max_count, true))
> > +			return -EINVAL;
> > +
> >   		if (n >= max_count)
> >   			return -EINVAL;
> >   
>
> Something like so perhaps ?
>
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -1035,24 +1035,14 @@ int x86_schedule_events(struct cpu_hw_ev
>   }
>   
>   static int add_nr_metric_event(struct cpu_hw_events *cpuc,
> -			       struct perf_event *event,
> -			       int *max_count, bool sibling)
> +			       struct perf_event *event)
>   {
> -	/* The TopDown metrics events cannot be shared. */
> -	if (is_metric_event(event) &&
> -	    (++cpuc->n_metric_event > INTEL_TD_METRIC_NUM)) {
> -		cpuc->n_metric_event--;
> -		return -EINVAL;
> +	if (is_metric_event(event)) {
> +		if (cpuc->n_metric == INTEL_TD_METRIC_NUM)
> +			return -EINVAL;
> +		cpuc->n_metric++;
>   	}
>   
> -	/*
> -	 * Take the accepted metrics events into account for leader event.
> -	 */
> -	if (!sibling)
> -		*max_count += cpuc->n_metric_event;
> -	else if (is_metric_event(event))
> -		(*max_count)++;
> -
>   	return 0;
>   }
>   
> @@ -1060,7 +1050,24 @@ static void del_nr_metric_event(struct c
>   				struct perf_event *event)
>   {
>   	if (is_metric_event(event))
> -		cpuc->n_metric_event--;
> +		cpuc->n_metric--;
> +}
> +
> +static int collect_event(struct cpu_hw_events *cpuc, struct perf_event *event,
> +			 int max_count, int n)
> +{
> +
> +	if (x86_pmu.intel_cap.perf_metrics && add_nr_metric_event(cpuc, event))
> +		return -EINVAL;
> +
> +	if (n >= max_count + cpuc->n_metric)
> +		return -EINVAL;
> +
> +	cpuc->event_list[n] = event;
> +	if (is_counter_pair(&event->hw))
> +		cpuc->n_pair++;
> +
> +	return 0;
>   }
>   
>   /*
> @@ -1098,37 +1105,20 @@ static int collect_events(struct cpu_hw_
>   		cpuc->pebs_output = is_pebs_pt(leader) + 1;
>   	}
>   
> -	if (x86_pmu.intel_cap.perf_metrics &&
> -	    add_nr_metric_event(cpuc, leader, &max_count, false))
> +	if (is_x86_event(leader) && collect_event(cpuc, leader, max_count, n))
>   		return -EINVAL;
> +	n++;

If a leader is not an x86 event, n will be mistakenly increased.
But is it possible that a leader is not an x86 event here?

Seems impossible for now. A SW event cannot be a leader for a mix group.
We don't allow the core PMU and the perf_invalid_context PMU in the same 
group.
If so, I think it deserves a comment, in case the situation changes 
later, e.g.,

 +	if (is_x86_event(leader) && collect_event(cpuc, leader, max_count, n))
  		return -EINVAL;
 +	/*
 +	 * Currently, for a x86 core event group, the leader must be a
 +	 * x86 core event. A SW event cannot be a leader for a mix
 +	 * group. We don't allow the core PMU and the perf_invalid_contex +	 
* PMU in the same group.
 +	 */
 +	n++;


Thanks,
Kan
>   
> -	if (is_x86_event(leader)) {
> -		if (n >= max_count)
> -			return -EINVAL;
> -		cpuc->event_list[n] = leader;
> -		n++;
> -		if (is_counter_pair(&leader->hw))
> -			cpuc->n_pair++;
> -	}
>   	if (!dogrp)
>   		return n;
>   
>   	for_each_sibling_event(event, leader) {
> -		if (!is_x86_event(event) ||
> -		    event->state <= PERF_EVENT_STATE_OFF)
> +		if (!is_x86_event(event) || event->state <= PERF_EVENT_STATE_OFF)
>   			continue;
>   
> -		if (x86_pmu.intel_cap.perf_metrics &&
> -		    add_nr_metric_event(cpuc, event, &max_count, true))
> -			return -EINVAL;
> -
> -		if (n >= max_count)
> +		if (collect_event(cpuc, event, max_count, n))
>   			return -EINVAL;
> -
> -		cpuc->event_list[n] = event;
>   		n++;
> -		if (is_counter_pair(&event->hw))
> -			cpuc->n_pair++;
>   	}
>   	return n;
>   }
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -313,7 +313,7 @@ struct cpu_hw_events {
>   	 * Perf Metrics
>   	 */
>   	/* number of accepted metrics events */
> -	int				n_metric_event;
> +	int				n_metric;
>   
>   	/*
>   	 * AMD specific bits