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?

Seems sensible as a starting point.

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.

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

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.

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.

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.

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?

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.

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.

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

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.

@mhorne

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