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

[Konstantin Belousov]

On Mon, Jun 04, 2018 at 03:08:15PM +0300, Konstantin Belousov 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 ?

And more, it is safe to access userspace from userret() so you can
walk usermode stack in the pmc callback directly, without scheduling
an ast.

> 
> > +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.
> 
> 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.
> 
> > @@ -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 ?
>