From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3AE2DC433DF for ; Thu, 18 Jun 2020 10:51:19 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E553420720 for ; Thu, 18 Jun 2020 10:51:18 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="MujnHoJq" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E553420720 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=2BzDHjLOGWa7yx/MHU8mP4iHNQhg6Qf1xi3KgOlQS8M=; b=MujnHoJqxWW1GW tfOi4rB15I0TS3jtjLhoWoO2HoavhHIh99N1NafvySxqMQA2nqTxh6T5NycEnZB13MjiFxk0MT63v /Jy9SrKRwB+uUfhex/YBCFMCwJxdJpibwAT0wlY27TIGkdb2bltICbGDDqTuUcQujCJE5t69f7fF8 3Tr8M2JLlMYNzf4+Xy5K0Iqe0WWuD1gS/IbeN5B1lH5Qtk3ooz6M83fvFAEAKEoKzceguHbfc3P+P FxMEdYS1hKLUCJxnf8eSy0IlLnMItzhoc7pqClbZCKvZK04tXxsVrj++aVC8+U/s21uDKbxppPTVM wr1BZMuqdJYY+5ndZDLA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jls8U-0006Ub-Rl; Thu, 18 Jun 2020 10:51:10 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jls8A-0006DR-VE for linux-arm-kernel@lists.infradead.org; Thu, 18 Jun 2020 10:50:52 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 304F231B; Thu, 18 Jun 2020 03:50:50 -0700 (PDT) Received: from [192.168.0.110] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 606CD3F71F; Thu, 18 Jun 2020 03:50:48 -0700 (PDT) Subject: Re: [PATCH v5 3/7] arm64: perf: Remove PMU locking To: Stephen Boyd , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <20200617113851.607706-1-alexandru.elisei@arm.com> <20200617113851.607706-4-alexandru.elisei@arm.com> <159242503203.62212.1690942414916053920@swboyd.mtv.corp.google.com> From: Alexandru Elisei Message-ID: Date: Thu, 18 Jun 2020 11:51:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: <159242503203.62212.1690942414916053920@swboyd.mtv.corp.google.com> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200618_035051_092469_1B054DFB X-CRM114-Status: GOOD ( 17.26 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, Julien Thierry , Peter Zijlstra , maz@kernel.org, Jiri Olsa , Will Deacon , Arnaldo Carvalho de Melo , Alexander Shishkin , Ingo Molnar , catalin.marinas@arm.com, Namhyung Kim , will@kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, On 6/17/20 9:17 PM, Stephen Boyd wrote: > Quoting Alexandru Elisei (2020-06-17 04:38:47) >> From: Julien Thierry >> >> The PMU is disabled and enabled, and the counters are programmed from >> contexts where interrupts or preemption is disabled. >> >> The functions to toggle the PMU and to program the PMU counters access the >> registers directly and don't access data modified by the interrupt handler. >> That, and the fact that they're always called from non-preemptible >> contexts, means that we don't need to disable interrupts or use a spinlock. > Maybe we should add a lockdep assertion that the code isn't preemptible? > I.e. add a cant_sleep() call? Or is it more that we don't need locking > because we're just doing register accesses and don't need to protect > those accesses from each other? It's both. The spinlocks were there to protect the functions from being preempted and possibly migrated to another CPU, and from being interrupted by the PMU irq handler. There was no data race with the interrupt handler, but before the previous patch ("arm64: perf: Avoid PMXEV* indirection"), in order to read/write/program a counter, one had to write the counter number to a counter selection register, and then write/read the desired value from another register. This was done from both the armv8pmu_{enable,disable}_event() functions and the irq handler, and the spinlock was necessary. Now that we can access a counter using a single register access, there's no need to protect the functions from being interrupted by the IRQ handler. As for armv8pmu_{start,stop}(), they consist of one register write, so it's also safe for the irq handler to interrupt them. For the preemption part of the locking. The armv8pmu_{enable,disable}_event(), when called by the perf core code via the pmu->{start,stop,add,del} callbacks, are guaranteed to be called with IRQs and preemption disabled, as per the comment in include/linux/perf_event.h. They are also called from the arm_pmu driver by the CPU PM notifiers, which should also be executed with interrupts disabled. Should we check here that the top level code respects these guarantees? The armv8pmu_{start,stop}() functions are called from the irq handler, so we're safe from preemption in this case. They are also called via pmu->pmu_{enable,disable} callbacks, and I didn't find an explicit contract regarding preemption in include/linux/perf_event.h. I've checked the other call sites, and I didn't find any instances where they are called with preemption enabled, which makes sense as we don't want to disable the PMU on a another CPU by accident. I would be inclined to add cant_sleep() calls to armv8pmu_{start,stop}(). In the previous iteration, there were WARN_ONs in these functions, and Will said [1] they can be removed because they are per-CPU operations. Will, what do you think about adding the lockdep assertions? [1] https://www.spinics.net/lists/arm-kernel/msg745161.html Thanks, Alex _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel