[PATCH 0/4] MIPS/Perf-events: Functional fixes and cleanups

[PATCH 0/4] MIPS/Perf-events: Functional fixes and cleanups

[Deng-Cheng Zhu]

As patch names suggest, this series contains various fixes and does a
slight cleanup.

Deng-Cheng Zhu (4):
  MIPS/Perf-events: update the map of unsupported events for 74K
  MIPS/Perf-events: remove erroneous check on active_events
  MIPS/Perf-events: temporarily connect event to its pmu at event init
  MIPS/Perf-events: Cleanup event->destroy at event init

 arch/mips/kernel/perf_event.c        |   11 +---------
 arch/mips/kernel/perf_event_mipsxx.c |   35 ++++++++++++++++++++++++---------
 2 files changed, 26 insertions(+), 20 deletions(-)

[PATCH 0/4] MIPS/Perf-events: Functional fixes and cleanups

[Deng-Cheng Zhu]

As patch names suggest, this series contains various fixes and does a
slight cleanup.

Deng-Cheng Zhu (4):
  MIPS/Perf-events: update the map of unsupported events for 74K
  MIPS/Perf-events: remove erroneous check on active_events
  MIPS/Perf-events: temporarily connect event to its pmu at event init
  MIPS/Perf-events: Cleanup event->destroy at event init

 arch/mips/kernel/perf_event.c        |   11 +---------
 arch/mips/kernel/perf_event_mipsxx.c |   35 ++++++++++++++++++++++++---------
 2 files changed, 26 insertions(+), 20 deletions(-)

[PATCH 1/4] MIPS/Perf-events: update the map of unsupported events for 74K

[Deng-Cheng Zhu]

Update the raw event info for 74K according to the latest document.

Signed-off-by: Deng-Cheng Zhu <dczhu@mips.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: David Daney <david.daney@cavium.com>
---
 arch/mips/kernel/perf_event_mipsxx.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
index e5ad09a..1f654ca 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -858,13 +858,16 @@ mipsxx_pmu_disable_event(int idx)
 #endif
 
 /* 74K */
+/*
+ * MIPS document MD00519 (MIPS32(r) 74K(tm) Processor Core Family Software
+ * User's Manual, Revision 01.05)
+ */
 #define IS_UNSUPPORTED_74K_EVENT(r, b)					\
-	((r) == 5 || ((r) >= 135 && (r) <= 137) ||			\
-	 ((b) >= 10 && (b) <= 12) || (b) == 22 || (b) == 27 ||		\
-	 (b) == 33 || (b) == 34 || ((b) >= 47 && (b) <= 49) ||		\
-	 (r) == 178 || (b) == 55 || (b) == 57 || (b) == 60 ||		\
-	 (b) == 61 || (r) == 62 || (r) == 191 ||			\
-	 ((b) >= 64 && (b) <= 127))
+	((r) == 5 || (r) == 135 || ((b) >= 10 && (b) <= 12) ||		\
+	 (b) == 27 || (b) == 33 || (b) == 34 || (b) == 47 ||		\
+	 (b) == 48 || (r) == 178 || (r) == 187 || (b) == 60 ||		\
+	 (b) == 61 || (r) == 191 || (r) == 71 || (r) == 72 ||		\
+	 (b) == 73 || ((b) >= 77 && (b) <= 127))
 #define IS_BOTH_COUNTERS_74K_EVENT(b)					\
 	((b) == 0 || (b) == 1)
 
-- 
1.7.1

[PATCH 1/4] MIPS/Perf-events: update the map of unsupported events for 74K

[Deng-Cheng Zhu]

Update the raw event info for 74K according to the latest document.

Signed-off-by: Deng-Cheng Zhu <dczhu@mips.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: David Daney <david.daney@cavium.com>
---
 arch/mips/kernel/perf_event_mipsxx.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
index e5ad09a..1f654ca 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -858,13 +858,16 @@ mipsxx_pmu_disable_event(int idx)
 #endif
 
 /* 74K */
+/*
+ * MIPS document MD00519 (MIPS32(r) 74K(tm) Processor Core Family Software
+ * User's Manual, Revision 01.05)
+ */
 #define IS_UNSUPPORTED_74K_EVENT(r, b)					\
