From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Rutland Subject: Re: [PATCH V5 2/4] drivers/perf: imx_ddr: Add ddr performance counter support Date: Thu, 25 Apr 2019 18:16:59 +0100 Message-ID: <20190425171659.GB4319@lakrids.cambridge.arm.com> References: <1555447156-28306-1-git-send-email-Frank.Li@nxp.com> <1555447156-28306-2-git-send-email-Frank.Li@nxp.com> <20190424105822.GA14614@fuggles.cambridge.arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20190424105822.GA14614@fuggles.cambridge.arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Will Deacon Cc: Aisheng Dong , "devicetree@vger.kernel.org" , "festevam@gmail.com" , "s.hauer@pengutronix.de" , Frank Li , "robh+dt@kernel.org" , dl-linux-imx , "kernel@pengutronix.de" , "lznuaa@gmail.com" , "shawnguo@kernel.org" , "linux-arm-kernel@lists.infradead.org" List-Id: devicetree@vger.kernel.org On Wed, Apr 24, 2019 at 11:58:22AM +0100, Will Deacon wrote: > Hi Frank, > > On Tue, Apr 16, 2019 at 08:39:33PM +0000, Frank Li wrote: > > Add ddr performance monitor support for iMX8QXP > > > > There are 4 counters for ddr perfomance events. > > counter 0 is dedicated for cycles. > > you choose any up to 3 no cycles events. > > I was about to pick this up, but I still have a few questions/comments > on the code: > > > +static irqreturn_t ddr_perf_irq_handler(int irq, void *p) > > +{ > > + int i; > > + u8 reg; > > + int val; > > + int counter; > > + struct ddr_pmu *pmu = (struct ddr_pmu *) p; > > + struct perf_event *event; > > + > > + /* Only cycles counter overflowed can issue irq. all counters will > > + * be stopped when cycles counter overflow. but other counter don't stop > > + * when overflow happen. Update all of the local counter values, > > + * then reset the cycles counter, so the others can continue > > + * counting. cycles counter is fastest counter in all events. at last > > + * 4 times than other counters. > > + */ > > + for (i = 0; i < NUM_COUNTER; i++) { > > + > > + if (!pmu->active_events[i]) > > + continue; > > + > > + event = pmu->active_events[i]; > > + counter = event->hw.idx; > > + reg = counter * 4 + COUNTER_CNTL; > > + val = readl(pmu->base + reg); > > Does this read have a side-effect, or can it be removed given that you don't > use val for anything else? > > > + ddr_perf_event_update(event); > > + > > + if (counter == EVENT_CYCLES_COUNTER) { > > + ddr_perf_event_enable(pmu, > > + EVENT_CYCLES_ID, > > + EVENT_CYCLES_COUNTER, > > + true); > > + ddr_perf_event_update(event); > > + } > > + } > > + > > + return IRQ_HANDLED; > > +} > > What stops the IRQ handler running concurrently with perf callbacks? I can't > see any locking here, or is the IRQ supposed to be affine to the same CPU > that's handling the perf context? To be correct, the IRQ affinity should be set to match the logical affinity of the PMU, but it looks like that's not done at setup or migrate time. Frank, please take a look at what drivers/perf/arm-ccn.c does with teh IRQ, including the use of IRQF_NOBALANCING | IRQF_NO_THREAD flags when requesting the IRQ to begin with. Thanks, Mark. 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.5 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham 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 87AEEC43219 for ; Thu, 25 Apr 2019 17:17:14 +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 0E8DB2084F for ; Thu, 25 Apr 2019 17:17:15 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="rJLb+v/I" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0E8DB2084F 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:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=7hKVja4wZ5tKB9rHx1U8mXmrtHVIqVPZ+X/ilzg9PMk=; b=rJLb+v/ICMcyUn Sgp2KcL2JpCVEelQiGtoV9S81I1zxKLjr7yHS5oUP4Kye83qgaID1zU8qiaybx8ZHyFlk0dERKTK4 0VzjRvQRnk+pVpuJYUx7gvA/5sjAUxrHHUlOxUmCvvAu6aAH61UgwHTco/Mm5MyRyUuqGPJWTElm3 qSTb54LKZblt/67vy0T7QAMRjVXcnfQwF1CMIsNvaoUJ+16h1I1ptDDAW09hvanN6s6gLs3aCtPog +BpYC7eulsu/G544c0sUJnP0RA4coqNJGkvUSWd2swKN1rpxg3piybfI8cAoI1z1h/7nzc9sn5bBu 0brL9TtsU1x6XvxvJZvg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hJhzf-0003FU-RW; Thu, 25 Apr 2019 17:17:07 +0000 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70] helo=foss.arm.com) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hJhzc-0003F1-Sm for linux-arm-kernel@lists.infradead.org; Thu, 25 Apr 2019 17:17:06 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5096080D; Thu, 25 Apr 2019 10:17:04 -0700 (PDT) Received: from lakrids.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2674C3F246; Thu, 25 Apr 2019 10:17:02 -0700 (PDT) Date: Thu, 25 Apr 2019 18:16:59 +0100 From: Mark Rutland To: Will Deacon Subject: Re: [PATCH V5 2/4] drivers/perf: imx_ddr: Add ddr performance counter support Message-ID: <20190425171659.GB4319@lakrids.cambridge.arm.com> References: <1555447156-28306-1-git-send-email-Frank.Li@nxp.com> <1555447156-28306-2-git-send-email-Frank.Li@nxp.com> <20190424105822.GA14614@fuggles.cambridge.arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190424105822.GA14614@fuggles.cambridge.arm.com> User-Agent: Mutt/1.11.1+11 (2f07cb52) (2018-12-01) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190425_101704_937987_E9985B9A X-CRM114-Status: GOOD ( 24.60 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Aisheng Dong , "devicetree@vger.kernel.org" , "festevam@gmail.com" , "s.hauer@pengutronix.de" , Frank Li , "robh+dt@kernel.org" , dl-linux-imx , "kernel@pengutronix.de" , "lznuaa@gmail.com" , "shawnguo@kernel.org" , "linux-arm-kernel@lists.infradead.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 On Wed, Apr 24, 2019 at 11:58:22AM +0100, Will Deacon wrote: > Hi Frank, > > On Tue, Apr 16, 2019 at 08:39:33PM +0000, Frank Li wrote: > > Add ddr performance monitor support for iMX8QXP > > > > There are 4 counters for ddr perfomance events. > > counter 0 is dedicated for cycles. > > you choose any up to 3 no cycles events. > > I was about to pick this up, but I still have a few questions/comments > on the code: > > > +static irqreturn_t ddr_perf_irq_handler(int irq, void *p) > > +{ > > + int i; > > + u8 reg; > > + int val; > > + int counter; > > + struct ddr_pmu *pmu = (struct ddr_pmu *) p; > > + struct perf_event *event; > > + > > + /* Only cycles counter overflowed can issue irq. all counters will > > + * be stopped when cycles counter overflow. but other counter don't stop > > + * when overflow happen. Update all of the local counter values, > > + * then reset the cycles counter, so the others can continue > > + * counting. cycles counter is fastest counter in all events. at last > > + * 4 times than other counters. > > + */ > > + for (i = 0; i < NUM_COUNTER; i++) { > > + > > + if (!pmu->active_events[i]) > > + continue; > > + > > + event = pmu->active_events[i]; > > + counter = event->hw.idx; > > + reg = counter * 4 + COUNTER_CNTL; > > + val = readl(pmu->base + reg); > > Does this read have a side-effect, or can it be removed given that you don't > use val for anything else? > > > + ddr_perf_event_update(event); > > + > > + if (counter == EVENT_CYCLES_COUNTER) { > > + ddr_perf_event_enable(pmu, > > + EVENT_CYCLES_ID, > > + EVENT_CYCLES_COUNTER, > > + true); > > + ddr_perf_event_update(event); > > + } > > + } > > + > > + return IRQ_HANDLED; > > +} > > What stops the IRQ handler running concurrently with perf callbacks? I can't > see any locking here, or is the IRQ supposed to be affine to the same CPU > that's handling the perf context? To be correct, the IRQ affinity should be set to match the logical affinity of the PMU, but it looks like that's not done at setup or migrate time. Frank, please take a look at what drivers/perf/arm-ccn.c does with teh IRQ, including the use of IRQF_NOBALANCING | IRQF_NO_THREAD flags when requesting the IRQ to begin with. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel