linux-kernel - Re: [PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160111104609.GE6499@leverpostej>
Date:	Mon, 11 Jan 2016 10:46:10 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	"Suzuki K. Poulose" <Suzuki.Poulose@....com>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	arm@...nel.org, punit.agrawal@....com, peterz@...radead.org
Subject: Re: [PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable

On Tue, Jan 05, 2016 at 09:59:13AM +0000, Suzuki K. Poulose wrote:
> 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 ?

As the driver stands today, yes.

However, wouldn't it make more sense to disable the PMU for the overflow
handler, such that we can reuse the batching logic?

Thanks,
Mark.