-	((r) == 5 || ((r) >= 135 && (r) <= 137) ||			\
-	 ((b) >= 10 && (b) <= 12) || (b) == 22 || (b) == 27 ||		\
-	 (b) == 33 || (b) == 34 || ((b) >= 47 && (b) <= 49) ||		\
-	 (r) == 178 || (b) == 55 || (b) == 57 || (b) == 60 ||		\
-	 (b) == 61 || (r) == 62 || (r) == 191 ||			\
-	 ((b) >= 64 && (b) <= 127))
+	((r) == 5 || (r) == 135 || ((b) >= 10 && (b) <= 12) ||		\
+	 (b) == 27 || (b) == 33 || (b) == 34 || (b) == 47 ||		\
+	 (b) == 48 || (r) == 178 || (r) == 187 || (b) == 60 ||		\
+	 (b) == 61 || (r) == 191 || (r) == 71 || (r) == 72 ||		\
+	 (b) == 73 || ((b) >= 77 && (b) <= 127))
 #define IS_BOTH_COUNTERS_74K_EVENT(b)					\
 	((b) == 0 || (b) == 1)
 
-- 
1.7.1

[PATCH 2/4] MIPS/Perf-events: remove erroneous check on active_events

[Deng-Cheng Zhu]

Port the following patch for ARM by Mark Rutland:

- 57ce9bb39b476accf8fba6e16aea67ed76ea523d
    ARM: 6902/1: perf: Remove erroneous check on active_events

    When initialising a PMU, there is a check to protect against races with
    other CPUs filling all of the available event slots. Since armpmu_add
    checks that an event can be scheduled, we do not need to do this at
    initialisation time. Furthermore the current code is broken because it
    assumes that atomic_inc_not_zero will unconditionally increment
    active_counts and then tries to decrement it again on failure.

    This patch removes the broken, redundant code.

Signed-off-by: Deng-Cheng Zhu <dczhu@mips.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: David Daney <david.daney@cavium.com>
---
 arch/mips/kernel/perf_event.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
