hwpmc/amd: add PerfMonV2 global-control path

[afscoelho_gmail.com]

Summary

Add support for AMD PerfMonV2 (Family 19h+) core counters, which need
both the per-counter EVSEL enable bit and the global GLOBAL_CTL bit set
to count. Detects PerfMonV2 at init and switches to v2-specific
start/stop/interrupt handlers; older CPUs and L3/DF counters keep using
the classic path unchanged. Adds a read-only sysctl,
kern.hwpmc.amd_perfmon_v2, to report which path is active.

[ali_mashtizadeh.com]

Maybe I'm missing something but it doesn't look like this feature is completely implemented.

sys/dev/hwpmc/hwpmc_amd.c

705

Shouldn't you maintain the mask and only enable core counters that are running otherwise you might get a spurious interrupt on the wrong counter.

780

This doesn't look right am I confused and you did it elsewhere. Shouldn't you use the global counter control to block the interrupts in a non-racey way. This is basically the same as the old code but you don't wait for the spurious interrupt.

1516

Are these printf's just for debugging I don't think we should keep them right?

[afscoelho_gmail.com]

In D58256#1336173, @ali_mashtizadeh.com wrote:

Maybe I'm missing something but it doesn't look like this feature is completely implemented.

I think it looks incomplete to you because this patch is just the foundation for the next one, LBR V2, so I intentionally didn't introduce changes to user visible behavior, pmcstat works exactly as before, and the only new sysctl reporting if the v2 path is active. These changes make counters to start/stop via EVSEL + GLOBAL_CTL pair, and the NMI handler finds overflows with one GLOBAL_STATUS read instead of polling every counter. But I may have missed something for this foundation, do you have someting in mind?

sys/dev/hwpmc/hwpmc_amd.c

705

Everything I did here was to mimic what Linux does, and in Linux amd_pmu_core_enable_all() writes the full counter mask unconditionally and so counter that is not running is already frozen by its own EVSEL Enable bit. Because of that, it can`t raise an NMI.

780

Hmm, you are right! I got confused because of GLOBAL_CTL. From what I got, clearing this counter's GLOBAL_CTL bit stops it immediately, the hardware makes an AND between it and the EVSEL bit, so dropping any of they freezes the counter immediately . This is why the race will not happen. But, what I did wrong was to not wait for the possible in flight overflow NMI. Thanks for noticing it, I will fix in the next patch

1516

yeah, its a left over : )

[mhorne]

Seems sensible as a starting point.

sys/dev/hwpmc/hwpmc_amd.c

944–947

Slightly more idiomatic/efficient way to loop over a bitmap is with ffsll():

status = rdmsr(AMD_PMC_GLOBAL_STATUS);
status &= amd_global_cntr_mask;

int idx;
while ((idx = ffsll(status) - 1) != 0) {
    status &= ~(1ul << idx);

    /* handle overflow */
}

(Please double check the return of ffsll() to figure out exactly where the - 1 is needed)

[ali_mashtizadeh.com]

I think there’s some flexibility in how we use it but as I discussed in the meeting last week, the main thing Linux used it for is to reduce the overhead of the wrmsrs.

That’s why I suggested you need to extend the pcd callback with a start/stop all calls that you can use to commit your bitvector to the msr register once you record all the updates.

[afscoelho_gmail.com]

Thanks for the reviews. I pushed the fixes in the latest diff.

[afscoelho_gmail.com]

I stacked two patches here, D58571 and D58572. Both are false-positive KASSERTs in pmc_capture_user_callchain() that only fire on INVARIANTS kernels. @janeri hit them while stress-testing with INVARIANTS enabled.

[ali_mashtizadeh.com]

This looks like a step in the right direction and the API looks reasonable, but I'm not sure you thought through all the use cases. Someone concurrently using a system wide counter to monitor something at the same time as a process wide counter is running. E.g. two different pmcstat instances:

pmcstat -s instructions -w 1 # You don't want to stop the systemwide instance.
pmcstat -p instructions /bin/ls

I think you need to keep track of which counters you've been asked to start/stop on a per-CPU basis and use the new registers to stop all of those counters, but none of the other counters that might concurrently be running. I would keep the per-counter start/stop but just update a per-cpu mask that gets applied after in the call to startall/stopall.

[ali_mashtizadeh.com]

sys/dev/hwpmc/hwpmc_mod.c

5202

This is where I'm confused it looks like you half did what I said but if I'm understanding the logic then you don't know which counters belong to this process and which ones belong to other processes or are system wide.

[afscoelho_gmail.com]

In D58256#1346324, @ali_mashtizadeh.com wrote:

This looks like a step in the right direction and the API looks reasonable, but I'm not sure you thought through all the use cases. Someone concurrently using a system wide counter to monitor something at the same time as a process wide counter is running. E.g. two different pmcstat instances:

pmcstat -s instructions -w 1 # You don't want to stop the systemwide instance.
pmcstat -p instructions /bin/ls

I think you need to keep track of which counters you've been asked to start/stop on a per-CPU basis and use the new registers to stop all of those counters, but none of the other counters that might concurrently be running. I would keep the per-counter start/stop but just update a per-cpu mask that gets applied after in the call to startall/stopall.

I think the latest diff is already doing what you're suggesting, but I didn't explain the two masks good enough.

pc_global_mask is the complete value we want in GLOBAL_CTL for that CPU. System-wide counters update it directly. Process counters are tracked separately in pc_virtual_mask. start_all adds those bits to the global mask, and stop_all removes those same bits. The system-wide bits stay untouched.

So in your example, if the system counter uses bit 0 and the process counter uses bit 1, the mask goes 01 -> 11 -> 01. The system counter keeps running. The rules that already exist for row allocation prevent system and process counters from sharing the same bit.

I tested this exact case on my zen 4 machine, with both counters fixed to CPU 0. pmccontrol showed one STANDALONE row and one THREAD row at the same time, and the system-wide count kept increasing before, during, and after the process counter stopped.

sys/dev/hwpmc/hwpmc_mod.c

5202

The mask here isn't meant to track the process itself. It tracks which virtual counter rows are currently running on this CPU. stop_all only removes the bits in pc_virtual_mask. System wide bits stay in pc_global_mask, and other CPUs have their own masks. So this stops the exiting process's counters without affecting the system counter or process counters running on another CPU.

[afscoelho_gmail.com]

This comment was removed by afscoelho_gmail.com.