Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1300675 > unrolled thread

[PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable "Suzuki K. Poulose" <suzuki.poulose@arm.com> - 2016-01-04 13:00 +0100
    Re: [PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable Mark Rutland <mark.rutland@arm.com> - 2016-01-04 20:30 +0100
      Re: [PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> - 2016-01-05 11:00 +0100
        Re: [PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable Mark Rutland <mark.rutland@arm.com> - 2016-01-11 11:50 +0100
          Re: [PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> - 2016-01-11 12:10 +0100
            Re: [PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable Mark Rutland <mark.rutland@arm.com> - 2016-01-11 12:30 +0100
              Re: [PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> - 2016-01-11 19:20 +0100

#1300675 — [PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable

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. Since we reprogram the
counters every time before count, we can set the counters for all the
event counters which are !STOPPED && ARCH.

Grouping the writes to counters can ammortise the cost of the operation
on PMUs where it is expensive (e.g, CCI-500).

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 drivers/bus/arm-cci.c |   42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 0189f3a..c768ee4 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -916,6 +916,40 @@ static void hw_perf_event_destroy(struct perf_event *event)
 	}
 }
 
+/*
+ * Program the CCI PMU counters which have PERF_HES_ARCH set
+ * with the event period and mark them ready before we enable
+ * PMU.
+ */
+void cci_pmu_update_counters(struct cci_pmu *cci_pmu)
+{
+	int i;
+	unsigned long mask[BITS_TO_LONGS(cci_pmu->num_cntrs)];
+
+	memset(mask, 0, BITS_TO_LONGS(cci_pmu->num_cntrs) * sizeof(unsigned long));
+
+	for_each_set_bit(i, cci_pmu->hw_events.used_mask, cci_pmu->num_cntrs) {
+		struct hw_perf_event *hwe;
+
+		if (!cci_pmu->hw_events.events[i]) {
+			WARN_ON(1);
+			continue;
+		}
+
+		hwe = &cci_pmu->hw_events.events[i]->hw;
+		/* Leave the events which are not counting */
+		if (hwe->state & PERF_HES_STOPPED)
+			continue;
+		if (hwe->state & PERF_HES_ARCH) {
+			set_bit(i, mask);
+			hwe->state &= ~PERF_HES_ARCH;
+			local64_set(&hwe->prev_count, CCI_CNTR_PERIOD);
+		}
+	}
+
+	pmu_write_counters(cci_pmu, mask, CCI_CNTR_PERIOD);
+}
+
 static void cci_pmu_enable(struct pmu *pmu)
 {
 	struct cci_pmu *cci_pmu = to_cci_pmu(pmu);
@@ -927,6 +961,7 @@ static void cci_pmu_enable(struct pmu *pmu)
 		return;
 
 	raw_spin_lock_irqsave(&hw_events->pmu_lock, flags);
+	cci_pmu_update_counters(cci_pmu);
 	__cci_pmu_enable();
 	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
 
@@ -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;
 	pmu_enable_counter(cci_pmu, idx);
 
 	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1300988

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.

> Since we reprogram the counters every time before count, we can set
> the counters for all the event counters which are !STOPPED && ARCH.
> 
> Grouping the writes to counters can ammortise the cost of the operation
> on PMUs where it is expensive (e.g, CCI-500).
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/bus/arm-cci.c |   42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 0189f3a..c768ee4 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -916,6 +916,40 @@ static void hw_perf_event_destroy(struct perf_event *event)
>  	}
>  }
>  
> +/*
> + * Program the CCI PMU counters which have PERF_HES_ARCH set
> + * with the event period and mark them ready before we enable
> + * PMU.
> + */
> +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);

> +
> +	for_each_set_bit(i, cci_pmu->hw_events.used_mask, cci_pmu->num_cntrs) {
> +		struct hw_perf_event *hwe;
> +
> +		if (!cci_pmu->hw_events.events[i]) {
> +			WARN_ON(1);
> +			continue;
> +		}
> +

		if (WARN_ON(!cci_pmu->hw_events.events[i]))
			continue;

> +		hwe = &cci_pmu->hw_events.events[i]->hw;
> +		/* Leave the events which are not counting */
> +		if (hwe->state & PERF_HES_STOPPED)
> +			continue;
> +		if (hwe->state & PERF_HES_ARCH) {
> +			set_bit(i, mask);
> +			hwe->state &= ~PERF_HES_ARCH;
> +			local64_set(&hwe->prev_count, CCI_CNTR_PERIOD);
> +		}
> +	}
> +
> +	pmu_write_counters(cci_pmu, mask, CCI_CNTR_PERIOD);
> +}
> +
>  static void cci_pmu_enable(struct pmu *pmu)
>  {
>  	struct cci_pmu *cci_pmu = to_cci_pmu(pmu);
> @@ -927,6 +961,7 @@ static void cci_pmu_enable(struct pmu *pmu)
>  		return;
>  
>  	raw_spin_lock_irqsave(&hw_events->pmu_lock, flags);
> +	cci_pmu_update_counters(cci_pmu);
>  	__cci_pmu_enable();
>  	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
>  
> @@ -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.

What am I missing?

Mark.

>  	pmu_enable_counter(cci_pmu, idx);
>  
>  	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
> -- 
> 1.7.9.5
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301384

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1306050

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.

[toc] | [prev] | [next] | [standalone]

#1306082

On 11/01/16 10:46, Mark Rutland wrote:
> 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:
>> 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?

None of the PMU drivers do that AFAIK. Hence, didn't want to change it for
CCI. We could use the batching logic, if decide to do so. I can go ahead
with that if there are no other side effects with that.

Suzuki

[toc] | [prev] | [next] | [standalone]

#1306097

On Mon, Jan 11, 2016 at 11:08:27AM +0000, Suzuki K. Poulose wrote:
> On 11/01/16 10:46, Mark Rutland wrote:
> >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:
> >>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?
> 
> None of the PMU drivers do that AFAIK.

I see.

The Intel PMU driver disables the PMU for the interrupt handler; see
intel_pmu_handle_irq in arch/x86/kernel/cpu/perf_event_intel.c. It looks
like that's a special-case for sampling.

I guess we may have the only case where it makes sense to batch counter
writes as opposed to batching configuration writes.

> Hence, didn't want to change it for CCI. We could use the batching
> logic, if decide to do so. I can go ahead with that if there are no
> other side effects with that.

We'll lose events regardless as our RMW sequence will race against the
counters. Batching will make that window slightly larger, but other than
that I don't see a problem.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1306584

On 11/01/16 11:24, Mark Rutland wrote:
> On Mon, Jan 11, 2016 at 11:08:27AM +0000, Suzuki K. Poulose wrote:
>> On 11/01/16 10:46, Mark Rutland wrote:
>>> On Tue, Jan 05, 2016 at 09:59:13AM +0000, Suzuki K. Poulose wrote:
>>>> On 04/01/16 19:24, Mark Rutland wrote:
>> Hence, didn't want to change it for CCI. We could use the batching
>> logic, if decide to do so. I can go ahead with that if there are no
>> other side effects with that.
>
> We'll lose events regardless as our RMW sequence will race against the
> counters. Batching will make that window slightly larger, but other than
> that I don't see a problem.

OK, will do that.

Cheers
Suzuki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web