index 0aee944..b1d51b5 100644
--- a/arch/mips/kernel/perf_event.c
+++ b/arch/mips/kernel/perf_event.c
@@ -385,11 +385,6 @@ static int mipspmu_event_init(struct perf_event *event)
 		return -ENODEV;
 
 	if (!atomic_inc_not_zero(&active_events)) {
-		if (atomic_read(&active_events) > MIPS_MAX_HWEVENTS) {
-			atomic_dec(&active_events);
-			return -ENOSPC;
-		}
-
 		mutex_lock(&pmu_reserve_mutex);
 		if (atomic_read(&active_events) == 0)
 			err = mipspmu_get_irq();
-- 
1.7.1

[PATCH 2/4] MIPS/Perf-events: remove erroneous check on active_events

[Deng-Cheng Zhu]

Port the following patch for ARM by Mark Rutland:

- 57ce9bb39b476accf8fba6e16aea67ed76ea523d
    ARM: 6902/1: perf: Remove erroneous check on active_events

    When initialising a PMU, there is a check to protect against races with
    other CPUs filling all of the available event slots. Since armpmu_add
    checks that an event can be scheduled, we do not need to do this at
    initialisation time. Furthermore the current code is broken because it
    assumes that atomic_inc_not_zero will unconditionally increment
    active_counts and then tries to decrement it again on failure.

    This patch removes the broken, redundant code.

Signed-off-by: Deng-Cheng Zhu <dczhu@mips.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: David Daney <david.daney@cavium.com>
---
 arch/mips/kernel/perf_event.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
index 0aee944..b1d51b5 100644
--- a/arch/mips/kernel/perf_event.c
+++ b/arch/mips/kernel/perf_event.c
@@ -385,11 +385,6 @@ static int mipspmu_event_init(struct perf_event *event)
 		return -ENODEV;
 
 	if (!atomic_inc_not_zero(&active_events)) {
-		if (atomic_read(&active_events) > MIPS_MAX_HWEVENTS) {
-			atomic_dec(&active_events);
-			return -ENOSPC;
-		}
-
 		mutex_lock(&pmu_reserve_mutex);
 		if (atomic_read(&active_events) == 0)
 			err = mipspmu_get_irq();
-- 
1.7.1

[PATCH 3/4] MIPS/Perf-events: temporarily connect event to its pmu at event init

[Deng-Cheng Zhu]

When arch level event init is called, the event is not yet connected to
the PMU, thereby causing validate_group() to always do dummy work. On MIPS,
this is due to the following lines in validate_event() called by
validate_group():

if (event->pmu != &pmu || event->state <= PERF_EVENT_STATE_OFF)
        return 1;

This patch fixes it.

Signed-off-by: Deng-Cheng Zhu <dczhu@mips.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: David Daney <david.daney@cavium.com>
---
 arch/mips/kernel/perf_event_mipsxx.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
index 1f654ca..c804fdd 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -548,6 +548,7 @@ static int __hw_perf_event_init(struct perf_event *event)
 	struct perf_event_attr *attr = &event->attr;
 	struct hw_perf_event *hwc = &event->hw;
 	const struct mips_perf_event *pev;
+	struct pmu *tmp;
 	int err;
 
 	/* Returning MIPS event descriptor for generic perf event. */
@@ -611,11 +612,19 @@ static int __hw_perf_event_init(struct perf_event *event)
 	}
 
 	err = 0;
-	if (event->group_leader != event) {
+
+	/*
+	 * we temporarily connect event to its pmu such that
+	 * validate_event() in validate_group() can classify
+	 * it as a MIPS event by passing (event->pmu == &pmu).
+	 */
+	tmp = event->pmu;
+	event->pmu = &pmu;
+
+	if (event->group_leader != event)
 		err = validate_group(event);
-		if (err)
-			return -EINVAL;
-	}
+
+	event->pmu = tmp;
 
 	event->destroy = hw_perf_event_destroy;
 
-- 
1.7.1

[PATCH 3/4] MIPS/Perf-events: temporarily connect event to its pmu at event init

[Deng-Cheng Zhu]

When arch level event init is called, the event is not yet connected to
the PMU, thereby causing validate_group() to always do dummy work. On MIPS,
this is due to the following lines in validate_event() called by
validate_group():

if (event->pmu != &pmu || event->state <= PERF_EVENT_STATE_OFF)
        return 1;

This patch fixes it.

Signed-off-by: Deng-Cheng Zhu <dczhu@mips.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: David Daney <david.daney@cavium.com>
---
 arch/mips/kernel/perf_event_mipsxx.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
index 1f654ca..c804fdd 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -548,6 +548,7 @@ static int __hw_perf_event_init(struct perf_event *event)
 	struct perf_event_attr *attr = &event->attr;
 	struct hw_perf_event *hwc = &event->hw;
 	const struct mips_perf_event *pev;
+	struct pmu *tmp;
 	int err;
 
 	/* Returning MIPS event descriptor for generic perf event. */
@@ -611,11 +612,19 @@ static int __hw_perf_event_init(struct perf_event *event)
 	}
 
 	err = 0;
-	if (event->group_leader != event) {
+
+	/*
+	 * we temporarily connect event to its pmu such that
+	 * validate_event() in validate_group() can classify
+	 * it as a MIPS event by passing (event->pmu == &pmu).
+	 */
+	tmp = event->pmu;
+	event->pmu = &pmu;
+
+	if (event->group_leader != event)
 		err = validate_group(event);
-		if (err)
-			return -EINVAL;
-	}
+
+	event->pmu = tmp;
 
 	event->destroy = hw_perf_event_destroy;
 
-- 
1.7.1

[PATCH 4/4] MIPS/Perf-events: Cleanup event->destroy at event init

[Deng-Cheng Zhu]

Simplify the code by changing the place of event->destroy().

Signed-off-by: Deng-Cheng Zhu <dczhu@mips.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: David Daney <david.daney@cavium.com>
---
 arch/mips/kernel/perf_event.c        |    6 +-----
 arch/mips/kernel/perf_event_mipsxx.c |    3 +++
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
index b1d51b5..7a8d0a0 100644
--- a/arch/mips/kernel/perf_event.c
+++ b/arch/mips/kernel/perf_event.c
@@ -397,11 +397,7 @@ static int mipspmu_event_init(struct perf_event *event)
 	if (err)
 		return err;
 
