Messages in this thread Patch in this message | | | Subject | Re: [PATCH V6 07/14] perf/x86/intel: Generic support for hardware TopDown metrics | From | "Liang, Kan" <> | Date | Tue, 21 Jul 2020 10:05:55 -0400 |
| |
On 7/21/2020 5:43 AM, Peter Zijlstra wrote: > On Fri, Jul 17, 2020 at 07:05:47AM -0700, kan.liang@linux.intel.com 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 >
| |