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

Groups > linux.kernel > #1413481 > unrolled thread

[PATCH 0/2] qcom: add l2 cache perf events driver

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] qcom: add l2 cache perf events driver Neil Leeder <nleeder@codeaurora.org> - 2016-06-03 23:10 +0200
    [PATCH 1/2] perf: allow add to change event state Neil Leeder <nleeder@codeaurora.org> - 2016-06-03 23:10 +0200
      Re: [PATCH 1/2] perf: allow add to change event state Peter Zijlstra <peterz@infradead.org> - 2016-06-03 23:50 +0200
    Re: [PATCH 0/2] qcom: add l2 cache perf events driver Mark Rutland <mark.rutland@arm.com> - 2016-06-06 11:10 +0200
      Re: [PATCH 0/2] qcom: add l2 cache perf events driver Neil Leeder <nleeder@codeaurora.org> - 2016-06-08 17:30 +0200
        Re: [PATCH 0/2] qcom: add l2 cache perf events driver Mark Rutland <mark.rutland@arm.com> - 2016-06-08 18:20 +0200
          Re: [PATCH 0/2] qcom: add l2 cache perf events driver Neil Leeder <nleeder@codeaurora.org> - 2016-06-08 21:30 +0200

#1413481 — [PATCH 0/2] qcom: add l2 cache perf events driver

This adds a new dynamic PMU to the Perf Events framework to program
and control the L2 cache PMUs in some Qualcomm Technologies SOCs.

The driver exports formatting and event information to sysfs so it can
be used by the perf user space tools with the syntax:
perf stat -e l2cache/event=0x42/

One point to note is that there are certain combinations of events
which are invalid, and which are detected in event_add(). Simply having
event_add() fail would result in event_sched_in() making it Inactive,
treating it as over-allocation of counters, leading to
repeated attempts to allocate the events and ending up with a
statistical count.  A solution for this situation is to turn the
conflicting event off in event_add(). This allows a single error
message to be generated, and no recurring attempts to re-add
the invalid event. In order for this to work, event_sched_in()
needs to detect that event_add() changed the state, and not override it
and force it to Inactive.

