Page MenuHomeFreeBSD

libpmc: Handle PMCALLOCATE log with PMC code on PMU event system
ClosedPublic

Authored by jrtc27 on Apr 15 2023, 11:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 12 2024, 3:02 PM
Unknown Object (File)
Jan 10 2024, 1:59 AM
Unknown Object (File)
Dec 22 2023, 10:28 PM
Unknown Object (File)
Dec 22 2023, 9:44 PM
Unknown Object (File)
Dec 12 2023, 2:30 AM
Unknown Object (File)
Oct 27 2023, 3:50 PM
Unknown Object (File)
Oct 15 2023, 9:34 AM
Unknown Object (File)
Oct 15 2023, 9:34 AM
Subscribers

Details

Summary

On an arm64 system that reports as a Cortex A72 r0p3, running

pmcstat -P CPU_CYCLES command

works, but

pmcstat -P cpu-cycles command

does not. This is because the former uses the PMU event from the JSON
source, resulting in pl_event in the log event being a small index
(here, 5) into the generated events table, whilst the latter does not
match any of the JSON events and falls back on PMC's own tables, mapping
it to the PMC event 0x14111, i.e. PMC_EV_ARMV8_EVENT_11H. Then, when
libpmc gets the PMCALLOCATE event, it tries to use the event as an index
into the JSON-derived table, but doing so only makes sense for the
former, whilst for the latter it will go way out of bounds and either
read junk (which may trigger the != NULL assertion) or segfault. As far
as I can tell we don't have anything lying around to tell us which of
the two cases we're in, but we can exploit the fact that the first
0x1000 PMC event codes are reserved, and that none of our PMU events
tables reach that number of entries yet.

MFC after: 1 month

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There is an MD flag to distinguish between the two cases on arm64, PM_MD_RAW_EVENT. Given that we support pmu-events on x86, arm64, and now powerpc64, it is definitely appropriate and useful to make this flag MI and set it for pmc_op_pmcallocate.pm_flags whenever we get the event index from pmu-events. Then, the flag should be available as part of the log entry in pl_flags.

If you can do that as part of this fix then great, otherwise I will handle it as a follow-up. No objection to this as-is.

I believe PR 268857 also describes this issue.

There is an MD flag to distinguish between the two cases on arm64, PM_MD_RAW_EVENT. Given that we support pmu-events on x86, arm64, and now powerpc64, it is definitely appropriate and useful to make this flag MI and set it for pmc_op_pmcallocate.pm_flags whenever we get the event index from pmu-events. Then, the flag should be available as part of the log entry in pl_flags.

If you can do that as part of this fix then great, otherwise I will handle it as a follow-up. No objection to this as-is.

I believe PR 268857 also describes this issue.

It's not really a thing that other architectures want in the kernel, though; amd gets the hardware code from a->pm_md.pm_amd.pm_amd_config, ia[fp] get it from pm_md.pm_iap.pm_iap_config, power8 gets it from pm_md.pm_event and ucf seems to get it from pm_md.pm_ucp.pm_ucp_evsel in start rather than allocate. So introducing such an MI flag would, IMO, have to come with some much more invasive reworking of every architecture's hwpmc driver (and corresponding libpmc part). I wouldn't be opposed to such an overhaul - the current state is an inconsistent mess that's evolved rather than been designed - but that's quite an undertaking to do without breaking things.

@mhorne If you're happy for this to land as-is, could you please formally approve it?

Go ahead, your fix works and is suitable for MFC.

ACK to your other comments. I think the need to rework the class-dependent fields is overstated; there are uglier warts than these in this code. I will post something that iterates on this fix next week.

This revision is now accepted and ready to land.Jun 6 2023, 5:25 PM