pmcstat: implement showing offsets into symbols in top mode

[mjg]

Summary

The -I option (and hotkey) is reused for this. Skipping symbol resolution is moved to the new -A option (and hotkey).

This is handy for looking at (false) sharing across the interconnect, for instance you can cpuset 2 will-it-scale microbenchmarks to separate sockets and:

pmcstat -n 10000 -S mem_load_l3_miss_retired.remote_hitm -IT

%SAMP IMAGE FUNCTION CALLERS

13.0 kernel     lockmgr_lock_fast_path+0x79    VOP_LOCK1_APV
10.7 kernel     __rw_rlock_int+0x35            cache_lookup
9.0 kernel     vfs_busy+0x1f3                 lookup
6.5 kernel     _vhold+0x2ba                   _vget_prep
6.0 kernel     _vhold+0x6b                    _vget_prep
6.0 kernel     _vget_prep+0x4a                cache_lookup:4.7 vget:1.3
4.9 zfs.ko     zfs_acl_next_ace+0x3c          zfs_zaccess_aces_check
4.6 kernel     vputx+0x110                    namei:3.0 lookup:1.6
4.2 kernel     lockmgr_unlock_fast_path+0x4d  VOP_UNLOCK_APV
3.8 zfs.ko     zfs_acl_next_ace+0x106         zfs_zaccess_aces_check
3.6 kernel     vrefact+0x1a                   namei:2.0 lookup:1.6
3.2 zfs.ko     rrw_exit+0x39                  zfs_freebsd_access
2.4 kernel     vfs_busy+0x23f                 lookup
1.9 zfs.ko     zfs_zaccess+0x1b               zfs_freebsd_access
1.4 kernel     tmpfs_alloc_vp+0x138           tmpfs_root
1.4 zfs.ko     zfs_acl_next_ace+0xf9          zfs_zaccess_aces_check
1.2 kernel     _vget_prep+0x6d                cache_lookup
0.9 kernel     lock_delay+0x42                __mtx_lock_sleep
0.8 kernel     lockmgr_lock_fast_path+0x9f    VOP_LOCK1_APV
0.7 kernel     lockmgr_lock_fast_path+0x80    VOP_LOCK1_APV
0.7 kernel     tmpfs_alloc_vp+0x4c            tmpfs_root
0.6 kernel     __mtx_lock_sleep+0x10c
0.5 kernel     vrefact+0                      lookup

manpage date will be updated in the commit

[bcr]

Minor nit with the manpage. Check with textproc/igor and "mandoc -Tlint" to make sure they don't spot anything I missed.
Thanks for the change, would be good to have that functionality in pmcstat.

usr.sbin/pmcstat/pmcstat.8

230

You need a line break after a sentence stop.

[markj]

Why did you change the meaning of -I instead of putting the new functionality under a new option?

[mjg]

I think it fits better. 'A' for address, 'I' for instruction. If there is a time to change it, it is now. I would not do it if pmcstat was actually in widespread use and people had the opt ingrained in their fingers.

[markj]

In D21658#472440, @mjg wrote:

I think it fits better. 'A' for address, 'I' for instruction. If there is a time to change it, it is now. I would not do it if pmcstat was actually in widespread use and people had the opt ingrained in their fingers.

I don't agree with the claim about widespread use. It's used by pretty much every large FreeBSD user I've interacted with, and as a general rule changing options in this way will cause pain. pmcstat already uses pretty much every letter already, and most of the letters don't make much sense anyway.

usr.sbin/pmcstat/pmcstat.8

228

"toggle symbol resolution"

[mjg]

The option was added at my request and highly doubt anyone else is using it (including the big user). I highly doubt any user will get upset over this change, but worst case I can drop the swap.

Ignoring the letters for the moment do you have other comments?

[markj]

In D21658#472446, @mjg wrote:

The option was added at my request and highly doubt anyone else is using it (including the big user). I highly doubt any user will get upset over this change, but worst case I can drop the swap.

Ok, with that context it seems more reasonable. I don't object to the swap, but again, pmcstat's UI is a mess and I don't see the point in breaking compatibility to make it slightly nicer.

Ignoring the letters for the moment do you have other comments?

I left a couple inline.

usr.sbin/pmcstat/pmcpl_callgraph.c

514

Why not just

/* Format name. FLAG_SKIP_TOP_FN_RES takes precedence over FLAG_SHOW_OFFSET. */
sym = ...;
if (sym == NULL || (args.pa_flags & FLAG_SKIP_TOP_FN_RES) != 0) {
       snprintf(ns, sizeof(ns), "%p", (void *)(cg->pcg_image->pi_vaddr + cg->pcg_func));
} else if ((args.pa_flags & FLAG_SHOW_OFFSET) != 0) {
    ...
} else {
    ...
}

[mjg]

I think pmcstat needs serious help and the compatibility change (which I highly doubt hurts anyone) is a step towards that goal. I want a usable tool without the need to rewrite current one, if it can be helped. Should someone want to write something new from scratch I'm happy to see it, but I don't want to the work myself.

usr.sbin/pmcstat/pmcpl_callgraph.c

514

This mixes handling the error case with regular one, if you insist I can change it.

[markj]

In D21658#472473, @mjg wrote:

I think pmcstat needs serious help and the compatibility change (which I highly doubt hurts anyone) is a step towards that goal. I want a usable tool without the need to rewrite current one, if it can be helped. Should someone want to write something new from scratch I'm happy to see it, but I don't want to the work myself.

We try to provide backwards compatibility, and that means that your goal is not really attainable in general. (And isn't it part of why pmc(1) was added?) If the modified option is present only in 12.0, then yes I agree that it probably won't affect many people. But from my perspective it basically amounts to putting lipstick on a pig. If you are going to MFC the change, so that 12.1 and 13.0 are compatible, then I don't strongly object.

usr.sbin/pmcstat/pmcpl_callgraph.c

490

This assignment is redundant.

514

I don't insist. I just find it easier to read.

[mjg]

• fix manpage
• drop redundant sym assignment from the original

[mjg]

I already noted pmcstat needs serious help. I think adding this with (the optional key swap) is solves the immediate problem of not having the feature while not having to spend a significant amount of time writing a new tool from scratch.

[markj]

In D21658#472502, @mjg wrote:

I already noted pmcstat needs serious help. I think adding this with (the optional key swap) is solves the immediate problem of not having the feature while not having to spend a significant amount of time writing a new tool from scratch.

Sure, I'm not arguing with any of this, just the gratuitous rename.

[lwhsu]

usr.sbin/pmcstat/pmcstat.8

28

don't forget to update the date. :-)