From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031850AbbKEKOb (ORCPT ); Thu, 5 Nov 2015 05:14:31 -0500 Received: from eu-smtp-delivery-143.mimecast.com ([207.82.80.143]:30310 "EHLO eu-smtp-delivery-143.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030570AbbKEKO3 convert rfc822-to-8bit (ORCPT ); Thu, 5 Nov 2015 05:14:29 -0500 Subject: Re: [PATCHv2 3/4] arm-cci: Add routines to enable/disable all counters To: Mark Rutland References: <1445346326-30820-1-git-send-email-suzuki.poulose@arm.com> <1445346326-30820-4-git-send-email-suzuki.poulose@arm.com> <20151104182854.GH23860@leverpostej> Cc: linux-arm-kernel@lists.infradead.org, punit.agrawal@arm.com, arm@kernel.org, linux-kernel@vger.kernel.org From: "Suzuki K. Poulose" Message-ID: <563B2C01.80701@arm.com> Date: Thu, 5 Nov 2015 10:14:25 +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: <20151104182854.GH23860@leverpostej> X-OriginalArrivalTime: 05 Nov 2015 10:14:26.0093 (UTC) FILETIME=[BFDBDDD0:01D117B2] X-MC-Unique: JRsSXKXMSXOwvcz44QDVNA-1 Content-Type: text/plain; charset=WINDOWS-1252; format=flowed Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/11/15 18:28, Mark Rutland wrote: > On Tue, Oct 20, 2015 at 02:05:25PM +0100, Suzuki K. Poulose wrote: >> Adds helper routines to manipulate the counter controls for >> all the counters on the CCI PMU. >> >> pmu_disable_counters_ctrl: Iterates over the counters, >> checking the status of each counter and disabling any enabled >> counters. For each such changed counter, the mask is updated so that >> one can restore the state later using pmu_enable_counters_ctrl. >> >> /* >> + * Restore the status of the counters. >> + * For each counter set in the mask, enable the counter back. >> + */ >> +static void pmu_restore_counters_ctrl(struct cci_pmu *cci_pmu, unsigned long *mask) >> +{ >> + int i; >> + >> + for_each_set_bit(i, mask, cci_pmu->num_cntrs) >> + pmu_enable_counter(cci_pmu, i); >> +} >> + >> +/* >> + * For all counters on the CCI-PMU, disable any 'enabled' counters, >> + * saving the changed counters in the mask, so that we can restore >> + * it later using pmu_restore_counters_ctrl. >> + */ >> +static void pmu_disable_counters_ctrl(struct cci_pmu *cci_pmu, unsigned long *mask) >> +{ >> + int i; >> + >> + for (i = 0; i < cci_pmu->num_cntrs; i++) { >> + clear_bit(i, mask); >> + if (pmu_get_counter_ctrl(cci_pmu, i)) { >> + set_bit(i, mask); >> + pmu_disable_counter(cci_pmu, i); >> + } >> + } >> +} > > I don't understand what's going on with the mask here. Why do we clear > ieach bit when the only user (introduced in the next patch) explicitly > clears the mask anyway? To be more precise, it should have been : if (pmu_get_counter_ctrl(cci_pmu, i)) { set_bit(i, mask); pmu_disable_counter(cci_pmu, i); } else clear_bit(i, mask); > > Can we not get rid of the mask entirely? The combination of used_mask > and each event's hwc->state tells us which counters are actually in use. The problem is that neither hwc->state nor the cci_pmu->hw_events->events is protected by pmu_lock, while enable/disable counter is. So we cannot really rely on ((struct perf_event *)(cci_pmu->hw_events->events[counter]))->hw->state. What we do above is, create a mask of the counters which are enabled at the moment and disable all of them. We then program the counter and then re-enable those which were enabled (as marked in the mask). Suzuki > > Thanks, > Mark. >