Page MenuHomeFreeBSD

pmcstat: implement showing offsets into symbols in top mode
Needs ReviewPublic

Authored by mjg on Sep 14 2019, 11:57 AM.

Details

Reviewers
mmacy
markj
Group Reviewers
manpages
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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 26494

Event Timeline

mjg created this revision.Sep 14 2019, 11:57 AM
mjg edited the summary of this revision. (Show Details)Sep 14 2019, 11:58 AM
mjg edited the summary of this revision. (Show Details)
mjg edited the summary of this revision. (Show Details)
bcr added a subscriber: bcr.Sep 15 2019, 10:26 AM

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 added a comment.Sep 15 2019, 4:54 PM

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

mjg added a comment.Sep 15 2019, 5:02 PM

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 added a comment.Sep 15 2019, 5:29 PM
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 added a comment.EditedSep 15 2019, 5:32 PM

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 added a comment.Sep 15 2019, 5:46 PM
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 added a comment.Sep 15 2019, 6:11 PM

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 added a comment.Sep 15 2019, 6:53 PM
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 updated this revision to Diff 62133.Sep 15 2019, 8:25 PM
  • fix manpage
  • drop redundant sym assignment from the original
mjg added a comment.Sep 15 2019, 8:26 PM

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 added a comment.Sep 16 2019, 2:57 AM
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 added a subscriber: lwhsu.Sep 16 2019, 11:02 PM
lwhsu added inline comments.
usr.sbin/pmcstat/pmcstat.8
28

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