Details

lib/libpmc/libpmc_pmu_util.c
152	It seems that this function could be eliminated altogether, since there is no distinction in behaviour between power8 and power9. To elaborate, the presence of a `pmc_cpuid` value matching the regex in `pmu-events/arch/powerpc/mapfile.csv` is enough to detect whether a CPU is supported or not. This is checked in `pmu_events_map_get()`.
sys/dev/hwpmc/hwpmc_power8.c
46	I would suggest splitting the removal of these events into a separate change.
193
sys/dev/hwpmc/hwpmc_powerpc.c
605	Worth having a simple comment here, something like `/* Set the value for kern.hwpmc.cpuid */`.

Removed "pmu_events_mfr" function
Did not remove event codes in this review
Added space setting "pcd->pcd_allocate_pmc"
Added comment when setting "kern.hwpmc.cpuid"

Harbormaster completed remote builds in B40397: Diff 91981.Jul 8 2021, 7:58 PM

@mhorne Thanks for the review!

Added a new version with requested changes.
I will send another one removing the event codes.

mhorne added inline comments.Jul 8 2021, 8:24 PM

sys/dev/hwpmc/hwpmc_power8.c
308	Did you mean to add this back? It doesn't appear necessary.

leonardo.bianconi_eldorado.org.br added inline comments.Jul 8 2021, 8:35 PM

sys/dev/hwpmc/hwpmc_power8.c
308	Yes, otherwise I would need to remove the enum events as they are not used anywhere.

Looks good. One final nitpick, but I won't block based on that alone.

sys/dev/hwpmc/hwpmc_power8.c
161	`0xFFFF` is the maximum width of the event code? A named macro would make this more obvious, and perhaps it's worth returning `EINVAL` if `pm_ev` exceeds this value.
308	Got it, thanks :)

This revision is now accepted and ready to land.Jul 9 2021, 2:22 PM

Removing and operation as it is not necessary.

This revision now requires review to proceed.Jul 11 2021, 4:12 PM

Harbormaster completed remote builds in B40443: Diff 92079.Jul 11 2021, 4:12 PM

leonardo.bianconi_eldorado.org.br marked an inline comment as done.Jul 11 2021, 4:19 PM

leonardo.bianconi_eldorado.org.br added inline comments.

sys/dev/hwpmc/hwpmc_power8.c
161	I tested and the config can receive the entire code to allocate the counter. I have extracted only the code event, removing the PMC flags, but it is not necessary as both ways work, so I removed the `and` operation. I didn't add a maximum value check to return `EINVAL` because it is not a continuous number. Do you think it is necessary to get the maximum value combination of PMC code and event code to add a check as you mentioned?

mhorne accepted this revision.Jul 13 2021, 1:58 PM

mhorne added inline comments.

sys/dev/hwpmc/hwpmc_power8.c
161	If there is no clear upper bound then it is not necessary to add anything. It is only helpful as a defensive check when the width of the event code is defined to be, for example, 16 bits wide.

This revision is now accepted and ready to land.Jul 13 2021, 1:58 PM

Overall the changes look good.

But I tested them on a POWER9 VM and it seems there is something wrong.

Running this:

stress -c 1 -t 5 & pmcstat -O sample.out -S pm_inst_cmpl -l 5
pmcstat -R sample.out -G sample.graph

resulted in sample.graph listing pm_cmplu_stall_fxlong events, instead of pm_inst_cmpl.
pmcstat -R sample.out -g also reports pm_cmplu_stall_fxlong events, instead of pm_inst_cmpl ones.

So, it seems either the wrong event is being selected or there is something wrong with the event to name mapping part.
An interesting thing is that pmcstat -L lists pm_cmplu_stall_fxlong as the third event (event 2 if counting from 0) and the event value of pm_inst_cmpl is 2.

Trying the same command with another event resulted in a core dump:

stress -c 1 -t 5 & pmcstat -O sample.out -S pm_inst_from_rl4 -l 5
pmcstat -R sample.out -G sample.graph
Assertion failed: (pme->table[idx].name), function pmc_pmu_event_get_by_idx, file /usr/src/lib/libpmc/libpmc_pmu_util.c, line 251.
Abort trap (core dumped)

Analysing it, gdb says that idx is 0x2404a.

Note however that these tests were performed using incremental kernel/world builds and only reinstalling the kernel, its modules, pmc libraries and binaries (because llvm 12 is currently broken on PowerPC).

sys/dev/hwpmc/hwpmc_power8.c
155–158	nitpick: long statements should be indented with 4 spaces.
172–173	nitpick: long statements should be indented with 4 spaces.

I have investigated the issue further and have some suggestions of how it can be fixed (inline).

These were tested on a VM and seem to work fine.

lib/libpmc/libpmc_pmu_util.c
622	It seems pm_ev needs to point to idx for libpmc to map correctly between names and events. The event code could be passed in a pm_md field, such as pm_md.pm_event. This field should be added to sys/powerpc/include/pmc_mdep.h, like: union pmc_md_op_pmcallocate { - uint64_t __pad[4]; + uint32_t pm_event; + uint32_t __pad1; + uint64_t __pad2[3]; };
sys/dev/hwpmc/hwpmc_power8.c
161	With pm_md.pm_event being passed from libpmc, config can be obtained from the lower half of it. The upper half holds the counter number. The counter value should be between 1 to 4 and it must match ri + 1, in order to select the correct PMC for the given config, unless counter is 0, then any PMC may be used. We should also handle 2 special cases: PMC5 and PMC6, that are not programmable, but always count instructions and cycles, respectively. This way, we save the other 4 programmable PMCs to count other events. I've seen some event codes in libpmc event database with counter values greater than 4. I don't know how they should be programmed, but I guess it's safe to treat them as invalid for now. The macros in the suggestion above are: #define PM_EVENT_CODE(pe) (pe & 0xffff) #define PM_EVENT_COUNTER(pe) ((pe >> 16) & 0xffff)

mhorne added inline comments.Jul 16 2021, 5:42 PM

lib/libpmc/libpmc_pmu_util.c
622	Thanks for narrowing this down, the use of `idx` vs `ped.ped_event` is not obvious at all in the amd64 code, and if you look you'll see that my arm64 version is accidentally incomplete :) I agree with the analysis.