Page MenuHomeFreeBSD

hwpmc : fix perf counter MSR access
ClosedPublic

Authored by shreyankamartya229_gmail.com on Sep 6 2019, 4:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 19, 8:41 PM
Unknown Object (File)
Tue, Mar 19, 8:41 PM
Unknown Object (File)
Tue, Mar 19, 8:41 PM
Unknown Object (File)
Tue, Mar 19, 8:41 PM
Unknown Object (File)
Mon, Mar 11, 4:05 AM
Unknown Object (File)
Tue, Mar 5, 3:49 AM
Unknown Object (File)
Sat, Mar 2, 11:11 PM
Unknown Object (File)
Feb 7 2024, 10:38 AM

Details

Summary

amd_intr() does not account for the offset (0x200) in the counter MSR address and ends up accessing invalid regions while reading counter value after the 4th counter (0xC001000[8,9,..]) and erroneously updates the counter values for counters [1-4].

Test Plan

Tested on amd64 platform

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

shreyankamartya229_gmail.com edited the test plan for this revision. (Show Details)

amd_intr() should only check core pmcs for interrupts since other types of pmcs (L3,DF) cannot generate interrupts.

Newer AMD processors have an NMI latency, due to which pmc NMI's are ignored in certain scenarios such as:
Stopping a pmc ( pcd_stop_pmc() (hwpmc_mod.c)) followed by marking the pmc as free ( pcd_config_pmc( , , NULL)). In this scenario we receive an NMI after the pmc has been marked free and the interrupt ends being ignored.

A similar fix for linux was done here:
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=914123fa39042e651d79eaf86bbf63a1b938dddf

PMCs interrupting at the same time are collapsed into one interrupt. With the current interrupt handler, the majority of samples are missing for one of the event when two such events are used (unhalted_cpu_cycles and retired_intructions). Out of the two, the one which is allocated first reports all the samples while the other one reports negligible numbers.

Adding a fix for NMI latency.
Similar fix done in linux: https://lore.kernel.org/patchwork/patch/1062166/

Although it it did not work for linux due to reasons mentioned here: https://lkml.org/lkml/2019/8/1/796
I think it should work fine on freebsd. Please correct me if I'm wrong.

one little note, for future uploads can you include context (as described on https://wiki.freebsd.org/Phabricator) e.g.

git diff -U999999 other-branch > change.diff
git show -U999999 <commit-hash> > change.diff
svn diff --diff-cmd=diff -x -U999999 > change.diff

Sorry for the delay in taking a look at this. Could you please upload a diff with lots of context (see the -U999999 parameter mentioned here: https://wiki.freebsd.org/Phabricator)?

sys/dev/hwpmc/hwpmc_amd.c
779

Opening braces should be on the preceding line (same comment applies below too).

836

We do not use C99 // style comments.

@markj, @emaste : Updated the diff as per suggestions. Please let me know of any other inaccuracies.

This fixes GPFs for me when using hwpmc on AMD Rome. The GPFs were triggered out of MSR writes in the amd hwpmc interrupt handler.

One more style(9) nit noted, no need to reupload for that. I don't know the AMD PMC implementation well enough to review/approve, but have no objection.

sys/dev/hwpmc/hwpmc_amd.c
879–880

style nit should be } else {

This revision was not accepted when it landed; it landed in state Needs Review.Nov 7 2019, 7:54 PM
This revision was automatically updated to reflect the committed changes.