| Message ID | cover.1600357241.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Arm CMN-600 PMU driver | expand |
On 18/09/2020, 16:29, "Robin Murphy" <robin.murphy@arm.com> wrote: > Here's an update with some very minor cleanup tweaks, plus a proposal > for some more in-depth debug information. Given that nobody seems to > have any significant complaints about the interface, I assume we're all > happy to merge the basic driver as-is and fix anything later if needed. > > Robin. Agree with merging the driver as-is. Tested both the updated driver and the debugfs on Graviton2 (ACPI). Both map and initial numbers seem right. Tested-by: Tsahi Zidenberg <tsahee@amazon.com> May I offer a small addition to the documentation? Something along these lines: --- a/Documentation/admin-guide/perf/arm-cmn.rst +++ b/Documentation/admin-guide/perf/arm-cmn.rst @@ -41,8 +41,14 @@ specified by "occupid". By default each event provides an aggregate count over all nodes of the given type. To target a specific node, "bynodeid" must be set to 1 and -"nodeid" to the appropriate value derived from the CMN configuration -(as defined in the "Node ID Mapping" section of the TRM). +"nodeid" to the appropriate value derived from the CMN configuration. + +The CMN map is available in /sys/kernel/debug/arm-cmn/map. +A nodeid could be calculated with this formulae: + node-id = d | (p << 2) | (y << 3) | (x << (3 + xybits)) +where: + x,y,p,d are node location as can be seen in the map + xybits is 2 for meshes <= 2*2, and 3 otherwise. Watchpoints -----------
On Mon, Oct 05, 2020 at 09:16:13AM +0000, Zidenberg, Tsahi wrote: > > > On 18/09/2020, 16:29, "Robin Murphy" <robin.murphy@arm.com> wrote: > > Here's an update with some very minor cleanup tweaks, plus a proposal > > for some more in-depth debug information. Given that nobody seems to > > have any significant complaints about the interface, I assume we're all > > happy to merge the basic driver as-is and fix anything later if needed. > > > > Robin. > > Agree with merging the driver as-is. > Tested both the updated driver and the debugfs on Graviton2 (ACPI). > Both map and initial numbers seem right. > > Tested-by: Tsahi Zidenberg <tsahee@amazon.com> > > May I offer a small addition to the documentation? > Something along these lines: > > --- a/Documentation/admin-guide/perf/arm-cmn.rst > +++ b/Documentation/admin-guide/perf/arm-cmn.rst > @@ -41,8 +41,14 @@ specified by "occupid". > > By default each event provides an aggregate count over all nodes of the > given type. To target a specific node, "bynodeid" must be set to 1 and > -"nodeid" to the appropriate value derived from the CMN configuration > -(as defined in the "Node ID Mapping" section of the TRM). > +"nodeid" to the appropriate value derived from the CMN configuration. > + > +The CMN map is available in /sys/kernel/debug/arm-cmn/map. > +A nodeid could be calculated with this formulae: > + node-id = d | (p << 2) | (y << 3) | (x << (3 + xybits)) > +where: > + x,y,p,d are node location as can be seen in the map > + xybits is 2 for meshes <= 2*2, and 3 otherwise. I already queued the driver, but if you send this as a stand-alone patch (on top of the arm64 for-next/perf branch) then I'm happy to take it. Thanks, Will > Watchpoints > ----------- >
On 2020-10-05 11:21, Will Deacon wrote: > On Mon, Oct 05, 2020 at 09:16:13AM +0000, Zidenberg, Tsahi wrote: >> >> >> On 18/09/2020, 16:29, "Robin Murphy" <robin.murphy@arm.com> wrote: >> > Here's an update with some very minor cleanup tweaks, plus a proposal >> > for some more in-depth debug information. Given that nobody seems to >> > have any significant complaints about the interface, I assume we're all >> > happy to merge the basic driver as-is and fix anything later if needed. >> > >> > Robin. >> >> Agree with merging the driver as-is. >> Tested both the updated driver and the debugfs on Graviton2 (ACPI). >> Both map and initial numbers seem right. >> >> Tested-by: Tsahi Zidenberg <tsahee@amazon.com> >> >> May I offer a small addition to the documentation? >> Something along these lines: >> >> --- a/Documentation/admin-guide/perf/arm-cmn.rst >> +++ b/Documentation/admin-guide/perf/arm-cmn.rst >> @@ -41,8 +41,14 @@ specified by "occupid". >> >> By default each event provides an aggregate count over all nodes of the >> given type. To target a specific node, "bynodeid" must be set to 1 and >> -"nodeid" to the appropriate value derived from the CMN configuration >> -(as defined in the "Node ID Mapping" section of the TRM). >> +"nodeid" to the appropriate value derived from the CMN configuration. >> + >> +The CMN map is available in /sys/kernel/debug/arm-cmn/map. >> +A nodeid could be calculated with this formulae: >> + node-id = d | (p << 2) | (y << 3) | (x << (3 + xybits)) >> +where: >> + x,y,p,d are node location as can be seen in the map >> + xybits is 2 for meshes <= 2*2, and 3 otherwise. Note that I disagree strongly with removing the reference to the documentation that canonically defines the node ID format, especially when the proposal is to replace it with a harder-to-read definition that is also wrong ;) If there's actually interest in using the coordinate format rather than just whole ID values, then we should generate format attributes that overlay the appropriate sections of the nodeid field. I haven't tried to implement that so far since it's not entirely trivial and I wasn't sure if anyone would care. > I already queued the driver, but if you send this as a stand-alone patch (on > top of the arm64 for-next/perf branch) then I'm happy to take it. Ha, so much for taking my holiday after -rc6 and expecting to come back to chasing this up at -rc1... cheers Will, that was a very nice surprise to return to! Robin.
On 14/10/2020, 19:31, "Robin Murphy" <robin.murphy@arm.com> wrote: > On Mon, Oct 05, 2020 at 09:16:13AM +0000, Zidenberg, Tsahi wrote: >> By default each event provides an aggregate count over all nodes of the >> given type. To target a specific node, "bynodeid" must be set to 1 and >> -"nodeid" to the appropriate value derived from the CMN configuration >> -(as defined in the "Node ID Mapping" section of the TRM). >> +"nodeid" to the appropriate value derived from the CMN configuration. >> + >> +The CMN map is available in /sys/kernel/debug/arm-cmn/map. >> +A nodeid could be calculated with this formulae: >> + node-id = d | (p << 2) | (y << 3) | (x << (3 + xybits)) >> +where: >> + x,y,p,d are node location as can be seen in the map >> + xybits is 2 for meshes <= 2*2, and 3 otherwise. > Note that I disagree strongly with removing the reference to the > documentation that canonically defines the node ID format, especially > when the proposal is to replace it with a harder-to-read definition that > is also wrong ;) Would be happy to be corrected :) > If there's actually interest in using the coordinate format rather than > just whole ID values, then we should generate format attributes that > overlay the appropriate sections of the nodeid field. I haven't tried to > implement that so far since it's not entirely trivial and I wasn't sure > if anyone would care. I agree that over 90% of the work with the counters should be with aggregate values that the driver already handles well by default. At the next level, there might be a lot to gain from just understanding if the counter in question is going up across the board, or just for a specific node. Are there a lot of HNF cache misses in the system, or from one specific HNF? To answer that question, the developer doesn't need to hold the SOC SPEC, just to have the list of relevant node-ids per type/counter. I really like the arm-cmn/map you created. There is still a small missing link between that map to relevant node-ids for the above. In my opinion, looking at specific node-ids would be a rare enough task, requiring enough expertise, that I am comfortable putting a formulae in documentation without actually modifying API, at least at this stage. Thank you! Tsahi.