This patchset requires:
[PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

Neil Leeder (2):
  perf: allow add to change event state
  soc: qcom: add l2 cache perf events driver

 drivers/soc/qcom/Kconfig               |  10 +
 drivers/soc/qcom/Makefile              |   1 +
 drivers/soc/qcom/perf_event_l2.c       | 917 +++++++++++++++++++++++++++++++++
 include/linux/soc/qcom/perf_event_l2.h |  82 +++
 kernel/events/core.c                   |   3 +-
 5 files changed, 1012 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/qcom/perf_event_l2.c
 create mode 100644 include/linux/soc/qcom/perf_event_l2.h

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [next] | [standalone]

#1413486 — [PATCH 1/2] perf: allow add to change event state

When the platform-specific pmu->add function returns
an error, it may have also changed the event's state.
If so, do not override that new state.

Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
---
 kernel/events/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index c0ded24..95c4cf3d3 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1952,7 +1952,8 @@ event_sched_in(struct perf_event *event,
 	perf_log_itrace_start(event);
 
 	if (event->pmu->add(event, PERF_EF_START)) {
-		event->state = PERF_EVENT_STATE_INACTIVE;
+		if (event->state == PERF_EVENT_STATE_ACTIVE)
+			event->state = PERF_EVENT_STATE_INACTIVE;
 		event->oncpu = -1;
 		ret = -EAGAIN;
 		goto out;
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

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

#1413510 — Re: [PATCH 1/2] perf: allow add to change event state

On Fri, Jun 03, 2016 at 05:03:31PM -0400, Neil Leeder wrote:
> When the platform-specific pmu->add function returns
> an error, it may have also changed the event's state.
> If so, do not override that new state.

This is inadequate; it fails to what the problem is and why this is a
good solution.

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

#1414802

On Fri, Jun 03, 2016 at 05:03:30PM -0400, Neil Leeder wrote:
> This adds a new dynamic PMU to the Perf Events framework to program
> and control the L2 cache PMUs in some Qualcomm Technologies SOCs.
> 
> The driver exports formatting and event information to sysfs so it can
> be used by the perf user space tools with the syntax:
> perf stat -e l2cache/event=0x42/
> 
> One point to note is that there are certain combinations of events
> which are invalid, and which are detected in event_add().

Which combinations of events are invalid?

Please elaborate.

> Simply having event_add() fail would result in event_sched_in() making
> it Inactive, treating it as over-allocation of counters, leading to
> repeated attempts to allocate the events and ending up with a
> statistical count.  A solution for this situation is to turn the
> conflicting event off in event_add(). This allows a single error
> message to be generated, and no recurring attempts to re-add the
> invalid event. In order for this to work, event_sched_in()
> needs to detect that event_add() changed the state, and not override it
> and force it to Inactive.

For heterogeneous PMUs, we added the pmu::filter_match(event) callback
for a similar purpose: preventing an event from being scheduled on a
core which does not support that event, while allowing other events to
be scheduled.

So if you truly need to filter events, the infrastructure for doing so
already exists.

However, you will need to elaborate on "there are certain combinations
of events which are invalid".

> This patchset requires:
> [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

A link would be remarkably helpful.

Better would be to fold that patch into this series, as it's the only
user, and both are helpful review context for the other.

Thanks,
Mark.

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

#1417576

On 6/6/2016 05:04 AM, Mark Rutland wrote:
> On Fri, Jun 03, 2016 at 05:03:30PM -0400, Neil Leeder wrote:
>> This adds a new dynamic PMU to the Perf Events framework to program
>> and control the L2 cache PMUs in some Qualcomm Technologies SOCs.
>>
>> The driver exports formatting and event information to sysfs so it can
>> be used by the perf user space tools with the syntax:
>> perf stat -e l2cache/event=0x42/
>>
>> One point to note is that there are certain combinations of events
>> which are invalid, and which are detected in event_add().
> 
> Which combinations of events are invalid?
> 
> Please elaborate.
> 
>> Simply having event_add() fail would result in event_sched_in() making
>> it Inactive, treating it as over-allocation of counters, leading to
>> repeated attempts to allocate the events and ending up with a
>> statistical count.  A solution for this situation is to turn the
>> conflicting event off in event_add(). This allows a single error
>> message to be generated, and no recurring attempts to re-add the
>> invalid event. In order for this to work, event_sched_in()
>> needs to detect that event_add() changed the state, and not override it
>> and force it to Inactive.
> 
> For heterogeneous PMUs, we added the pmu::filter_match(event) callback
> for a similar purpose: preventing an event from being scheduled on a
> core which does not support that event, while allowing other events to
> be scheduled.
> 
> So if you truly need to filter events, the infrastructure for doing so
> already exists.
> 
> However, you will need to elaborate on "there are certain combinations
> of events which are invalid".
> 

Qualcomm PMUs have events arranged in a matrix of rows and columns.
Only one event can be enabled from each column at once. So this isn't a
heterogeneous CPU issue, and it doesn't seem to fit into filter_match()
because it is not an absolute restriction that this event can't be
enabled on this cpu, it's related to the other events which have 
already been enabled.

>> This patchset requires:
>> [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers
> 
> A link would be remarkably helpful.

http://archive.arm.linux.org.uk/lurker/message/20160603.205900.1970f20d.en.html

> 
> Better would be to fold that patch into this series, as it's the only
> user, and both are helpful review context for the other.
> 

The L2 PMU driver is the first user of the L2-accessors patch
but it won't be the only one, which is why I kept it separate.

> Thanks,
> Mark.
> 

Neil

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

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

#1417641

On Wed, Jun 08, 2016 at 11:21:16AM -0400, Neil Leeder wrote:
> 
> 
> On 6/6/2016 05:04 AM, Mark Rutland wrote:
> > On Fri, Jun 03, 2016 at 05:03:30PM -0400, Neil Leeder wrote:
> >> This adds a new dynamic PMU to the Perf Events framework to program
> >> and control the L2 cache PMUs in some Qualcomm Technologies SOCs.
> >>
> >> The driver exports formatting and event information to sysfs so it can
> >> be used by the perf user space tools with the syntax:
> >> perf stat -e l2cache/event=0x42/
> >>
> >> One point to note is that there are certain combinations of events
> >> which are invalid, and which are detected in event_add().
> > 
> > Which combinations of events are invalid?
> > 
> > Please elaborate.
> > 
> >> Simply having event_add() fail would result in event_sched_in() making
> >> it Inactive, treating it as over-allocation of counters, leading to
> >> repeated attempts to allocate the events and ending up with a
> >> statistical count.  A solution for this situation is to turn the
> >> conflicting event off in event_add(). This allows a single error
> >> message to be generated, and no recurring attempts to re-add the
> >> invalid event. In order for this to work, event_sched_in()
> >> needs to detect that event_add() changed the state, and not override it
> >> and force it to Inactive.
> > 
> > For heterogeneous PMUs, we added the pmu::filter_match(event) callback
> > for a similar purpose: preventing an event from being scheduled on a
> > core which does not support that event, while allowing other events to
> > be scheduled.
> > 
> > So if you truly need to filter events, the infrastructure for doing so
> > already exists.
> > 
> > However, you will need to elaborate on "there are certain combinations
> > of events which are invalid".
> > 
> 
> Qualcomm PMUs have events arranged in a matrix of rows and columns.
> Only one event can be enabled from each column at once. So this isn't a
> heterogeneous CPU issue, and it doesn't seem to fit into filter_match()
> because it is not an absolute restriction that this event can't be
> enabled on this cpu, it's related to the other events which have 
> already been enabled.

The above is useful context. Please add (something like) it to the cover
and relevant patches in future postings!

Ok. So if I understand correctly, each counter can only count certain
events (and therefore each event can only go into some counters), rather
than all counters being identical?

So the issue is that there is no _suitable_ counter available for an
event, but there are still counters available for events in general.

This case is somewhat different to the heterogeneous PMU case.

Unfortunately, trying to filter events in this manner can be very
expensive, and allows a malicious user to DoS the system, as Peter
pointed out when I tried to do similar things in this area. Take a look
at [1] and associated replies.

If you can test the availability of a relevant counter very cheaply,
then having a specific return code for the case of no relevant counter
may be more palatable.

> >> This patchset requires:
> >> [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers
> > 
> > A link would be remarkably helpful.
> 
> http://archive.arm.linux.org.uk/lurker/message/20160603.205900.1970f20d.en.html
> 
> > 
> > Better would be to fold that patch into this series, as it's the only
> > user, and both are helpful review context for the other.
> > 
> 
> The L2 PMU driver is the first user of the L2-accessors patch
> but it won't be the only one, which is why I kept it separate.

If other users aren't going to appear in the same merge window, IMO it
would be better to place them in the same series for now. Otherwise,
please have a link in the cover in future postings.

Thanks,
Mark.

[1] http://lkml.kernel.org/r/1392054264-23570-5-git-send-email-mark.rutland@arm.com

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

#1417790

On 6/8/2016 12:12 PM, Mark Rutland wrote:
> On Wed, Jun 08, 2016 at 11:21:16AM -0400, Neil Leeder wrote:
>>
>>
>> On 6/6/2016 05:04 AM, Mark Rutland wrote:
>>> On Fri, Jun 03, 2016 at 05:03:30PM -0400, Neil Leeder wrote:
>>>> This adds a new dynamic PMU to the Perf Events framework to program
>>>> and control the L2 cache PMUs in some Qualcomm Technologies SOCs.
>>>>
>>>> The driver exports formatting and event information to sysfs so it can
>>>> be used by the perf user space tools with the syntax:
>>>> perf stat -e l2cache/event=0x42/
>>>>
>>>> One point to note is that there are certain combinations of events
>>>> which are invalid, and which are detected in event_add().
>>>
>>> Which combinations of events are invalid?
>>>
>>> Please elaborate.
>>>
>>>> Simply having event_add() fail would result in event_sched_in() making
>>>> it Inactive, treating it as over-allocation of counters, leading to
>>>> repeated attempts to allocate the events and ending up with a
>>>> statistical count.  A solution for this situation is to turn the
>>>> conflicting event off in event_add(). This allows a single error
>>>> message to be generated, and no recurring attempts to re-add the
>>>> invalid event. In order for this to work, event_sched_in()
>>>> needs to detect that event_add() changed the state, and not override it
>>>> and force it to Inactive.
>>>
>>> For heterogeneous PMUs, we added the pmu::filter_match(event) callback
>>> for a similar purpose: preventing an event from being scheduled on a
>>> core which does not support that event, while allowing other events to
>>> be scheduled.
>>>
>>> So if you truly need to filter events, the infrastructure for doing so
>>> already exists.
>>>
>>> However, you will need to elaborate on "there are certain combinations
>>> of events which are invalid".
>>>
>>
>> Qualcomm PMUs have events arranged in a matrix of rows and columns.
>> Only one event can be enabled from each column at once. So this isn't a
>> heterogeneous CPU issue, and it doesn't seem to fit into filter_match()
>> because it is not an absolute restriction that this event can't be
>> enabled on this cpu, it's related to the other events which have 
>> already been enabled.
> 
> The above is useful context. Please add (something like) it to the cover
> and relevant patches in future postings!
> 
> Ok. So if I understand correctly, each counter can only count certain
> events (and therefore each event can only go into some counters), rather
> than all counters being identical?
> 
> So the issue is that there is no _suitable_ counter available for an
> event, but there are still counters available for events in general.
> 
> This case is somewhat different to the heterogeneous PMU case.
> 
> Unfortunately, trying to filter events in this manner can be very
> expensive, and allows a malicious user to DoS the system, as Peter
> pointed out when I tried to do similar things in this area. Take a look
> at [1] and associated replies.
> 
> If you can test the availability of a relevant counter very cheaply,
> then having a specific return code for the case of no relevant counter
> may be more palatable.
> 

Not quite. Any event can go into any counter, but once an event from a given
column has been assigned to a counter, no other events from the same column
can be placed in any other counter.

Here I detect this condition on the first call to pmu->add() for
the conflicting event, and turn that event's state to Off.
That should ensure there are no more attempts to schedule it, which should avoid
DoS concerns.

But I may see if filter_match() could be used here anyway. Instead of having a
static list of valid PMUs, look at the list of already enabled events for this PMU
and fail if the conflict is detected. I think this would remove the need for a
change in state if add() is never called for the event.

>>>> This patchset requires:
>>>> [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers
>>>
>>> A link would be remarkably helpful.
>>
>> http://archive.arm.linux.org.uk/lurker/message/20160603.205900.1970f20d.en.html
>>
>>>
>>> Better would be to fold that patch into this series, as it's the only
>>> user, and both are helpful review context for the other.
>>>
>>
>> The L2 PMU driver is the first user of the L2-accessors patch
>> but it won't be the only one, which is why I kept it separate.
> 
> If other users aren't going to appear in the same merge window, IMO it
> would be better to place them in the same series for now. Otherwise,
> please have a link in the cover in future postings.

Ok, makes sense.

> Thanks,
> Mark.
> 
> [1] http://lkml.kernel.org/r/1392054264-23570-5-git-send-email-mark.rutland@arm.com
> 

Neil

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web