Hello folks this is a quick draft I threw together to sketch out providing a unified interface for branch histories like LBR on Intel and AMD, and BRBE on ARM. I looped in everyone that's working on a relevant patch.

In D58534#1345537, @sarah.walker2_arm.com wrote:

In D58534#1345234, @ali_mashtizadeh.com wrote:

So is the reason we aren't hard coding this and integrating BRBE into the pmc module is that it will be used by hwt as well? Is this best exposed through hwt or is it just extended data that can be exposed throught he multipart flag I added for AMD IBS? The multipart flag lets you add arbitrary payloads alongside a counter. LBR will be using this as well.

Only problem I can see is the potential race on an unload when setting pmc_intr_callback to NULL.

It was decided that hwt was the best method to use for dumping BRBE samples, however BRBE uses the PMC interrupt which hwpmc owns. Integrating the BRBE functionality into hwpmc would therefore add a dependency between hwpmc and hwt which seemed less than ideal, hence this approach.

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:

So is the reason we aren't hard coding this and integrating BRBE into the pmc module is that it will be used by hwt as well? Is this best exposed through hwt or is it just extended data that can be exposed throught he multipart flag I added for AMD IBS? The multipart flag lets you add arbitrary payloads alongside a counter.

These two diffs should be merged but does it fix the -U flag bug? I don’t see why we need to keep the two path I had a patch to unify them but I hit a different assert.

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.

In D56979#1336240, @nickbanks_netflix.com wrote:

In D56979#1336171, @ali_mashtizadeh.com wrote:

I made a high level pass over the diff I think I have enough context to review it carefully now.

1. I wonder if it would be better to integrate with libctf to pull the enumerations and type information so that you don't have to be so verbose. Granted in the case of TCP, the definitions I'd worry about maintaining are tied to FreeBSD's ABI, but this might allow us to reduce the burden and just extract types straight from CTF.

From a quick look, it seems that CTF would miss out on any preprocessor stuff like #defines, which is actually what makes up most of the verbosity in the schemas. Also, it creates a weird dependency problem, because you'd need to build to then generate the schema. So, I think I'll push back on that for now. Perhaps a middle ground: in the transport calls, we've discussed adding some build-phase validation that throws a compile error if there is any discrepancy between the schema and builds (to eliminate the drift problem).

2. I think my prior point that I mentioned offline about allowing parameters when subscribing has a lot broader applicability. It's a lot of data when our machines are loaded, it might be useful to ask for a specific connection in the case of TCP and/or configure specific counters for pmc.

I agree it would be interesting. I see it as a way for the subscriber (i.e. elog cmd line) to pass provider-specific data/args that can do anything. For TCP, we have some out of band (to elog) hooks already (i.e. socket options to turn on elog for one connection), but having it directly integrated into elog (i.e. enable='tcp.port=443') would be cool. But I think it could easily be added as a follow on feature, and not block the base reviews which are already huge.

3. I've written a few lock data structures. The buffering strategy seems really complicated and it has a few drawbacks. I think you can use a simpler ring structure with two independent atomic variables for the head and tail. This avoids write-write contention between the reader-writer, and would eliminate the need for a lock on machines without a 64-bit swap operation.. To deal with nesting you could use a ready bit/byte to avoid double buffering. If you follow this approach it becomes relatively easy to reason about the safety of allowing the userspace process to drain the log from a shared memory buffer and probably about half as many lines of code in the core.

I will look into this and see how the performance compares.

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

I made a high level pass over the diff I think I have enough context to review it carefully now.

Any more comments on these or can someone land this stack? I got the remaining functionality I described that I would like to rebase and prepare for review.

Fix usage until other patch lands

Update magic

Tinderbox passes on my tree, except for unrelated ZFS build problems for amd64.LINT*.

Portability fixes

Portability fixes

Portability fixes

I'm running a build on a few architectures to make sure I don't break anything. I'll post when it's done.

Fix the study command defaults

Fix string length check

Misused git-arc sorry my first attempt so it should have any changes.

Thanks for your comments I fixed the armv7 and cleaned up the default/flamegraph to use unhalted-cycles. Seems that is defined now on x86 and ARM64 correctly, but I'll make a separate patch to add reasonable aliases for POWER8/9.

Also can someone with access on review make a sched group?

We may have hit this one but that's not the one I'm testing a patch for right now. If it looks good in our setup I'll share it. It's a bit more of a fundamental scheduler change that we may or may not want to have enabled by default.

Thanks Adrian for your comments if there's anything you want me to urgently fix I can do that. Otherwise my goal was get the general and IBS tooling in people's hands.