-	err = __hw_perf_event_init(event);
-	if (err)
-		hw_perf_event_destroy(event);
-
-	return err;
+	return __hw_perf_event_init(event);
 }
 
 static struct pmu pmu = {
diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
index c804fdd..bd75473 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -628,6 +628,9 @@ static int __hw_perf_event_init(struct perf_event *event)
 
 	event->destroy = hw_perf_event_destroy;
 
+	if (err)
+		event->destroy(event);
+
 	return err;
 }
 
-- 
1.7.1

[PATCH 4/4] MIPS/Perf-events: Cleanup event->destroy at event init

[Deng-Cheng Zhu]

Simplify the code by changing the place of event->destroy().

Signed-off-by: Deng-Cheng Zhu <dczhu@mips.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: David Daney <david.daney@cavium.com>
---
 arch/mips/kernel/perf_event.c        |    6 +-----
 arch/mips/kernel/perf_event_mipsxx.c |    3 +++
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
index b1d51b5..7a8d0a0 100644
--- a/arch/mips/kernel/perf_event.c
+++ b/arch/mips/kernel/perf_event.c
@@ -397,11 +397,7 @@ static int mipspmu_event_init(struct perf_event *event)
 	if (err)
 		return err;
 
-	err = __hw_perf_event_init(event);
-	if (err)
-		hw_perf_event_destroy(event);
-
-	return err;
+	return __hw_perf_event_init(event);
 }
 
 static struct pmu pmu = {
diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
index c804fdd..bd75473 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -628,6 +628,9 @@ static int __hw_perf_event_init(struct perf_event *event)
 
 	event->destroy = hw_perf_event_destroy;
 
+	if (err)
+		event->destroy(event);
+
 	return err;
 }
 
-- 
1.7.1

Re: [PATCH 3/4] MIPS/Perf-events: temporarily connect event to its pmu at event init

[Deng-Cheng Zhu]

On 10/24/2011 06:56 PM, Deng-Cheng Zhu wrote:
> When arch level event init is called, the event is not yet connected to
> the PMU, thereby causing validate_group() to always do dummy work. On MIPS,
> this is due to the following lines in validate_event() called by
> validate_group():
>
> if (event->pmu !=&pmu || event->state<= PERF_EVENT_STATE_OFF)
>          return 1;
>
> This patch fixes it.
>
> Signed-off-by: Deng-Cheng Zhu<dczhu@mips.com>
> Cc: Peter Zijlstra<a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras<paulus@samba.org>
> Cc: Ingo Molnar<mingo@elte.hu>
> Cc: Arnaldo Carvalho de Melo<acme@ghostprotocols.net>
> Cc: David Daney<david.daney@cavium.com>
> ---
>   arch/mips/kernel/perf_event_mipsxx.c |   17 +++++++++++++----
>   1 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
> index 1f654ca..c804fdd 100644
> --- a/arch/mips/kernel/perf_event_mipsxx.c
> +++ b/arch/mips/kernel/perf_event_mipsxx.c
> @@ -548,6 +548,7 @@ static int __hw_perf_event_init(struct perf_event *event)
>   	struct perf_event_attr *attr =&event->attr;
>   	struct hw_perf_event *hwc =&event->hw;
>   	const struct mips_perf_event *pev;
> +	struct pmu *tmp;
>   	int err;
>
>   	/* Returning MIPS event descriptor for generic perf event. */
> @@ -611,11 +612,19 @@ static int __hw_perf_event_init(struct perf_event *event)
>   	}
>
>   	err = 0;
> -	if (event->group_leader != event) {
> +
> +	/*
> +	 * we temporarily connect event to its pmu such that
> +	 * validate_event() in validate_group() can classify
> +	 * it as a MIPS event by passing (event->pmu ==&pmu).
> +	 */
> +	tmp = event->pmu;
> +	event->pmu =&pmu;
> +
> +	if (event->group_leader != event)
>   		err = validate_group(event);
> -		if (err)
> -			return -EINVAL;
> -	}
> +
> +	event->pmu = tmp;
>
>   	event->destroy = hw_perf_event_destroy;
>

After looking at David Ahern's reply to my another patch (link
provided below), I started to think whether the PMU and event state
checks are redundant in validate_event() on MIPS (ARM may also have
the same consideration).

