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.

Signed-off-by: Andre Silva <[e-mail]>

[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

766

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.

875

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.

1693

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

766

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.

875

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

1693

yeah, its a left over : )

[mhorne]

Seems sensible as a starting point.

sys/dev/hwpmc/hwpmc_amd.c

1049–1052

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

5299

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

5299

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.

[afscoelho_gmail.com]

Hi @ali_mashtizadeh.com, sorry, I updated the diff without writing anything about it, so it probably looked like I ignored your comment. The two masks are in there now. pc_global_mask is just what we want GLOBAL_CTL to be on that CPU. System wide counters write to it directly. Process counters go into pc_virtual_mask first and only get OR'd into the global one in start_all, then cleared again in stop_all. So stop_all never clears a system wide bit, it only removes the bits it put there itself.

I tried your example on my Zen 4 box, both pmcstat pinned to CPU 0, and the system wide count kept increasing before, during and after the process one stopped. pmccontrol shows one STANDALONE and one THREAD row at the same time.

Can you take another look when you have time?

[ali_mashtizadeh.com]

Yes it does look right now, I had to think through it quite a bit I didn't notice the subtle change to fix it as you mentioned. I'm thinking through the atomic operations. If I'm understanding what you did, you are depending on acquire semantics to serve as an interrupt barriers (i.e., compiler memory barriers). I would feel better if we just used seq_cst atomics but I'll take a look over the code.

[afscoelho_gmail.com]

In D58256#1352246, @ali_mashtizadeh.com wrote:

Yes it does look right now, I had to think through it quite a bit I didn't notice the subtle change to fix it as you mentioned. I'm thinking through the atomic operations. If I'm understanding what you did, you are depending on acquire semantics to serve as an interrupt barriers (i.e., compiler memory barriers). I would feel better if we just used seq_cst atomics but I'll take a look over the code.

Thanks for the review. Just a quick thing about the fences that seq_cst has, I think they're not needed here because the masks are all cpu local.

[janeri_gmail.com]

Tested on AMD EPYC 9654 (Zen4, Family 19h Model 11h, 192 threads), FreeBSD -CURRENT.

Test suite pmu_perfmonv2_test -- 6/6 passed:

• pmv2_sysctl_active -- kern.hwpmc.amd_perfmon_v2 == 1 confirmed on boot
• pmv2_counting_nonzero -- ex_ret_instr ls_not_halted_cyc non-zero in process mode, V2 start/stop path exercised
• pmv2_system_wide -- system-wide alloc on cpu0, NMI overflow handler fires correctly
• pmv2_concurrent_alloc -- all 6 K8 core slots simultaneously with GLOBAL_CTL bitmask, no counter leakage
• pmv2_start_stop_no_leak -- 20 alloc/start/stop/release cycles, no GLOBAL_CTL bits stuck after release
• pmv2_l3_df_unaffected -- L3 DF counters unaffected by V2 path

[ali_mashtizadeh.com]

Had to think through the atomics a bit since your just using it for synchronization with the interrupt it looks right.

[afscoelho_gmail.com]

Update to the rebuilt PerfMonV2 grouping and multiplexing stack.

[afscoelho_gmail.com]

Hi @mhorne, I have pushed the latest diff with the rebuilt stack and all previous comments are resolved. Could you take another look when you get a chance? Thanks for your time.

[afscoelho_gmail.com]

@mhorne

[mhorne]

I do not feel able to review this, but I will merge today if I can.