Yes, I don't have the ncurses bits ready so I haven't bothered with SIGWINCH just yet. In this first set of diffs it print once to the current console.

In D57780#1328923, @mhorne wrote:

Hi, commenting here. From a high-level perspective this seems like a good move; this pmc(8) utility is open to improvements and seems like it never got past a beta phase.
It is good to be thoughtful about the old commands, but as you note they are broken and the tool as a whole is undocumented. So I do not think we are burdened with supporting them indefinitely.

I think my higher level comment in the previous diff still holds that you can just expose the amd functions through the pmc_classdep struct to make it platform agnostic and then remove the #ifdefs and amd specific checks throughout the code. Everyone zero's out that structure so we depend on a NULL pointer meaning that the call isn't implemented. Just have your wrapper function return EOPNOTSUPP if it's a null pointer.

Also take a look at the Intel Fixed function counters IAF that are also always present. Start/stop are nops.

Just to follow up with our call can you quickly layout the plan for integrating with:

In the existing code we have to allocate counters into specific indexes in an order to check that they meet our constraints. Either:

I'm assuming we are going to multiplex the counters based on the sample 'rate'?

For context we worked with AMD to get Instruction Based Sampling (IBS) support into HWPMC and we are making a number of ongoing improvements. I built into the somewhat abandoned pmc command a new set of tools for collecting and analyzing pmc logs. I split the this stack of patches into the smallest set I could but to do that I've left out a lot of functionality that improves substantially over pmcstat.

@imp @mhorne Can one of you please look at this and try to land it? Mitchell your first pass review I think was taken care of and I did two passes after. This is blocking me from landing the tools.

Thanks Andre for all the hard work sorry to point out one more change.

The only major issue is that we shouldn't expose bit63 as a flag that's just CAP_USER and CAP_SYSTEM. The other changes are real minor. Mitchell didn't like the structure size changing but we have to support it if we want any resemblance of backwards compatibility.

Ah I realize this is for the formulas they provide. Looks good.

I think you can abandon this one too I removed that disable and the PMC_IS_STOPPED macro is a mess going forward as the bits are moving around now.

It looks like we landed some version of this diff already? Should we close it?

Abandon this one too.

I just started looking through the backlog I think we can abandon this change now?

Might make sense to change these limits to #defines and also you might as well add this information to the man page for hwpmc.

The change looks good overall, but do we need to do anything with these fields or it's just annotations for the user?

Looks good to me, just be aware that I landed a fix that forces us to whitelist json fields. I don't see any new fields but you should check that it builds against head as of bfb2fd5f66183454cfe8771595df09c0f23c7efb

Also do you have a third party tool that tests rdpmc from the app? I recall there’s a library that did this was curious what APIs it’s currently using.

In D55607#1272461, @paulo_nlink.com.br wrote:

Thank you for the clarification. I now understand the RDPMC mapping
from AMD BKDG page 440, including the non-contiguous DF range
(ECX 6-9 and ECX 16-27).

Would you agree with promoting amd_core_npmcs and amd_l3_npmcs to
static module-level variables to implement the correct mapping in
amd_get_msr()?

I pulled out my capability change, again this is reusing the existing code used by a few ARM64 platforms.

If you want to fix your patch to the rdpmc problem we should land this and maybe wait for my other fix for the per-package change.

The WASI folks seem fine upstreaming patches. Might be nice to make clean pull requests for these projects after we resolve Brook's comment.

In D25150#554652, @mmacy wrote:

I'm a bit confused. The counter descriptions are just copied from Linux. If you're just copying from there, just add them as is. If you're doing something new then add them as a different file.

Some counters from the Linux repo appear to be undocumented at the moment and I left those out. Also they seem to think some of these are available on earlier platforms but I don't see that in the documentation nor can I test it.

• Adding missing counters for Zen+ and Zen2

Most of it is in Linux's amdzen2 files should I just merge those under a new name? Seems weird they didn't use versioning. Do you have a better plan how to deal with these? (Also is the Linux tree the correct source for these json files?)

Could you clarify if these files are synced from Linux or generated from the manuals? I wanted to add a few counters that appear in the documentation but are missing from both the Intel and FreeBSD definitions. What would be the best way should we add definitions to the existing files in the correct place or create a new file to track these additions?

In D20366#440218, @delphij wrote:

In D20366#440204, @ali_mashtizadeh.com wrote:

This version does not change the default, but allows users to change the parameters.

But can't one already use e.g. $6$rounds=100000$xxxxx$ as salt? Or are you talking about something different?

This version does not change the default, but allows users to change the parameters.

• strn -> strl

• Fix man page