svn commit: r334595 - in head: sys/dev/hwpmc sys/kern sys/sys usr.sbin/pmcstat

[Konstantin Belousov]

On Mon, Jun 04, 2018 at 10:27:21AM -0700, Matthew Macy wrote:
> On Mon, Jun 4, 2018 at 5:08 AM, Konstantin Belousov <kostikbel at gmail.com> wrote:
> > On Mon, Jun 04, 2018 at 01:10:23AM +0000, Matt Macy wrote:
> >> @@ -2214,6 +2236,11 @@ pmc_hook_handler(struct thread *td, int function, void
> >>
> >>               pmc_capture_user_callchain(PCPU_GET(cpuid), PMC_HR,
> >>                   (struct trapframe *) arg);
> >> +
> >> +             KASSERT(td->td_pinned == 1,
> >> +                     ("[pmc,%d] invalid td_pinned value", __LINE__));
> >> +             sched_unpin();  /* Can migrate safely now. */
> > sched_pin() is called from pmc_post_callchain_callback(), which is
> > called from userret(). userret() is executed with interrupts and
> > preemption enabled, so there is a non-trivial chance that the thread
> > already migrated.
> >
> > In fact, I do not see a need to disable migration for the thread if user
> > callchain is planned to be gathered. You only need to remember the cpu
> > where the interrupt occured, to match it against the request.  Or are
> > per-cpu PMC registers still accessed during callchain collection ?
> 
> The buffers are pcpu. Although it would in principle be safe in this
> case since I
> don't modify the read/write indices. However, I'd have to add another field for
> the CPU and it doesn't handle the case of multiple migrations.
> 
You already moved the collection to userret(), thanks. So the only
reason to sched_pin() in pmc_process_thread_userret() is to make
pmc_capture_user_callchain() to operate on the stable cpu ?

May be, add a comment there, and move the assert that td_pinned > 0, into
pmc_capture_user_callchain() ?

> >
> >> +int
> >> +pmc_process_interrupt(int cpu, int ring, struct pmc *pm, struct trapframe *tf,
> >> +    int inuserspace)
> >> +{
> >> +     struct thread *td;
> >> +
> >> +     td = curthread;
> >> +     if ((pm->pm_flags & PMC_F_USERCALLCHAIN) &&
> >> +             td && td->td_proc &&
> >> +             (td->td_proc->p_flag & P_KPROC) == 0 &&
> >> +             !inuserspace) {
> > I am curious why a lot of the pmc code checks for curthread != NULL and,
> > like this fragment, for curproc != NULL.  I am sure that at least on x86,
> > we never let curthread point to the garbage, even during the context
> > switches.  NMI handler has the same cargo-cult check, BTW.
> 
> I didn't think they could be NULL, but have been cargo culting the
> existing code.
You already cleaned this, thanks.

> 
> > Also, please fix the indentation of the conditions block.
> 
> 
> >
> >> +             atomic_add_int(&curthread->td_pmcpend, 1);
> > You can use atomic_store_int() there, I believe,  Then there would be
> > no locked op executed at all, on x86.
> 
> Storing a 1 would enable me to early terminate the loop.
> 
> >
> >> @@ -375,6 +375,7 @@ struct thread {
> >>       void            *td_lkpi_task;  /* LinuxKPI task struct pointer */
> >>       TAILQ_ENTRY(thread) td_epochq;  /* (t) Epoch queue. */
> >>       epoch_section_t td_epoch_section; /* (t) epoch section object */
> >> +     int             td_pmcpend;
> > Why this member was not put into the zeroed region ?  Wouldn't a garbage
> > there cause uneccessary ASTs ?
> 
> It would cause _1_ unnecessary check for callchains after initial
> creation. Putting it in the zero area would break the ABI.
We do not care about KBI stability on HEAD.  If you care about it more than
usual, you can bump __FreeBSD_version to prevent older modules from load,
when struct thread layout changed.

Practically we change KBI as needed without special measures.