The related patch link:
http://www.spinics.net/lists/kernel/msg1252452.html

_If_ the state fix goes to group checks instead of to the perf tool,
then the following line in validate_event() on MIPS seems redundant:

if (event->pmu !=&pmu || event->state <= PERF_EVENT_STATE_OFF)
         return 1;

This is because validate_event() is only called by validate_group()
which is called only by __hw_perf_event_init(), and the issue of the
pmu check is already addressed in this patch, and we don't want the
grouped events which are in PERF_EVENT_STATE_OFF (by "attr->disabled =
1") to be filtered out here.

Also, _if_ the state fix goes to group checks instead of to the perf
tool, then X86 may need a patch to allow collect_events() to do real
work in validate_group().

Any comments?


Deng-Cheng

Re: [PATCH 3/4] MIPS/Perf-events: temporarily connect event to its pmu at event init

[Deng-Cheng Zhu]

On 10/24/2011 06:56 PM, Deng-Cheng Zhu wrote:
> When arch level event init is called, the event is not yet connected to
> the PMU, thereby causing validate_group() to always do dummy work. On MIPS,
> this is due to the following lines in validate_event() called by
> validate_group():
>
> if (event->pmu !=&pmu || event->state<= PERF_EVENT_STATE_OFF)
>          return 1;
>
> This patch fixes it.
>
> Signed-off-by: Deng-Cheng Zhu<dczhu@mips.com>
> Cc: Peter Zijlstra<a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras<paulus@samba.org>
> Cc: Ingo Molnar<mingo@elte.hu>
> Cc: Arnaldo Carvalho de Melo<acme@ghostprotocols.net>
> Cc: David Daney<david.daney@cavium.com>
> ---
>   arch/mips/kernel/perf_event_mipsxx.c |   17 +++++++++++++----
>   1 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
> index 1f654ca..c804fdd 100644
> --- a/arch/mips/kernel/perf_event_mipsxx.c
> +++ b/arch/mips/kernel/perf_event_mipsxx.c
> @@ -548,6 +548,7 @@ static int __hw_perf_event_init(struct perf_event *event)
>   	struct perf_event_attr *attr =&event->attr;
>   	struct hw_perf_event *hwc =&event->hw;
>   	const struct mips_perf_event *pev;
> +	struct pmu *tmp;
>   	int err;
>
>   	/* Returning MIPS event descriptor for generic perf event. */
> @@ -611,11 +612,19 @@ static int __hw_perf_event_init(struct perf_event *event)
>   	}
>
>   	err = 0;
> -	if (event->group_leader != event) {
> +
> +	/*
> +	 * we temporarily connect event to its pmu such that
> +	 * validate_event() in validate_group() can classify
> +	 * it as a MIPS event by passing (event->pmu ==&pmu).
> +	 */
> +	tmp = event->pmu;
> +	event->pmu =&pmu;
> +
> +	if (event->group_leader != event)
>   		err = validate_group(event);
> -		if (err)
> -			return -EINVAL;
> -	}
> +
> +	event->pmu = tmp;
>
>   	event->destroy = hw_perf_event_destroy;
>

After looking at David Ahern's reply to my another patch (link
provided below), I started to think whether the PMU and event state
checks are redundant in validate_event() on MIPS (ARM may also have
the same consideration).

The related patch link:
http://www.spinics.net/lists/kernel/msg1252452.html

_If_ the state fix goes to group checks instead of to the perf tool,
then the following line in validate_event() on MIPS seems redundant:

if (event->pmu !=&pmu || event->state <= PERF_EVENT_STATE_OFF)
         return 1;

