| Message ID | b8a14c14fcdf028939ebf57849863e8ae01743de.1686588640.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf/arm-cmn: Identifier support | expand |
On Mon, 12 Jun 2023, Robin Murphy wrote: > Expose a sysfs identifier encapsulating the CMN part number and revision > so that jevents can narrow down a fundamental set of possible events for > calculating metrics. Configuration-dependent aspects - such as whether a > given node type is present, and/or a given node ID is valid - are still > not covered, and in general it's hard to see how userspace could handle > them, so we won't be removing any data or validation logic from the > driver any time soon, but at least it's a step in a useful direction. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> Reviewed-and-tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> > --- > drivers/perf/arm-cmn.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c > index 8cf4ed932950..088dc5c690a4 100644 > --- a/drivers/perf/arm-cmn.c > +++ b/drivers/perf/arm-cmn.c > @@ -1199,19 +1199,31 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev, > static struct device_attribute arm_cmn_cpumask_attr = > __ATTR(cpumask, 0444, arm_cmn_cpumask_show, NULL); > > -static struct attribute *arm_cmn_cpumask_attrs[] = { > +static ssize_t arm_cmn_identifier_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); > + > + return sysfs_emit(buf, "%03x%02x\n", cmn->part, cmn->rev); > +} > + > +static struct device_attribute arm_cmn_identifier_attr = > + __ATTR(identifier, 0444, arm_cmn_identifier_show, NULL); > + > +static struct attribute *arm_cmn_other_attrs[] = { > &arm_cmn_cpumask_attr.attr, > + &arm_cmn_identifier_attr.attr, > NULL, > }; > > -static const struct attribute_group arm_cmn_cpumask_attr_group = { > - .attrs = arm_cmn_cpumask_attrs, > +static const struct attribute_group arm_cmn_other_attrs_group = { > + .attrs = arm_cmn_other_attrs, > }; > > static const struct attribute_group *arm_cmn_attr_groups[] = { > &arm_cmn_event_attrs_group, > &arm_cmn_format_attrs_group, > - &arm_cmn_cpumask_attr_group, > + &arm_cmn_other_attrs_group, > NULL > }; > > -- > 2.39.2.101.g768bb238c484.dirty > >
在 2023/6/13 上午1:16, Robin Murphy 写道: > Expose a sysfs identifier encapsulating the CMN part number and revision > so that jevents can narrow down a fundamental set of possible events for > calculating metrics. Configuration-dependent aspects - such as whether a > given node type is present, and/or a given node ID is valid - are still > not covered, and in general it's hard to see how userspace could handle > them, so we won't be removing any data or validation logic from the > driver any time soon, but at least it's a step in a useful direction. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/perf/arm-cmn.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c > index 8cf4ed932950..088dc5c690a4 100644 > --- a/drivers/perf/arm-cmn.c > +++ b/drivers/perf/arm-cmn.c > @@ -1199,19 +1199,31 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev, > static struct device_attribute arm_cmn_cpumask_attr = > __ATTR(cpumask, 0444, arm_cmn_cpumask_show, NULL); > > -static struct attribute *arm_cmn_cpumask_attrs[] = { > +static ssize_t arm_cmn_identifier_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); > + > + return sysfs_emit(buf, "%03x%02x\n", cmn->part, cmn->rev); > +} > + > +static struct device_attribute arm_cmn_identifier_attr = > + __ATTR(identifier, 0444, arm_cmn_identifier_show, NULL); > + > +static struct attribute *arm_cmn_other_attrs[] = { > &arm_cmn_cpumask_attr.attr, > + &arm_cmn_identifier_attr.attr, > NULL, > }; > > -static const struct attribute_group arm_cmn_cpumask_attr_group = { > - .attrs = arm_cmn_cpumask_attrs, > +static const struct attribute_group arm_cmn_other_attrs_group = { > + .attrs = arm_cmn_other_attrs, > }; > > static const struct attribute_group *arm_cmn_attr_groups[] = { > &arm_cmn_event_attrs_group, > &arm_cmn_format_attrs_group, > - &arm_cmn_cpumask_attr_group, > + &arm_cmn_other_attrs_group, > NULL > }; > Tested-by: Jing Zhang <renyu.zj@linux.alibaba.com> Thanks, Jing
On 12/06/2023 18:16, Robin Murphy wrote: > Expose a sysfs identifier encapsulating the CMN part number and revision > so that jevents can narrow down a fundamental set of possible events for > calculating metrics. Configuration-dependent aspects - such as whether a > given node type is present, and/or a given node ID is valid - are still > not covered, and in general it's hard to see how userspace could handle > them, so we won't be removing any data or validation logic from the > driver any time soon, but at least it's a step in a useful direction. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> FWIW, Reviewed-by: John Garry <john.g.garry@oracle.com> However a comment, below. > --- > drivers/perf/arm-cmn.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c > index 8cf4ed932950..088dc5c690a4 100644 > --- a/drivers/perf/arm-cmn.c > +++ b/drivers/perf/arm-cmn.c > @@ -1199,19 +1199,31 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev, > static struct device_attribute arm_cmn_cpumask_attr = > __ATTR(cpumask, 0444, arm_cmn_cpumask_show, NULL); > > -static struct attribute *arm_cmn_cpumask_attrs[] = { > +static ssize_t arm_cmn_identifier_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); > + > + return sysfs_emit(buf, "%03x%02x\n", cmn->part, cmn->rev); I don't think that this gives the "0x" prefix, right? It is just an encoded string of values, so not strictly necessary or even appropriate, I suppose. However, if userspace wants to improve scalability by, say, matching an event for all revs of a model, userspace (perf tool) needs to be programmed in the JSONs somehow since we have no delimiter. Thanks, John > +} > + > +static struct device_attribute arm_cmn_identifier_attr = > + __ATTR(identifier, 0444, arm_cmn_identifier_show, NULL); > + > +static struct attribute *arm_cmn_other_attrs[] = { > &arm_cmn_cpumask_attr.attr, > + &arm_cmn_identifier_attr.attr, > NULL, > }; > > -static const struct attribute_group arm_cmn_cpumask_attr_group = { > - .attrs = arm_cmn_cpumask_attrs, > +static const struct attribute_group arm_cmn_other_attrs_group = { > + .attrs = arm_cmn_other_attrs, > }; > > static const struct attribute_group *arm_cmn_attr_groups[] = { > &arm_cmn_event_attrs_group, > &arm_cmn_format_attrs_group, > - &arm_cmn_cpumask_attr_group, > + &arm_cmn_other_attrs_group, > NULL > }; >
On 2023-06-14 15:55, John Garry wrote: > On 12/06/2023 18:16, Robin Murphy wrote: >> Expose a sysfs identifier encapsulating the CMN part number and revision >> so that jevents can narrow down a fundamental set of possible events for >> calculating metrics. Configuration-dependent aspects - such as whether a >> given node type is present, and/or a given node ID is valid - are still >> not covered, and in general it's hard to see how userspace could handle >> them, so we won't be removing any data or validation logic from the >> driver any time soon, but at least it's a step in a useful direction. >> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > FWIW, Reviewed-by: > John Garry <john.g.garry@oracle.com> > > However a comment, below. > >> --- >> drivers/perf/arm-cmn.c | 20 ++++++++++++++++---- >> 1 file changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c >> index 8cf4ed932950..088dc5c690a4 100644 >> --- a/drivers/perf/arm-cmn.c >> +++ b/drivers/perf/arm-cmn.c >> @@ -1199,19 +1199,31 @@ static ssize_t arm_cmn_cpumask_show(struct >> device *dev, >> static struct device_attribute arm_cmn_cpumask_attr = >> __ATTR(cpumask, 0444, arm_cmn_cpumask_show, NULL); >> -static struct attribute *arm_cmn_cpumask_attrs[] = { >> +static ssize_t arm_cmn_identifier_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); >> + >> + return sysfs_emit(buf, "%03x%02x\n", cmn->part, cmn->rev); > > I don't think that this gives the "0x" prefix, right? It is just an > encoded string of values, so not strictly necessary or even appropriate, > I suppose. Indeed that was deliberate, to emphasise that this is still a string (of hex digits), not a single numeric value. > However, if userspace wants to improve scalability by, say, matching an > event for all revs of a model, userspace (perf tool) needs to be > programmed in the JSONs somehow since we have no delimiter. FWIW I don't mind the original idea of prefix-based string matching - it feels like about the right level of compromise to give a sufficient degree of expressiveness without having to go as far as inventing some whole crazy expression syntax for interpreting values - so all I've really done here vs. Jing's patch is streamline the string itself. I'm still assuming the same general usage model, such that a hypothetical JSON encoding of, say, the hnf_snp_sent_cluster event could have: "Compat": {"43603","43c??"} /* CMN-650 r2p0, all CMN-700 */ (array and explicit wildcard syntax made up for the sake of implied debate - not sure if I have a particular preference either way) If we assume that over time, events are more likely to be added and stick around than to be removed, then what might be handy would be the additional notion of something like a "CompatExcept" property to describe negative matches. That could definitely scale better for the NOT_CMN600 and CMN_650ON cases in the current driver logic. Then what would also be really cool would be the ability to define events hierarchically based on other aliases - e.g. the driver could just expose a general "sbsx" sysfs alias as "type=5,eventid=?" if SBSX nodes are present, then the detailed events like "sbsx_rd_req" are somehow encoded in JSON as the equivalent of "sbsx,eventid=1" such that they're hidden if not relevant - at which point I think I could actually punt all the fiddly bits out of driver code into JSON :D Cheers, Robin.
diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c index 8cf4ed932950..088dc5c690a4 100644 --- a/drivers/perf/arm-cmn.c +++ b/drivers/perf/arm-cmn.c @@ -1199,19 +1199,31 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev, static struct device_attribute arm_cmn_cpumask_attr = __ATTR(cpumask, 0444, arm_cmn_cpumask_show, NULL); -static struct attribute *arm_cmn_cpumask_attrs[] = { +static ssize_t arm_cmn_identifier_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); + + return sysfs_emit(buf, "%03x%02x\n", cmn->part, cmn->rev); +} + +static struct device_attribute arm_cmn_identifier_attr = + __ATTR(identifier, 0444, arm_cmn_identifier_show, NULL); + +static struct attribute *arm_cmn_other_attrs[] = { &arm_cmn_cpumask_attr.attr, + &arm_cmn_identifier_attr.attr, NULL, }; -static const struct attribute_group arm_cmn_cpumask_attr_group = { - .attrs = arm_cmn_cpumask_attrs, +static const struct attribute_group arm_cmn_other_attrs_group = { + .attrs = arm_cmn_other_attrs, }; static const struct attribute_group *arm_cmn_attr_groups[] = { &arm_cmn_event_attrs_group, &arm_cmn_format_attrs_group, - &arm_cmn_cpumask_attr_group, + &arm_cmn_other_attrs_group, NULL };
Expose a sysfs identifier encapsulating the CMN part number and revision so that jevents can narrow down a fundamental set of possible events for calculating metrics. Configuration-dependent aspects - such as whether a given node type is present, and/or a given node ID is valid - are still not covered, and in general it's hard to see how userspace could handle them, so we won't be removing any data or validation logic from the driver any time soon, but at least it's a step in a useful direction. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/perf/arm-cmn.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)