From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752056AbcAEJ7S (ORCPT ); Tue, 5 Jan 2016 04:59:18 -0500 Received: from foss.arm.com ([217.140.101.70]:57187 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751188AbcAEJ7Q (ORCPT ); Tue, 5 Jan 2016 04:59:16 -0500 Subject: Re: [PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable To: Mark Rutland References: <1451908490-2615-1-git-send-email-suzuki.poulose@arm.com> <1451908490-2615-6-git-send-email-suzuki.poulose@arm.com> <20160104192401.GD17127@leverpostej> Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, arm@kernel.org, punit.agrawal@arm.com, peterz@infradead.org From: "Suzuki K. Poulose" Message-ID: <568B93F1.8050202@arm.com> Date: Tue, 5 Jan 2016 09:59:13 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20160104192401.GD17127@leverpostej> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/01/16 19:24, Mark Rutland wrote: > On Mon, Jan 04, 2016 at 11:54:44AM +0000, Suzuki K. Poulose wrote: >> Delay setting the event periods for enabled events to pmu::pmu_enable(). >> We mark the event.hw->state PERF_HES_ARCH for the events that we know >> have their counts recorded and have been started. > > Please add a comment to the code stating exactly what PERF_HES_ARCH > means for the CCI PMU driver, so it's easy to find. > Sure. >> +void cci_pmu_update_counters(struct cci_pmu *cci_pmu) >> +{ >> + int i; >> + unsigned long mask[BITS_TO_LONGS(cci_pmu->num_cntrs)]; > > I think this can be: > > DECLARE_BITMAP(mask, cci_pmu->num_cntrs); > >> + >> + memset(mask, 0, BITS_TO_LONGS(cci_pmu->num_cntrs) * sizeof(unsigned long)); > > Likewise: > > bitmap_zero(mask, cci_pmu->num_cntrs); OK >> + if (!cci_pmu->hw_events.events[i]) { >> + WARN_ON(1); >> + continue; >> + } >> + > > if (WARN_ON(!cci_pmu->hw_events.events[i])) > continue; OK >> @@ -980,8 +1015,11 @@ static void cci_pmu_start(struct perf_event *event, int pmu_flags) >> /* Configure the counter unless you are counting a fixed event */ >> if (!pmu_fixed_hw_idx(cci_pmu, idx)) >> pmu_set_event(cci_pmu, idx, hwc->config_base); >> - >> - pmu_event_set_period(event); >> + /* >> + * Mark this counter, so that we can program the >> + * counter with the event_period. see cci_pmu_enable() >> + */ >> + hwc->state = PERF_HES_ARCH; > > Why couldn't we have kept pmu_event_set_period here, and have that set > prev_count and PERF_HES_ARCH? > > Then we'd be able to do the same betching for overflow too. The pmu is not disabled while we are in overflow irq handler. Hence there may not be a pmu_enable() which would set the period for the counter which overflowed, if defer the write in that case. Is that assumption wrong ? Cheers Suzuki