From owner-svn-src-all@freebsd.org Mon Jun 4 17:27:23 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 93373FD0E5C; Mon, 4 Jun 2018 17:27:23 +0000 (UTC) (envelope-from mmacy@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 3857273E60; Mon, 4 Jun 2018 17:27:23 +0000 (UTC) (envelope-from mmacy@freebsd.org) Received: from mail-it0-f44.google.com (mail-it0-f44.google.com [209.85.214.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) (Authenticated sender: mmacy) by smtp.freebsd.org (Postfix) with ESMTPSA id EFBA215A0E; Mon, 4 Jun 2018 17:27:22 +0000 (UTC) (envelope-from mmacy@freebsd.org) Received: by mail-it0-f44.google.com with SMTP id a195-v6so11149442itd.3; Mon, 04 Jun 2018 10:27:22 -0700 (PDT) X-Gm-Message-State: APt69E3SPneTNzGFpf1z8od8yhkshRWHDzpPbIrHzHkUaSchbpV4PhNW KdDCI5hcKFXSt1zcyZiCvjXAqcmDDbg9qVabR/I= X-Google-Smtp-Source: ADUXVKLxRCist7z9YMBrnILYA8Bq5QLkevHcSz2L4e6jG0i+knWfqqk2MTEaabhGcF+x5zVYljOJJGoQONhq+x/B3To= X-Received: by 2002:a24:6c8a:: with SMTP id w132-v6mr11458507itb.4.1528133242320; Mon, 04 Jun 2018 10:27:22 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a02:8cd:0:0:0:0:0 with HTTP; Mon, 4 Jun 2018 10:27:21 -0700 (PDT) In-Reply-To: <20180604120815.GB2450@kib.kiev.ua> References: <201806040110.w541ANZr044727@repo.freebsd.org> <20180604120815.GB2450@kib.kiev.ua> From: Matthew Macy Date: Mon, 4 Jun 2018 10:27:21 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r334595 - in head: sys/dev/hwpmc sys/kern sys/sys usr.sbin/pmcstat To: Konstantin Belousov Cc: src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" 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 17:27:23 -0000 On Mon, Jun 4, 2018 at 5:08 AM, 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 ? 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. > >> +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. > 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.