Date: Tue, 5 Jun 2018 13:21:44 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Matthew Macy <mmacy@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r334595 - in head: sys/dev/hwpmc sys/kern sys/sys usr.sbin/pmcstat Message-ID: <20180605102144.GF2450@kib.kiev.ua> In-Reply-To: <CAPrugNoxj_hm0-ncL7%2B_5WXbOQPCgjFLqkFmuen%2BNtWpuVm59Q@mail.gmail.com> References: <201806040110.w541ANZr044727@repo.freebsd.org> <20180604120815.GB2450@kib.kiev.ua> <CAPrugNoxj_hm0-ncL7%2B_5WXbOQPCgjFLqkFmuen%2BNtWpuVm59Q@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180605102144.GF2450>