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

Groups > linux.kernel > #1300664 > unrolled thread

[PATCH v5 03/11] arm-cci: Group writes to counter

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 03/11] arm-cci: Group writes to counter "Suzuki K. Poulose" <suzuki.poulose@arm.com> - 2016-01-04 13:00 +0100
    Re: [PATCH v5 03/11] arm-cci: Group writes to counter Mark Rutland <mark.rutland@arm.com> - 2016-01-04 20:10 +0100
      Re: [PATCH v5 03/11] arm-cci: Group writes to counter "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> - 2016-01-05 12:00 +0100
        Re: [PATCH v5 03/11] arm-cci: Group writes to counter Mark Rutland <mark.rutland@arm.com> - 2016-01-11 11:50 +0100
          Re: [PATCH v5 03/11] arm-cci: Group writes to counter "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> - 2016-01-11 11:50 +0100

#1300664 — [PATCH v5 03/11] arm-cci: Group writes to counter

Add a helper to group the writes to PMU counter, this will be
used to delay setting the event period to pmu::pmu_enable()

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Punit Agrawal <punit.agrawal@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 drivers/bus/arm-cci.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index ce0d3ef..f6b8717 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -785,6 +785,21 @@ static void pmu_write_counter(struct perf_event *event, u32 value)
 	__pmu_write_counter(cci_pmu, value, idx);
 }
 
+/* Write a value to a given set of counters */
+static void __pmu_write_counters(struct cci_pmu *cci_pmu, unsigned long *mask, u32 value)
+{
+	int i;
+
+	for_each_set_bit(i, mask, cci_pmu->num_cntrs)
+		__pmu_write_counter(cci_pmu, value, i);
+}
+
+static void __maybe_unused
+pmu_write_counters(struct cci_pmu *cci_pmu, unsigned long *mask, u32 value)
+{
+	__pmu_write_counters(cci_pmu, mask, value);
+}
+
 static u64 pmu_event_update(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
-- 
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]

#1300974

On Mon, Jan 04, 2016 at 11:54:42AM +0000, Suzuki K. Poulose wrote:
> Add a helper to group the writes to PMU counter, this will be
> used to delay setting the event period to pmu::pmu_enable()
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Punit Agrawal <punit.agrawal@arm.com>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/bus/arm-cci.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index ce0d3ef..f6b8717 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -785,6 +785,21 @@ static void pmu_write_counter(struct perf_event *event, u32 value)
>  	__pmu_write_counter(cci_pmu, value, idx);
>  }
>  
> +/* Write a value to a given set of counters */
> +static void __pmu_write_counters(struct cci_pmu *cci_pmu, unsigned long *mask, u32 value)
> +{
> +	int i;
> +
> +	for_each_set_bit(i, mask, cci_pmu->num_cntrs)
> +		__pmu_write_counter(cci_pmu, value, i);
> +}

I don't understand this as-is. Why do all the counters have the same
value?

> +static void __maybe_unused
> +pmu_write_counters(struct cci_pmu *cci_pmu, unsigned long *mask, u32 value)
> +{
> +	__pmu_write_counters(cci_pmu, mask, value);
> +}

Why are these not just one function for now?

Mark.
--
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]

#1301430

On 04/01/16 19:03, Mark Rutland wrote:
> On Mon, Jan 04, 2016 at 11:54:42AM +0000, Suzuki K. Poulose wrote:
>> Add a helper to group the writes to PMU counter, this will be
>> used to delay setting the event period to pmu::pmu_enable()
>>

>> +/* Write a value to a given set of counters */
>> +static void __pmu_write_counters(struct cci_pmu *cci_pmu, unsigned long *mask, u32 value)
>> +{
>> +	int i;
>> +
>> +	for_each_set_bit(i, mask, cci_pmu->num_cntrs)
>> +		__pmu_write_counter(cci_pmu, value, i);
>> +}
>
> I don't understand this as-is. Why do all the counters have the same
> value?

The only value we write to the counters is the period. This routine writes
a given value to a set of counters specified by the mask (not to be confused
with the PMU->hw_events->mask). This will help to group the writes to the counters,
especially since preparatory steps to write to a single counter itself is costly.
So, we do all the preparation only once for a batch of counters.