This is because validate_event() is only called by validate_group()
which is called only by __hw_perf_event_init(), and the issue of the
pmu check is already addressed in this patch, and we don't want the
grouped events which are in PERF_EVENT_STATE_OFF (by "attr->disabled =
1") to be filtered out here.

Also, _if_ the state fix goes to group checks instead of to the perf
tool, then X86 may need a patch to allow collect_events() to do real
work in validate_group().

Any comments?


Deng-Cheng

Re: [PATCH 1/4] MIPS/Perf-events: update the map of unsupported events for 74K

[Ralf Baechle]

On Mon, Oct 24, 2011 at 06:55:59PM +0800, Deng-Cheng Zhu wrote:

> Update the raw event info for 74K according to the latest document.

> +/*
> + * MIPS document MD00519 (MIPS32(r) 74K(tm) Processor Core Family Software
> + * User's Manual, Revision 01.05)
> + */
>  #define IS_UNSUPPORTED_74K_EVENT(r, b)					\
> -	((r) == 5 || ((r) >= 135 && (r) <= 137) ||			\
> -	 ((b) >= 10 && (b) <= 12) || (b) == 22 || (b) == 27 ||		\
> -	 (b) == 33 || (b) == 34 || ((b) >= 47 && (b) <= 49) ||		\
> -	 (r) == 178 || (b) == 55 || (b) == 57 || (b) == 60 ||		\
> -	 (b) == 61 || (r) == 62 || (r) == 191 ||			\
> -	 ((b) >= 64 && (b) <= 127))
> +	((r) == 5 || (r) == 135 || ((b) >= 10 && (b) <= 12) ||		\
> +	 (b) == 27 || (b) == 33 || (b) == 34 || (b) == 47 ||		\
> +	 (b) == 48 || (r) == 178 || (r) == 187 || (b) == 60 ||		\
> +	 (b) == 61 || (r) == 191 || (r) == 71 || (r) == 72 ||		\
> +	 (b) == 73 || ((b) >= 77 && (b) <= 127))

I wonder if such detailed checking of the performance counter
event numbers is really needed?  As long as feeding an bad number only
results in undefined counts of the performance counters I think we may
be better of by not checking the event numbers in detail.  Afair there
are MIPS licensee who have modified the counters to count extra events
so I sense some madness in that direction.

  Ralf

Re: [PATCH 1/4] MIPS/Perf-events: update the map of unsupported events for 74K

[Peter Zijlstra]

On Wed, 2011-11-09 at 20:40 +0000, Ralf Baechle wrote:
> On Mon, Oct 24, 2011 at 06:55:59PM +0800, Deng-Cheng Zhu wrote:
> 
> > Update the raw event info for 74K according to the latest document.
> 
> > +/*
> > + * MIPS document MD00519 (MIPS32(r) 74K(tm) Processor Core Family Software
> > + * User's Manual, Revision 01.05)
> > + */
> >  #define IS_UNSUPPORTED_74K_EVENT(r, b)					\
> > -	((r) == 5 || ((r) >= 135 && (r) <= 137) ||			\
> > -	 ((b) >= 10 && (b) <= 12) || (b) == 22 || (b) == 27 ||		\
> > -	 (b) == 33 || (b) == 34 || ((b) >= 47 && (b) <= 49) ||		\
> > -	 (r) == 178 || (b) == 55 || (b) == 57 || (b) == 60 ||		\
> > -	 (b) == 61 || (r) == 62 || (r) == 191 ||			\
> > -	 ((b) >= 64 && (b) <= 127))
> > +	((r) == 5 || (r) == 135 || ((b) >= 10 && (b) <= 12) ||		\
> > +	 (b) == 27 || (b) == 33 || (b) == 34 || (b) == 47 ||		\
> > +	 (b) == 48 || (r) == 178 || (r) == 187 || (b) == 60 ||		\
> > +	 (b) == 61 || (r) == 191 || (r) == 71 || (r) == 72 ||		\
> > +	 (b) == 73 || ((b) >= 77 && (b) <= 127))
> 
> I wonder if such detailed checking of the performance counter
> event numbers is really needed?  As long as feeding an bad number only
> results in undefined counts of the performance counters I think we may
> be better of by not checking the event numbers in detail.  Afair there
> are MIPS licensee who have modified the counters to count extra events
> so I sense some madness in that direction.

Right, we don't do much sanity checking on x86 either, all we do check
are privilege bits, the rest we directly feed to the hardware. This all
works as long as the hardware doesn't in fact fall over or worse of
course. 

On x86 it means you can program events that are outside those specified
in the SDM, some actually count, although outside of specific hardware
team for that chip I doubt there's anybody on the planet who can tell
you what ;-)

Not counting or counter utter crap is fine, that's what you get for
passing in random numbers.

