From owner-svn-src-all@freebsd.org Mon Jun 4 12:08:27 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C931DFD46F3; Mon, 4 Jun 2018 12:08:27 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 43032838CB; Mon, 4 Jun 2018 12:08:27 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTP id w54C8FLR066256; Mon, 4 Jun 2018 15:08:18 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua w54C8FLR066256 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id w54C8FH3066254; Mon, 4 Jun 2018 15:08:15 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Mon, 4 Jun 2018 15:08:15 +0300 From: Konstantin Belousov To: Matt Macy Cc: 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: <20180604120815.GB2450@kib.kiev.ua> References: <201806040110.w541ANZr044727@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201806040110.w541ANZr044727@repo.freebsd.org> User-Agent: Mutt/1.10.0 (2018-05-17) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Jun 2018 12:08:28 -0000 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 ? > +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 ?