Page MenuHomeFreeBSD

Add Power8 and Power9 PMCs
ClosedPublic

Authored by luporl on Jul 8 2021, 12:40 PM.

Details

Summary

Added support to allocate Power8 and 9 PMCs.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Very nice to see this change already, I was hoping someone would pick this up for PPC.

A couple of minor comments, but this looks pretty good.

lib/libpmc/libpmc_pmu_util.c
151

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.

348
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"

@mhorne Thanks for the review!

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

sys/dev/hwpmc/hwpmc_power8.c
308

Did you mean to add this back? It doesn't appear necessary.

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
295

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
leonardo.bianconi_eldorado.org.br added inline comments.
sys/dev/hwpmc/hwpmc_power8.c
295

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 added inline comments.
sys/dev/hwpmc/hwpmc_power8.c
295

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
289–292

nitpick: long statements should be indented with 4 spaces.

306–307

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
601

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
295

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)
lib/libpmc/libpmc_pmu_util.c
601

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.

lib/libpmc/libpmc_pmu_util.c
601

One mistake I've just noted in D31221: pmc_md_op_pmcallocate is a union, not a struct. Therefore the padding shouldn't be touched.

lib/libpmc/libpmc_pmu_util.c
601

Oh, that's true, I didn't realize it was a union, thanks for pointing it out.

luporl edited reviewers, added: leonardo.bianconi_eldorado.org.br; removed: luporl.

Commandeering revision to finish changes while Leonardo is out.

luporl marked an inline comment as done.
  • Fix event name mapping
  • Handle non-programmable PMCs (PMC5 and PMC6)
  • Add missing event counter check
This revision now requires review to proceed.Jul 21 2021, 6:46 PM

Looks good, thanks for finishing this. Will you handle the removal of the static power8 definitions as well?

sys/powerpc/include/pmc_mdep.h
12

No need to make this a struct.

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

Looks good, thanks for finishing this. Will you handle the removal of the static power8 definitions as well?

Yes, I'll post a new revision with it soon.

sys/powerpc/include/pmc_mdep.h
12

Ok, I added it to hold future fields, but it can be added later too, when needed.
I'll change it before committing.

This revision was automatically updated to reflect the committed changes.