Re: [PATCH 1/4] MIPS/Perf-events: update the map of unsupported events for 74K

[Deng-Cheng Zhu]

On 11/10/2011 04:40 AM, Ralf Baechle wrote:
> On Mon, Oct 24, 2011 at 06:55:59PM +0800, Deng-Cheng Zhu wrote:
>
>> Update the raw event info for 74K according to the latest document.
>
>> +/*
>> + * MIPS document MD00519 (MIPS32(r) 74K(tm) Processor Core Family Software
>> + * User's Manual, Revision 01.05)
>> + */
>>   #define IS_UNSUPPORTED_74K_EVENT(r, b)					\
>> -	((r) == 5 || ((r)>= 135&&  (r)<= 137) ||			\
>> -	 ((b)>= 10&&  (b)<= 12) || (b) == 22 || (b) == 27 ||		\
>> -	 (b) == 33 || (b) == 34 || ((b)>= 47&&  (b)<= 49) ||		\
>> -	 (r) == 178 || (b) == 55 || (b) == 57 || (b) == 60 ||		\
>> -	 (b) == 61 || (r) == 62 || (r) == 191 ||			\
>> -	 ((b)>= 64&&  (b)<= 127))
>> +	((r) == 5 || (r) == 135 || ((b)>= 10&&  (b)<= 12) ||		\
>> +	 (b) == 27 || (b) == 33 || (b) == 34 || (b) == 47 ||		\
>> +	 (b) == 48 || (r) == 178 || (r) == 187 || (b) == 60 ||		\
>> +	 (b) == 61 || (r) == 191 || (r) == 71 || (r) == 72 ||		\
>> +	 (b) == 73 || ((b)>= 77&&  (b)<= 127))
>
> ...
> Afair there are MIPS licensee who have modified the counters to count
> extra events so I sense some madness in that direction.

A good point. Now that the user is working with raw events, the manual 
is being used for sure. The user should have known what will be counted. 
But we should keep the mappings for event cntr_mask and range. Although 
they are also subject to changes by Arch licensees, patches will be 
needed for their CPU in that case.


Deng-Cheng

Re: [PATCH 1/4] MIPS/Perf-events: update the map of unsupported events for 74K

[Deng-Cheng Zhu]

On 11/10/2011 04:40 AM, Ralf Baechle wrote:
> On Mon, Oct 24, 2011 at 06:55:59PM +0800, Deng-Cheng Zhu wrote:
>
>> Update the raw event info for 74K according to the latest document.
>
>> +/*
>> + * MIPS document MD00519 (MIPS32(r) 74K(tm) Processor Core Family Software
>> + * User's Manual, Revision 01.05)
>> + */
>>   #define IS_UNSUPPORTED_74K_EVENT(r, b)					\
>> -	((r) == 5 || ((r)>= 135&&  (r)<= 137) ||			\
>> -	 ((b)>= 10&&  (b)<= 12) || (b) == 22 || (b) == 27 ||		\
>> -	 (b) == 33 || (b) == 34 || ((b)>= 47&&  (b)<= 49) ||		\
>> -	 (r) == 178 || (b) == 55 || (b) == 57 || (b) == 60 ||		\
>> -	 (b) == 61 || (r) == 62 || (r) == 191 ||			\
>> -	 ((b)>= 64&&  (b)<= 127))
>> +	((r) == 5 || (r) == 135 || ((b)>= 10&&  (b)<= 12) ||		\
>> +	 (b) == 27 || (b) == 33 || (b) == 34 || (b) == 47 ||		\
>> +	 (b) == 48 || (r) == 178 || (r) == 187 || (b) == 60 ||		\
>> +	 (b) == 61 || (r) == 191 || (r) == 71 || (r) == 72 ||		\
>> +	 (b) == 73 || ((b)>= 77&&  (b)<= 127))
>
> ...
> Afair there are MIPS licensee who have modified the counters to count
> extra events so I sense some madness in that direction.

A good point. Now that the user is working with raw events, the manual 
is being used for sure. The user should have known what will be counted. 
But we should keep the mappings for event cntr_mask and range. Although 
they are also subject to changes by Arch licensees, patches will be 
needed for their CPU in that case.


Deng-Cheng