Page MenuHomeFreeBSD

pmcstat: Fix usage message
ClosedPublic

Authored by 0mp on Aug 17 2020, 10:53 AM.

Details

Summary
pmcstat: Fix usage message

Diff Detail

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

Event Timeline

0mp requested review of this revision.Aug 17 2020, 10:53 AM
usr.sbin/pmcstat/pmcstat.c
377 ↗(On Diff #75884)

This sentence does not make any sense to me, but perhaps I'm just not the target audience of pmcstat. i'll be glad if someone could suggest a better wording.

I don't understand toggle in this context

I don't understand toggle in this context

Right, I've got no idea why I put it there when I wrote it.

In D26082#604606, @0mp wrote:

I don't understand toggle in this context

Right, I've got no idea why I put it there when I wrote it.

Oh, that was because the manual page uses the word "toggle".

Revert changes, only replace \n with \t

I had a look at the man page and the -U option is indeed a bit confusing.
Certainly replacing the \n is the right thing to do; I will try to have a look at the src.

This revision is now accepted and ready to land.Nov 4 2020, 3:42 PM
This revision was automatically updated to reflect the committed changes.

I had a look at the man page and the -U option is indeed a bit confusing.
Certainly replacing the \n is the right thing to do; I will try to have a look at the src.

Alright :) Thanks!

U is indeed a toggle:

case 'U':       /* toggle user-space callchain capture */
        do_userspace = !do_userspace;
        args.pa_required |= FLAG_HAS_SAMPLING_PMCS;
        break;

However it starts false and is only assigned in the U case, so -U -U will indeed turn it off again but that does not seem useful; I believe this should be changed in the src, and we should remove mention of toggle in the man page.

Continuing the trail, do_userspace is used like so:

if (do_callchain) {
        ev->ev_flags |= PMC_F_CALLCHAIN;
        if (do_userspace)
                ev->ev_flags |= PMC_F_USERCALLCHAIN;
}

That flag is

sys/sys/pmc.h
378:#define     PMC_F_USERCALLCHAIN     0x00000100 /*OP ALLOCATE use userspace stack */

And is used like so:

int
pmc_process_interrupt(int ring, struct pmc *pm, struct trapframe *tf)
{
        struct thread *td;

        td = curthread;
        if ((pm->pm_flags & PMC_F_USERCALLCHAIN) &&
            (td->td_proc->p_flag & P_KPROC) == 0 &&
            !TRAPF_USERMODE(tf)) {
                atomic_add_int(&td->td_pmcpend, 1);
                return (pmc_add_sample(PMC_UR, pm, tf));
        }
        return (pmc_add_sample(ring, pm, tf));
}

So, normally sampling PMCs capture kernel stacks in kernel mode, and user stacks in user mode. Specifying -U captures both the user and kernel stack in kernel mode.

I had a look at the man page and the -U option is indeed a bit confusing.
Certainly replacing the \n is the right thing to do; I will try to have a look at the src.

Yes, agreed.

U is indeed a toggle:
...
However it starts false and is only assigned in the U case, so -U -U will indeed turn it off again but that does not seem useful; I believe this should be changed in the src, and we should remove mention of toggle in the man page.

That's fine with me.

So, normally sampling PMCs capture kernel stacks in kernel mode, and user stacks in user mode. Specifying -U captures both the user and kernel stack in kernel mode.

That's correct. There are cases where it is useful to know which user-space code paths are causing work in the kernel. Imagine a case where a user-space application uses the same syscall with different arguments, and the arguments take variable amounts of time to process. It could be useful to have the full context about which user-space stack is causing work in the kernel.