The other option is to use hw_events->prev_count (which should be set before calling
the function) for each counter specified in the mask. I am fine with either of the
two.

>
>> +static void __maybe_unused
>> +pmu_write_counters(struct cci_pmu *cci_pmu, unsigned long *mask, u32 value)
>> +{
>> +	__pmu_write_counters(cci_pmu, mask, value);
>> +}
>
> Why are these not just one function for now?

Yes, this could be just one function for now, until we introduce the hooks. This was
a written to avoid another refactoring in the later patch.

Thanks
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]

#1306043

On Tue, Jan 05, 2016 at 10:51:47AM +0000, Suzuki K. Poulose wrote:
> On 04/01/16 19:03, Mark Rutland wrote:
> >On Mon, Jan 04, 2016 at 11:54:42AM +0000, Suzuki K. Poulose wrote:
> >>Add a helper to group the writes to PMU counter, this will be
> >>used to delay setting the event period to pmu::pmu_enable()
> >>
> 
> >>+/* Write a value to a given set of counters */
> >>+static void __pmu_write_counters(struct cci_pmu *cci_pmu, unsigned long *mask, u32 value)
> >>+{
> >>+	int i;
> >>+
> >>+	for_each_set_bit(i, mask, cci_pmu->num_cntrs)
> >>+		__pmu_write_counter(cci_pmu, value, i);
> >>+}
> >
> >I don't understand this as-is. Why do all the counters have the same
> >value?
> 
> The only value we write to the counters is the period. This routine writes
> a given value to a set of counters specified by the mask (not to be confused
> with the PMU->hw_events->mask). This will help to group the writes to the counters,
> especially since preparatory steps to write to a single counter itself is costly.
> So, we do all the preparation only once for a batch of counters.
> 
> The other option is to use hw_events->prev_count (which should be set before calling
> the function) for each counter specified in the mask. I am fine with either of the
> two.

I think this would be clearer using prev_count.

I guess it doesn't matter since we won't support sampling, but it would
match the shape of other PMU drivers.

> >>+static void __maybe_unused
> >>+pmu_write_counters(struct cci_pmu *cci_pmu, unsigned long *mask, u32 value)
> >>+{
> >>+	__pmu_write_counters(cci_pmu, mask, value);
> >>+}
> >
> >Why are these not just one function for now?
> 
> Yes, this could be just one function for now, until we introduce the hooks. This was
> a written to avoid another refactoring in the later patch.

Ok. Either way is fine.

Thanks,
Mark.

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

#1306045

On 11/01/16 10:44, Mark Rutland wrote:
> On Tue, Jan 05, 2016 at 10:51:47AM +0000, Suzuki K. Poulose wrote:
>> On 04/01/16 19:03, Mark Rutland wrote:
>>> On Mon, Jan 04, 2016 at 11:54:42AM +0000, Suzuki K. Poulose wrote:
>>>> Add a helper to group the writes to PMU counter, this will be
>>>> used to delay setting the event period to pmu::pmu_enable()
>>>>
>>
>>>> +/* Write a value to a given set of counters */
>>>> +static void __pmu_write_counters(struct cci_pmu *cci_pmu, unsigned long *mask, u32 value)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for_each_set_bit(i, mask, cci_pmu->num_cntrs)
>>>> +		__pmu_write_counter(cci_pmu, value, i);
>>>> +}
>>>
>>> I don't understand this as-is. Why do all the counters have the same
>>> value?
>>
>> The only value we write to the counters is the period. This routine writes
>> a given value to a set of counters specified by the mask (not to be confused
>> with the PMU->hw_events->mask). This will help to group the writes to the counters,
>> especially since preparatory steps to write to a single counter itself is costly.
>> So, we do all the preparation only once for a batch of counters.
>>
>> The other option is to use hw_events->prev_count (which should be set before calling
>> the function) for each counter specified in the mask. I am fine with either of the
>> two.
>
> I think this would be clearer using prev_count.
>
> I guess it doesn't matter since we won't support sampling, but it would
> match the shape of other PMU drivers.

OK, will use that.

Thanks
Suzuki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web