Message ID | 20230628102949.2598096-5-john.g.garry@oracle.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | perf tool: sys event metric support re-write | expand |
On Wed, Jun 28, 2023 at 3:30 AM John Garry <john.g.garry@oracle.com> wrote: > > Add a function to get the events table associated with a metric table for > struct pmu_sys_events. > > We could also use something like: > struct pmu_sys_events *sys = container_of(metrics, struct pmu_sys_events, > metric_table); > > to lookup struct pmu_sys_events, but that relies on the user always passing > a sys events metric struct pointer, so this way is safer, but slower. If an event is specific to a particular PMU, shouldn't the metric name the PMU with the event? For example: MetricName: "IPC", MetricExpr: "instructions / cycles", Here instructions and cycles can wildcard match on BIG.little/hybrid systems and so we get an IPC metric for each PMU - although, I suspect this isn't currently quite working. We can also, and currently, do: MetricName: "IPC", MetricExpr: "cpu_atom@instructions@ / cpu_atom@cycles@", ... MetricName: "IPC", MetricExpr: "cpu_core@instructions@ / cpu_core@cycles@", The @ is used to avoid parsing confusion with / meaning divide. The PMUs for the events are explicitly listed here. We could say the PMU is implied but then it gets complex for uncore events, for metrics that mix core and uncore events. Thanks, Ian > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > tools/perf/pmu-events/empty-pmu-events.c | 6 ++++++ > tools/perf/pmu-events/jevents.py | 11 +++++++++++ > tools/perf/pmu-events/pmu-events.h | 3 +++ > 3 files changed, 20 insertions(+) > > diff --git a/tools/perf/pmu-events/empty-pmu-events.c b/tools/perf/pmu-events/empty-pmu-events.c > index a630c617e879..ae431b6bdf91 100644 > --- a/tools/perf/pmu-events/empty-pmu-events.c > +++ b/tools/perf/pmu-events/empty-pmu-events.c > @@ -290,6 +290,12 @@ int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table, pmu > return 0; > } > > +const struct pmu_events_table * > +sys_events_find_events_table(__maybe_unused const struct pmu_metrics_table *metrics) > +{ > + return NULL; > +} > + > const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu) > { > const struct pmu_events_table *table = NULL; > diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py > index 80b569b8634b..947e8b1efa26 100755 > --- a/tools/perf/pmu-events/jevents.py > +++ b/tools/perf/pmu-events/jevents.py > @@ -786,6 +786,17 @@ int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table, > return 0; > } > > +const struct pmu_events_table * > +sys_events_find_events_table(const struct pmu_metrics_table *metrics) > +{ > + for (const struct pmu_sys_events *tables = &pmu_sys_event_tables[0]; > + tables->name; tables++) { > + if (&tables->metric_table == metrics) > + return &tables->event_table; > + } > + return NULL; > +} > + > const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu) > { > const struct pmu_events_table *table = NULL; > diff --git a/tools/perf/pmu-events/pmu-events.h b/tools/perf/pmu-events/pmu-events.h > index caf59f23cd64..a3642c08e39d 100644 > --- a/tools/perf/pmu-events/pmu-events.h > +++ b/tools/perf/pmu-events/pmu-events.h > @@ -77,6 +77,9 @@ typedef int (*pmu_metric_iter_fn)(const struct pmu_metric *pm, > const struct pmu_metrics_table *table, > void *data); > > +const struct pmu_events_table * > +sys_events_find_events_table(const struct pmu_metrics_table *metrics); > + > int pmu_events_table_for_each_event(const struct pmu_events_table *table, pmu_event_iter_fn fn, > void *data); > int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table, pmu_metric_iter_fn fn, > -- > 2.35.3 >
On Fri, Jun 30, 2023 at 12:00 PM Ian Rogers <irogers@google.com> wrote: > > On Wed, Jun 28, 2023 at 3:30 AM John Garry <john.g.garry@oracle.com> wrote: > > > > Add a function to get the events table associated with a metric table for > > struct pmu_sys_events. > > > > We could also use something like: > > struct pmu_sys_events *sys = container_of(metrics, struct pmu_sys_events, > > metric_table); > > > > to lookup struct pmu_sys_events, but that relies on the user always passing > > a sys events metric struct pointer, so this way is safer, but slower. > > If an event is specific to a particular PMU, shouldn't the metric name > the PMU with the event? For example: > > MetricName: "IPC", > MetricExpr: "instructions / cycles", > > Here instructions and cycles can wildcard match on BIG.little/hybrid > systems and so we get an IPC metric for each PMU - although, I suspect > this isn't currently quite working. We can also, and currently, do: > > MetricName: "IPC", > MetricExpr: "cpu_atom@instructions@ / cpu_atom@cycles@", > ... > MetricName: "IPC", > MetricExpr: "cpu_core@instructions@ / cpu_core@cycles@", > > The @ is used to avoid parsing confusion with / meaning divide. The > PMUs for the events are explicitly listed here. We could say the PMU > is implied but then it gets complex for uncore events, for metrics > that mix core and uncore events. So looking at the later patches, they are making it so the PMU doesn't need to be specified, so I think it is the same issue as here. My thought was that the PMU would always be required for metrics like memory bandwidth per million instructions, ie >1 PMU. I know this makes the metrics longer, I've tried to avoid writing json metrics and have used Python to write them in my own work: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/metric.py?h=perf-tools-next#n411 Thanks, Ian > Thanks, > Ian > > > > Signed-off-by: John Garry <john.g.garry@oracle.com> > > --- > > tools/perf/pmu-events/empty-pmu-events.c | 6 ++++++ > > tools/perf/pmu-events/jevents.py | 11 +++++++++++ > > tools/perf/pmu-events/pmu-events.h | 3 +++ > > 3 files changed, 20 insertions(+) > > > > diff --git a/tools/perf/pmu-events/empty-pmu-events.c b/tools/perf/pmu-events/empty-pmu-events.c > > index a630c617e879..ae431b6bdf91 100644 > > --- a/tools/perf/pmu-events/empty-pmu-events.c > > +++ b/tools/perf/pmu-events/empty-pmu-events.c > > @@ -290,6 +290,12 @@ int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table, pmu > > return 0; > > } > > > > +const struct pmu_events_table * > > +sys_events_find_events_table(__maybe_unused const struct pmu_metrics_table *metrics) > > +{ > > + return NULL; > > +} > > + > > const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu) > > { > > const struct pmu_events_table *table = NULL; > > diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py > > index 80b569b8634b..947e8b1efa26 100755 > > --- a/tools/perf/pmu-events/jevents.py > > +++ b/tools/perf/pmu-events/jevents.py > > @@ -786,6 +786,17 @@ int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table, > > return 0; > > } > > > > +const struct pmu_events_table * > > +sys_events_find_events_table(const struct pmu_metrics_table *metrics) > > +{ > > + for (const struct pmu_sys_events *tables = &pmu_sys_event_tables[0]; > > + tables->name; tables++) { > > + if (&tables->metric_table == metrics) > > + return &tables->event_table; > > + } > > + return NULL; > > +} > > + > > const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu) > > { > > const struct pmu_events_table *table = NULL; > > diff --git a/tools/perf/pmu-events/pmu-events.h b/tools/perf/pmu-events/pmu-events.h > > index caf59f23cd64..a3642c08e39d 100644 > > --- a/tools/perf/pmu-events/pmu-events.h > > +++ b/tools/perf/pmu-events/pmu-events.h > > @@ -77,6 +77,9 @@ typedef int (*pmu_metric_iter_fn)(const struct pmu_metric *pm, > > const struct pmu_metrics_table *table, > > void *data); > > > > +const struct pmu_events_table * > > +sys_events_find_events_table(const struct pmu_metrics_table *metrics); > > + > > int pmu_events_table_for_each_event(const struct pmu_events_table *table, pmu_event_iter_fn fn, > > void *data); > > int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table, pmu_metric_iter_fn fn, > > -- > > 2.35.3 > >
On 30/06/2023 21:16, Ian Rogers wrote: > On Fri, Jun 30, 2023 at 12:00 PM Ian Rogers<irogers@google.com> wrote: >> On Wed, Jun 28, 2023 at 3:30 AM John Garry<john.g.garry@oracle.com> wrote: >>> Add a function to get the events table associated with a metric table for >>> struct pmu_sys_events. >>> >>> We could also use something like: >>> struct pmu_sys_events *sys = container_of(metrics, struct pmu_sys_events, >>> metric_table); >>> >>> to lookup struct pmu_sys_events, but that relies on the user always passing >>> a sys events metric struct pointer, so this way is safer, but slower. Hi Ian, >> If an event is specific to a particular PMU, shouldn't the metric name >> the PMU with the event? I am working on the basis - which I think is quite sane in case of sys events - that event names are unique to a PMU type. > For example: >> >> MetricName: "IPC", >> MetricExpr: "instructions / cycles", >> >> Here instructions and cycles can wildcard match on BIG.little/hybrid >> systems and so we get an IPC metric for each PMU - although, I suspect >> this isn't currently quite working. We can also, and currently, do: >> >> MetricName: "IPC", >> MetricExpr: "cpu_atom@instructions@ / cpu_atom@cycles@", >> ... >> MetricName: "IPC", >> MetricExpr: "cpu_core@instructions@ / cpu_core@cycles@", I did not know that it was possible to state that an event is for a specific PMU type in this fashion - is this feature new? Does it work only for known terms, like cycles and instructions? >> The @ is used to avoid parsing confusion with / meaning divide. The >> PMUs for the events are explicitly listed here. We could say the PMU >> is implied but then it gets complex for uncore events, for metrics >> that mix core and uncore events. So this works ok for IPC and CPU PMUs as we want the same event for many PMU types and naturally it would have the same name. I am still not sure that sys event metrics need to specify a PMU. > So looking at the later patches, they are making it so the PMU doesn't > need to be specified, Right, as we assume that we use uniquely named events. Having non-unique event names seems to create problems. > so I think it is the same issue as here. My > thought was that the PMU would always be required for metrics like > memory bandwidth per million instructions, ie >1 PMU. We treat these sys PMUs as standalone, and it makes no sense (currently) to have a metric which contains events for multiple PMU types as we don't know if the system is created with those PMUs, and, if it is, what topology etc. > I know this > makes the metrics longer, I've tried to avoid writing json metrics and > have used Python to write them in my own work: > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/metric.py?h=perf-tools-next*n411__;Iw!!ACWV5N9M2RV99hQ!PE_9BEFVCr25fA9OHzfEDuT-MncA5pnPf5C3eTqYnXGKG9Q2OItrEIiEYz1T366HjAayQmYtZ6N6WxPJBCI$ Thanks John
On Mon, Jul 3, 2023 at 8:15 AM John Garry <john.g.garry@oracle.com> wrote: > > On 30/06/2023 21:16, Ian Rogers wrote: > > On Fri, Jun 30, 2023 at 12:00 PM Ian Rogers<irogers@google.com> wrote: > >> On Wed, Jun 28, 2023 at 3:30 AM John Garry<john.g.garry@oracle.com> wrote: > >>> Add a function to get the events table associated with a metric table for > >>> struct pmu_sys_events. > >>> > >>> We could also use something like: > >>> struct pmu_sys_events *sys = container_of(metrics, struct pmu_sys_events, > >>> metric_table); > >>> > >>> to lookup struct pmu_sys_events, but that relies on the user always passing > >>> a sys events metric struct pointer, so this way is safer, but slower. > > Hi Ian, > > >> If an event is specific to a particular PMU, shouldn't the metric name > >> the PMU with the event? > > I am working on the basis - which I think is quite sane in case of sys > events - that event names are unique to a PMU type. > > > For example: > >> > >> MetricName: "IPC", > >> MetricExpr: "instructions / cycles", > >> > >> Here instructions and cycles can wildcard match on BIG.little/hybrid > >> systems and so we get an IPC metric for each PMU - although, I suspect > >> this isn't currently quite working. We can also, and currently, do: > >> > >> MetricName: "IPC", > >> MetricExpr: "cpu_atom@instructions@ / cpu_atom@cycles@", > >> ... > >> MetricName: "IPC", > >> MetricExpr: "cpu_core@instructions@ / cpu_core@cycles@", > > I did not know that it was possible to state that an event is for a > specific PMU type in this fashion - is this feature new? Does it work > only for known terms, like cycles and instructions? It has been in metrics a long time (I didn't choose that @ was the / replacement :-) ). It should work for all events. > >> The @ is used to avoid parsing confusion with / meaning divide. The > >> PMUs for the events are explicitly listed here. We could say the PMU > >> is implied but then it gets complex for uncore events, for metrics > >> that mix core and uncore events. > > So this works ok for IPC and CPU PMUs as we want the same event for many > PMU types and naturally it would have the same name. > > I am still not sure that sys event metrics need to specify a PMU. There was a similar thought for hybrid metrics. The PMU could be implied from the PMU of the metric. I think there can be confusion from an implied PMU, for example the cycles event without a PMU will open two events on a hybrid CPU. If we imply the PMU then it can mean just 1 PMU, but if the PMU doesn't have the event presumably it means the multiple PMU behavior. In parse-events there is existing logic to wildcard events but to ignore those that don't match a given PMU. This is used to support the --cputype option in builtin-stat.c, there is a similar option for builtin-list.c. We can use this so that events in a metric only match the PMU of the metric. Currently there are core metrics but whose events are all uncore like: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/arch/x86/alderlake/adl-metrics.json?h=perf-tools-next#n1802 So we'd need to move these metrics to be on the appropriate uncore PMU. Supporting >1 PMU in a metric wouldn't work though as it would appear the event was missing. Having the metric specify the PMU avoids these problems, but is verbose. Thanks, Ian > > So looking at the later patches, they are making it so the PMU doesn't > > need to be specified, > > Right, as we assume that we use uniquely named events. Having non-unique > event names seems to create problems. > > > so I think it is the same issue as here. My > > thought was that the PMU would always be required for metrics like > > memory bandwidth per million instructions, ie >1 PMU. > > We treat these sys PMUs as standalone, and it makes no sense (currently) > to have a metric which contains events for multiple PMU types as we > don't know if the system is created with those PMUs, and, if it is, what > topology etc. > > > I know this > > makes the metrics longer, I've tried to avoid writing json metrics and > > have used Python to write them in my own work: > > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/metric.py?h=perf-tools-next*n411__;Iw!!ACWV5N9M2RV99hQ!PE_9BEFVCr25fA9OHzfEDuT-MncA5pnPf5C3eTqYnXGKG9Q2OItrEIiEYz1T366HjAayQmYtZ6N6WxPJBCI$ > > Thanks > John >
>>>> MetricExpr: "cpu_core@instructions@ / cpu_core@cycles@", >> I did not know that it was possible to state that an event is for a >> specific PMU type in this fashion - is this feature new? Does it work >> only for known terms, like cycles and instructions? > It has been in metrics a long time (I didn't choose that @ was the / > replacement
On Wed, Jul 12, 2023 at 3:55 AM John Garry <john.g.garry@oracle.com> wrote: > > > >>>> MetricExpr: "cpu_core@instructions@ / cpu_core@cycles@", > >> I did not know that it was possible to state that an event is for a > >> specific PMU type in this fashion - is this feature new? Does it work > >> only for known terms, like cycles and instructions? > > It has been in metrics a long time (I didn't choose that @ was the / > > replacement
On 12/07/2023 18:52, Ian Rogers wrote: >> To be crystal clear, when you say ">1 PMU", do you mean ">1 PMU instance >> of the same type" or ">1 PMU type"? > So I'm meaning that if you have a MetricExpr where the events are from >> 1 PMU, for example memory bandwidth coming from uncore PMUs and then > instructions from a core PMU, and you do something like a ratio > between these two. > >> in a metric wouldn't work though as it would >>> appear the event was missing. Having the metric specify the PMU avoids >>> these problems, but is verbose. >> The message I'm getting - correct me if I am wrong - is that you would >> still prefer the PMU specified per event in metric expr, right? We don't >> do that exactly for sys PMU metrics today - we specify "Unit" instead, like: >> >> MetricExpr: "sys_pmu_bar_event_foo1 + sys_pmu_bar_event_foo2" >> Compat: "baz" >> Unit:"sys_pmu_bar" >> >> And so you prefer something like the following, right? >> MetricExpr: "sys_pmu_foo@bar1@ + sys_pmu_foo@bar2@" >> >> If so, I think that is ok - I just want to get rid of Unit and Compat. > I think we're agreeing
On Thu, Jul 13, 2023 at 8:07 AM John Garry <john.g.garry@oracle.com> wrote: > > On 12/07/2023 18:52, Ian Rogers wrote: > >> To be crystal clear, when you say ">1 PMU", do you mean ">1 PMU instance > >> of the same type" or ">1 PMU type"? > > So I'm meaning that if you have a MetricExpr where the events are from > >> 1 PMU, for example memory bandwidth coming from uncore PMUs and then > > instructions from a core PMU, and you do something like a ratio > > between these two. > > > >> in a metric wouldn't work though as it would > >>> appear the event was missing. Having the metric specify the PMU avoids > >>> these problems, but is verbose. > >> The message I'm getting - correct me if I am wrong - is that you would > >> still prefer the PMU specified per event in metric expr, right? We don't > >> do that exactly for sys PMU metrics today - we specify "Unit" instead, like: > >> > >> MetricExpr: "sys_pmu_bar_event_foo1 + sys_pmu_bar_event_foo2" > >> Compat: "baz" > >> Unit:"sys_pmu_bar" > >> > >> And so you prefer something like the following, right? > >> MetricExpr: "sys_pmu_foo@bar1@ + sys_pmu_foo@bar2@" > >> > >> If so, I think that is ok - I just want to get rid of Unit and Compat. > > I think we're agreeing
On 13/07/2023 22:35, Ian Rogers wrote: >> >> { >> "MetricExpr": "pmu_unit@event_foo@ * pmu_unit@event_bar@ * 2", >> "MetricName": "metric_baz", >> }, >> { >> "EventCode": "0x84", >> "EventName": "event_foo ", >> "Compat": "0x00000040", >> "Unit": "pmu_unit" >> }, >> { >> "EventCode": "0x85", >> "EventName": "event_bar ", >> "Compat": "0x00000040", >> "Unit": "pmu_unit" >> }, >> >> And we have a pmu with name and hw id matching "pmu_unit" and >> "0x00000040" present, that we select metric metric_baz for soc_b > Not sure I'm fully understanding.With the sysfs layout we'd have to > have a way of supporting CPUIDs, we could have a mapfile.csv style > approach or perhaps encode the CPUID into the path. It is complex as > CPUIDs are wildcards in the tool. I am not sure why you mention CPUIDs. sys events and their metrics are matched only on Unit and Compat. Furthermore, my solution here is based what we have today, and would not be based on this sysfs solution which you mention. > >>> I think Unit may be useful, say on Intel >>> hybrid I want the tma_fround_bound metric on just cpu_atom. Currently >>> the use of Unit is messy for metrics, ie uncore metrics are associated >>> with core PMUs, and what to do with a MetricExpr with >1 PMU. I think >>> we're learning from trying. I'm just hoping the migration to a sysfs >>> style layout will still be possible, as I can see lots of upside in >>> terms of testing, 1 approach, etc. >> Do you have an RFC or something for this "sysfs style layout"? I think >> that it would be easier for me to understand your idea by seeing the SW. > When I get a chance
On Fri, Jul 14, 2023 at 4:58 AM John Garry <john.g.garry@oracle.com> wrote: > > On 13/07/2023 22:35, Ian Rogers wrote: > >> > >> { > >> "MetricExpr": "pmu_unit@event_foo@ * pmu_unit@event_bar@ * 2", > >> "MetricName": "metric_baz", > >> }, > >> { > >> "EventCode": "0x84", > >> "EventName": "event_foo ", > >> "Compat": "0x00000040", > >> "Unit": "pmu_unit" > >> }, > >> { > >> "EventCode": "0x85", > >> "EventName": "event_bar ", > >> "Compat": "0x00000040", > >> "Unit": "pmu_unit" > >> }, > >> > >> And we have a pmu with name and hw id matching "pmu_unit" and > >> "0x00000040" present, that we select metric metric_baz for soc_b > > Not sure I'm fully understanding.With the sysfs layout we'd have to > > have a way of supporting CPUIDs, we could have a mapfile.csv style > > approach or perhaps encode the CPUID into the path. It is complex as > > CPUIDs are wildcards in the tool. > > I am not sure why you mention CPUIDs. sys events and their metrics are > matched only on Unit and Compat. > > Furthermore, my solution here is based what we have today, and would not > be based on this sysfs solution which you mention. > > > > >>> I think Unit may be useful, say on Intel > >>> hybrid I want the tma_fround_bound metric on just cpu_atom. Currently > >>> the use of Unit is messy for metrics, ie uncore metrics are associated > >>> with core PMUs, and what to do with a MetricExpr with >1 PMU. I think > >>> we're learning from trying. I'm just hoping the migration to a sysfs > >>> style layout will still be possible, as I can see lots of upside in > >>> terms of testing, 1 approach, etc. > >> Do you have an RFC or something for this "sysfs style layout"? I think > >> that it would be easier for me to understand your idea by seeing the SW. > > When I get a chance
On 14/07/2023 16:55, Ian Rogers wrote: > In this > series my main concern was in the changes of the event lookup and > having implied PMUs. You mentioned doing these changes so I was > waiting for a v2. OK, fine, I can look to do this now. BTW, which git repo/branch do you guys use for dev? I thought that it would be acme git, but Namhyung says "We moved to new repos from acme to perf/perf-tools and perf/perf-tools-next" - where is repo "perf"? Thanks, John
On Mon, Jul 17, 2023 at 12:41 AM John Garry <john.g.garry@oracle.com> wrote: > > On 14/07/2023 16:55, Ian Rogers wrote: > > In this > > series my main concern was in the changes of the event lookup and > > having implied PMUs. You mentioned doing these changes so I was > > waiting for a v2. > > OK, fine, I can look to do this now. > > BTW, which git repo/branch do you guys use for dev? I thought that it > would be acme git, but Namhyung says "We moved to new repos from acme to > perf/perf-tools and perf/perf-tools-next" - where is repo "perf"? Current development is here now: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next Thanks, Ian > Thanks, > John
On 17/07/2023 22:39, Ian Rogers wrote: > On Mon, Jul 17, 2023 at 12:41 AM John Garry<john.g.garry@oracle.com> wrote: >> On 14/07/2023 16:55, Ian Rogers wrote: >>> In this >>> series my main concern was in the changes of the event lookup and >>> having implied PMUs. You mentioned doing these changes so I was >>> waiting for a v2. >> OK, fine, I can look to do this now. I was thinking about this a little further. So you suggest that the metric expression contains PMU name per term, like "cpu_atom@instructions@ / cpu_atom@cycles@" - how would/could this work for PMUs with more complex naming, like the form hisi_siclXXX_cpa_YYY? Would we use the "Unit" expression for the metric name, like "@hisi_sicl,cpa@event_foo"? >> >> BTW, which git repo/branch do you guys use for dev? I thought that it >> would be acme git, but Namhyung says "We moved to new repos from acme to >> perf/perf-tools and perf/perf-tools-next" - where is repo "perf"? > Current development is here now: > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next__;!!ACWV5N9M2RV99hQ!OQDHOClSjd6nVZhmgzrK3RwzXuQpP54QhqyIKpITa_MFD4PLdS7yPYSnvInFja9nrFx9Sd-UnlsJ6XUqAh4$ Can that be added to the MAINTAINERS file? I suppose it is ok under "PERFORMANCE EVENTS SUBSYTEM", since the two would-be git repos listed under that same entry would be pretty obvious in purpose. Cheers, John
On Tue, Jul 18, 2023 at 2:32 AM John Garry <john.g.garry@oracle.com> wrote: > > On 17/07/2023 22:39, Ian Rogers wrote: > > On Mon, Jul 17, 2023 at 12:41 AM John Garry<john.g.garry@oracle.com> wrote: > >> On 14/07/2023 16:55, Ian Rogers wrote: > >>> In this > >>> series my main concern was in the changes of the event lookup and > >>> having implied PMUs. You mentioned doing these changes so I was > >>> waiting for a v2. > >> OK, fine, I can look to do this now. > > I was thinking about this a little further. So you suggest that the > metric expression contains PMU name per term, like > "cpu_atom@instructions@ / cpu_atom@cycles@" - how would/could this work > for PMUs with more complex naming, like the form hisi_siclXXX_cpa_YYY? > Would we use the "Unit" expression for the metric name, like > "@hisi_sicl,cpa@event_foo"? How does this work for events? The "@hisi_sicl,cpa@event_foo" looks strange, shouldn't it be "hisi_sicl,cpa@event_foo@" but then hisi_sicl looks like an event name. > > >> > >> BTW, which git repo/branch do you guys use for dev? I thought that it > >> would be acme git, but Namhyung says "We moved to new repos from acme to > >> perf/perf-tools and perf/perf-tools-next" - where is repo "perf"? > > Current development is here now: > > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next__;!!ACWV5N9M2RV99hQ!OQDHOClSjd6nVZhmgzrK3RwzXuQpP54QhqyIKpITa_MFD4PLdS7yPYSnvInFja9nrFx9Sd-UnlsJ6XUqAh4$ > > Can that be added to the MAINTAINERS file? I suppose it is ok under > "PERFORMANCE EVENTS SUBSYTEM", since the two would-be git repos listed > under that same entry would be pretty obvious in purpose. Arnaldo could you take a look at doing this? Thanks, Ian > Cheers, > John >
On 19/07/2023 16:25, Ian Rogers wrote: >> I was thinking about this a little further. So you suggest that the >> metric expression contains PMU name per term, like >> "cpu_atom@instructions@ / cpu_atom@cycles@" - how would/could this work >> for PMUs with more complex naming, like the form hisi_siclXXX_cpa_YYY? >> Would we use the "Unit" expression for the metric name, like >> "@hisi_sicl,cpa@event_foo"? > How does this work for events? The "@hisi_sicl,cpa@event_foo" looks > strange, shouldn't it be "hisi_sicl,cpa@event_foo@" but then hisi_sicl > looks like an event name. Yeah, that was a typo from me - like you say, it would be hisi_sicl,cpa@event_foo@ So is that what you would be suggesting then, such that we specify the PMU in the metric terms? It does look a bit odd :) > >>>> BTW, which git repo/branch do you guys use for dev? I thought that it >>>> would be acme git, but Namhyung says "We moved to new repos from acme to >>>> perf/perf-tools and perf/perf-tools-next" - where is repo "perf"? >>> Current development is here now: >>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next__;!!ACWV5N9M2RV99hQ!OQDHOClSjd6nVZhmgzrK3RwzXuQpP54QhqyIKpITa_MFD4PLdS7yPYSnvInFja9nrFx9Sd-UnlsJ6XUqAh4$ >> Can that be added to the MAINTAINERS file? I suppose it is ok under >> "PERFORMANCE EVENTS SUBSYTEM", since the two would-be git repos listed >> under that same entry would be pretty obvious in purpose. > Arnaldo could you take a look at doing this? Thanks, John
On Wed, Jul 19, 2023 at 8:37 AM John Garry <john.g.garry@oracle.com> wrote: > > On 19/07/2023 16:25, Ian Rogers wrote: > >> I was thinking about this a little further. So you suggest that the > >> metric expression contains PMU name per term, like > >> "cpu_atom@instructions@ / cpu_atom@cycles@" - how would/could this work > >> for PMUs with more complex naming, like the form hisi_siclXXX_cpa_YYY? > >> Would we use the "Unit" expression for the metric name, like > >> "@hisi_sicl,cpa@event_foo"? > > How does this work for events? The "@hisi_sicl,cpa@event_foo" looks > > strange, shouldn't it be "hisi_sicl,cpa@event_foo@" but then hisi_sicl > > looks like an event name. > > Yeah, that was a typo from me - like you say, it would be > hisi_sicl,cpa@event_foo@ > > So is that what you would be suggesting then, such that we specify the > PMU in the metric terms? It does look a bit odd :) I guess there could be some kind of regular expression syntax: (hisi_scl|cpa)@event_foo We have the "has_event" function now in the metric expressions: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/expr.c?h=perf-tools-next#n480 So it could be: hisi_scl@event_foo@ if has_event(hisi_scl@event_foo@) else cpa@event_foo@ Which has the advantage of working today, but the disadvantage of being verbose. Thanks, Ian > > > >>>> BTW, which git repo/branch do you guys use for dev? I thought that it > >>>> would be acme git, but Namhyung says "We moved to new repos from acme to > >>>> perf/perf-tools and perf/perf-tools-next" - where is repo "perf"? > >>> Current development is here now: > >>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next__;!!ACWV5N9M2RV99hQ!OQDHOClSjd6nVZhmgzrK3RwzXuQpP54QhqyIKpITa_MFD4PLdS7yPYSnvInFja9nrFx9Sd-UnlsJ6XUqAh4$ > >> Can that be added to the MAINTAINERS file? I suppose it is ok under > >> "PERFORMANCE EVENTS SUBSYTEM", since the two would-be git repos listed > >> under that same entry would be pretty obvious in purpose. > > Arnaldo could you take a look at doing this? > > Thanks, > John
Em Wed, Jul 19, 2023 at 08:25:31AM -0700, Ian Rogers escreveu: > On Tue, Jul 18, 2023 at 2:32 AM John Garry <john.g.garry@oracle.com> wrote: > > > > On 17/07/2023 22:39, Ian Rogers wrote: > > > On Mon, Jul 17, 2023 at 12:41 AM John Garry<john.g.garry@oracle.com> wrote: > > >> On 14/07/2023 16:55, Ian Rogers wrote: > > >>> In this > > >>> series my main concern was in the changes of the event lookup and > > >>> having implied PMUs. You mentioned doing these changes so I was > > >>> waiting for a v2. > > >> OK, fine, I can look to do this now. > > > > I was thinking about this a little further. So you suggest that the > > metric expression contains PMU name per term, like > > "cpu_atom@instructions@ / cpu_atom@cycles@" - how would/could this work > > for PMUs with more complex naming, like the form hisi_siclXXX_cpa_YYY? > > Would we use the "Unit" expression for the metric name, like > > "@hisi_sicl,cpa@event_foo"? > > How does this work for events? The "@hisi_sicl,cpa@event_foo" looks > strange, shouldn't it be "hisi_sicl,cpa@event_foo@" but then hisi_sicl > looks like an event name. > > > > > >> > > >> BTW, which git repo/branch do you guys use for dev? I thought that it > > >> would be acme git, but Namhyung says "We moved to new repos from acme to > > >> perf/perf-tools and perf/perf-tools-next" - where is repo "perf"? > > > Current development is here now: > > > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next__;!!ACWV5N9M2RV99hQ!OQDHOClSjd6nVZhmgzrK3RwzXuQpP54QhqyIKpITa_MFD4PLdS7yPYSnvInFja9nrFx9Sd-UnlsJ6XUqAh4$ > > > > Can that be added to the MAINTAINERS file? I suppose it is ok under > > "PERFORMANCE EVENTS SUBSYTEM", since the two would-be git repos listed > > under that same entry would be pretty obvious in purpose. > > Arnaldo could you take a look at doing this? Sure, just added this: [acme@quaco perf-tools-next]$ git show commit 0146244875046fad472a39ffd61ec4f91719b62a (HEAD -> perf-tools-next) Author: Arnaldo Carvalho de Melo <acme@redhat.com> Date: Wed Jul 19 16:53:01 2023 -0300 MAINTAINERS: Add git information for perf-tools and perf-tools-next trees/branches Now the perf tools development is done on these trees/branches: git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools.git perf-tools git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git perf-tools-next For a while I'll continue mirroring what is these to the same branches in my git tree. Suggested-by: John Garry <john.g.garry@oracle.com> Cc: Adrian Hunter <adrian.hunter@intel.com> Cc: Ian Rogers <irogers@google.com> Cc: Jiri Olsa <jolsa@kernel.org> Cc: Namhyung Kim <namhyung@kernel.org> Link: https://lore.kernel.org/lkml/CAP-5=fVGOP6-k=BTRd_bn=N0HVy+1ShpdW5rk5ND0ZGhm_fQkg@mail.gmail.com Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> diff --git a/MAINTAINERS b/MAINTAINERS index aee340630ecaea38..e351cfc7cd41c570 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -16629,6 +16629,8 @@ L: linux-kernel@vger.kernel.org S: Supported W: https://perf.wiki.kernel.org/ T: git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git perf/core +T: git git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools.git perf-tools +T: git git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git perf-tools-next F: arch/*/events/* F: arch/*/events/*/* F: arch/*/include/asm/perf_event.h [acme@quaco perf-tools-next]$
diff --git a/tools/perf/pmu-events/empty-pmu-events.c b/tools/perf/pmu-events/empty-pmu-events.c index a630c617e879..ae431b6bdf91 100644 --- a/tools/perf/pmu-events/empty-pmu-events.c +++ b/tools/perf/pmu-events/empty-pmu-events.c @@ -290,6 +290,12 @@ int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table, pmu return 0; } +const struct pmu_events_table * +sys_events_find_events_table(__maybe_unused const struct pmu_metrics_table *metrics) +{ + return NULL; +} + const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu) { const struct pmu_events_table *table = NULL; diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py index 80b569b8634b..947e8b1efa26 100755 --- a/tools/perf/pmu-events/jevents.py +++ b/tools/perf/pmu-events/jevents.py @@ -786,6 +786,17 @@ int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table, return 0; } +const struct pmu_events_table * +sys_events_find_events_table(const struct pmu_metrics_table *metrics) +{ + for (const struct pmu_sys_events *tables = &pmu_sys_event_tables[0]; + tables->name; tables++) { + if (&tables->metric_table == metrics) + return &tables->event_table; + } + return NULL; +} + const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu) { const struct pmu_events_table *table = NULL; diff --git a/tools/perf/pmu-events/pmu-events.h b/tools/perf/pmu-events/pmu-events.h index caf59f23cd64..a3642c08e39d 100644 --- a/tools/perf/pmu-events/pmu-events.h +++ b/tools/perf/pmu-events/pmu-events.h @@ -77,6 +77,9 @@ typedef int (*pmu_metric_iter_fn)(const struct pmu_metric *pm, const struct pmu_metrics_table *table, void *data); +const struct pmu_events_table * +sys_events_find_events_table(const struct pmu_metrics_table *metrics); + int pmu_events_table_for_each_event(const struct pmu_events_table *table, pmu_event_iter_fn fn, void *data); int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table, pmu_metric_iter_fn fn,
Add a function to get the events table associated with a metric table for struct pmu_sys_events. We could also use something like: struct pmu_sys_events *sys = container_of(metrics, struct pmu_sys_events, metric_table); to lookup struct pmu_sys_events, but that relies on the user always passing a sys events metric struct pointer, so this way is safer, but slower. Signed-off-by: John Garry <john.g.garry@oracle.com> --- tools/perf/pmu-events/empty-pmu-events.c | 6 ++++++ tools/perf/pmu-events/jevents.py | 11 +++++++++++ tools/perf/pmu-events/pmu-events.h | 3 +++ 3 files changed, 20 insertions(+)