-
Notifications
You must be signed in to change notification settings - Fork 3.2k
libpmc: Fix the L3 counters for AMD Zen 1-4 #1984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for taking the time to contribute to FreeBSD! There is an issue that needs to be resolved:
Note Please review CONTRIBUTING.md, then update and push your branch again. |
62990e5 to
595ccc4
Compare
mhorne
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a much better approach, thanks.
lib/libpmc/libpmc_pmu_util.c
Outdated
| printf("PMC pmu '%s' is not supported!\n", pe->pmu); | ||
| return (ENODEV); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| printf("PMC pmu '%s' is not supported!\n", pe->pmu); | |
| return (ENODEV); | |
| printf("PMC pmu '%s' is not supported!\n", pe->pmu); | |
| return (EOPNOTSUPP); |
I suggest EOPNOTSUPP as an appropriate error that is already documented in pmc_attach(3).
Printing the error message in this case is not objectionable to me, but is not typical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the pmc command line I couldn't tell several different errors apart. I believe were the only consumer of libpmc, AMDuProf uses our ioctl's directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the error reporting in libpmc/pmcstat/hwpmc is most sorely needing improvement, as many of the error codes are overloaded. Particularly in the allocation path.
I was not aware that AMDuProf uses the ioctls directly. That is somewhat unfortunate as it forces our hand to maintain these interfaces.
The PMC tools have been in maintenance mode/slow decay for many years. They break easily and subtly. I have a renewed interest in trying to define some kind of strategy here for how we might move forward w.r.t. maintenance and the prioritization of new/missing functionality.
So, let's keep in touch on this to discuss needs/wants when it comes to these tools.
On AMD processors libpmc was using the topic field (based on filename) to determine the counter's subclass. Unfortunately, the JSON definitions for AMD Zen 1-4 have the L3 counters in files shared with other counters. This change has libpmc to use the pmu field (which is derived from the Unit field in JSON) to determine the correct counter subclass. Sponsored by: Netflix
595ccc4 to
cd8ae05
Compare
On AMD processors libpmc was using the topic field (based on filename) to determine the counter's subclass. Unfortunately, the JSON definitions for AMD Zen 1-4 have the L3 counters in files shared with other counters.
This change has libpmc to use the pmu field (which is derived from the Unit field in JSON) to determine the correct counter subclass.
Sponsored by: Netflix