[PATCH 0/5] MIPS/Perf-events: Sync with mainline upper layer

[PATCH 0/5] MIPS/Perf-events: Sync with mainline upper layer

[Deng-Cheng Zhu]

Current MIPS Perf-events uses older interfaces to the generic layer. So it
will not work. This patch set fixes this issue by adding MIPS counterparts
for a list of previous commits that went to mainline earlier.

Deng-Cheng Zhu (5):
  MIPS/Perf-events: Work with irq_work
  MIPS/Perf-events: Work with the new PMU interface
  MIPS/Perf-events: Check event state in validate_event()
  MIPS/Perf-events: Work with the new callchain interface
  MIPS/Perf-events: Use unsigned delta for right shift in event update

 arch/mips/Kconfig                    |    1 +
 arch/mips/include/asm/perf_event.h   |   12 +-
 arch/mips/kernel/perf_event.c        |  342 ++++++++++++++++------------------
 arch/mips/kernel/perf_event_mipsxx.c |    4 +-
 4 files changed, 169 insertions(+), 190 deletions(-)

[PATCH 1/5] MIPS/Perf-events: Work with irq_work

[Deng-Cheng Zhu]

This is the MIPS part of the following commit by Peter Zijlstra:

e360adbe29241a0194e10e20595360dd7b98a2b3
	irq_work: Add generic hardirq context callbacks

Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
---
 arch/mips/Kconfig                    |    1 +
 arch/mips/include/asm/perf_event.h   |   12 +-----------
 arch/mips/kernel/perf_event_mipsxx.c |    2 +-
 3 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 67a2fa2..c44c38d 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -4,6 +4,7 @@ config MIPS
 	select HAVE_GENERIC_DMA_COHERENT
 	select HAVE_IDE
 	select HAVE_OPROFILE
+	select HAVE_IRQ_WORK
 	select HAVE_PERF_EVENTS
 	select PERF_USE_VMALLOC
 	select HAVE_ARCH_KGDB
diff --git a/arch/mips/include/asm/perf_event.h b/arch/mips/include/asm/perf_event.h
index e00007c..d0c7749 100644
--- a/arch/mips/include/asm/perf_event.h
+++ b/arch/mips/include/asm/perf_event.h
@@ -11,15 +11,5 @@
 
 #ifndef __MIPS_PERF_EVENT_H__
 #define __MIPS_PERF_EVENT_H__
-
-/*
- * MIPS performance counters do not raise NMI upon overflow, a regular
- * interrupt will be signaled. Hence we can do the pending perf event
- * work at the tail of the irq handler.
- */
-static inline void
-set_perf_event_pending(void)
-{
-}
-
+/* Leave it empty here. The file is required by linux/perf_event.h */
 #endif /* __MIPS_PERF_EVENT_H__ */
diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
index 5c7c6fc..fa00edc 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -696,7 +696,7 @@ static int mipsxx_pmu_handle_shared_irq(void)
 	 * interrupt, not NMI.
 	 */
 	if (handled == IRQ_HANDLED)
-		perf_event_do_pending();
+		irq_work_run();
 
 #ifdef CONFIG_MIPS_MT_SMP
 	read_unlock(&pmuint_rwlock);
-- 
1.7.1

Re: [PATCH 1/5] MIPS/Perf-events: Work with irq_work

[Peter Zijlstra]

On Thu, 2010-11-18 at 14:56 +0800, Deng-Cheng Zhu wrote:
> This is the MIPS part of the following commit by Peter Zijlstra:
> 
> e360adbe29241a0194e10e20595360dd7b98a2b3
> 	irq_work: Add generic hardirq context callbacks

Looks ok,

Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
> ---
>  arch/mips/Kconfig                    |    1 +
>  arch/mips/include/asm/perf_event.h   |   12 +-----------
>  arch/mips/kernel/perf_event_mipsxx.c |    2 +-
>  3 files changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 67a2fa2..c44c38d 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -4,6 +4,7 @@ config MIPS
>  	select HAVE_GENERIC_DMA_COHERENT
>  	select HAVE_IDE
>  	select HAVE_OPROFILE
> +	select HAVE_IRQ_WORK
>  	select HAVE_PERF_EVENTS
>  	select PERF_USE_VMALLOC
>  	select HAVE_ARCH_KGDB
> diff --git a/arch/mips/include/asm/perf_event.h b/arch/mips/include/asm/perf_event.h
> index e00007c..d0c7749 100644
> --- a/arch/mips/include/asm/perf_event.h
> +++ b/arch/mips/include/asm/perf_event.h
> @@ -11,15 +11,5 @@
>  
>  #ifndef __MIPS_PERF_EVENT_H__
>  #define __MIPS_PERF_EVENT_H__
> -
> -/*
> - * MIPS performance counters do not raise NMI upon overflow, a regular
> - * interrupt will be signaled. Hence we can do the pending perf event
> - * work at the tail of the irq handler.
> - */
> -static inline void
> -set_perf_event_pending(void)
> -{
> -}
> -
> +/* Leave it empty here. The file is required by linux/perf_event.h */
>  #endif /* __MIPS_PERF_EVENT_H__ */
> diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
> index 5c7c6fc..fa00edc 100644
> --- a/arch/mips/kernel/perf_event_mipsxx.c
> +++ b/arch/mips/kernel/perf_event_mipsxx.c
> @@ -696,7 +696,7 @@ static int mipsxx_pmu_handle_shared_irq(void)
>  	 * interrupt, not NMI.
>  	 */
>  	if (handled == IRQ_HANDLED)
> -		perf_event_do_pending();
> +		irq_work_run();
>  
>  #ifdef CONFIG_MIPS_MT_SMP
>  	read_unlock(&pmuint_rwlock);

[PATCH 2/5] MIPS/Perf-events: Work with the new PMU interface

[Deng-Cheng Zhu]

This is the MIPS part of the following commits by Peter Zijlstra:

a4eaf7f14675cb512d69f0c928055e73d0c6d252
	perf: Rework the PMU methods
33696fc0d141bbbcb12f75b69608ea83282e3117
	perf: Per PMU disable
24cd7f54a0d47e1d5b3de29e2456bfbd2d8447b7
	perf: Reduce perf_disable() usage
b0a873ebbf87bf38bf70b5e39a7cadc96099fa13
	perf: Register PMU implementations
51b0fe39549a04858001922919ab355dee9bdfcf
	perf: Deconstify struct pmu

Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
---
 arch/mips/kernel/perf_event.c        |  275 +++++++++++++++++++---------------
 arch/mips/kernel/perf_event_mipsxx.c |    2 +
 2 files changed, 158 insertions(+), 119 deletions(-)

diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
index 2b7f3f7..1ee44a3 100644
--- a/arch/mips/kernel/perf_event.c
+++ b/arch/mips/kernel/perf_event.c
@@ -161,41 +161,6 @@ mipspmu_event_set_period(struct perf_event *event,
 	return ret;
 }
 
-static int mipspmu_enable(struct perf_event *event)
-{
-	struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
-	struct hw_perf_event *hwc = &event->hw;
-	int idx;
-	int err = 0;
-
-	/* To look for a free counter for this event. */
-	idx = mipspmu->alloc_counter(cpuc, hwc);
-	if (idx < 0) {
-		err = idx;
-		goto out;
-	}
-
-	/*
-	 * If there is an event in the counter we are going to use then
-	 * make sure it is disabled.
-	 */
-	event->hw.idx = idx;
-	mipspmu->disable_event(idx);
-	cpuc->events[idx] = event;
-
-	/* Set the period for the event. */
-	mipspmu_event_set_period(event, hwc, idx);
-
-	/* Enable the event. */
-	mipspmu->enable_event(hwc, idx);
-
-	/* Propagate our changes to the userspace mapping. */
-	perf_event_update_userpage(event);
-
-out:
-	return err;
-}
-
 static void mipspmu_event_update(struct perf_event *event,
 			struct hw_perf_event *hwc,
 			int idx)
@@ -231,32 +196,90 @@ again:
 	return;
 }
 
-static void mipspmu_disable(struct perf_event *event)
+static void mipspmu_start(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (!mipspmu)
+		return;
+
+	if (flags & PERF_EF_RELOAD)
+		WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
+
+	hwc->state = 0;
+
+	/* Set the period for the event. */
+	mipspmu_event_set_period(event, hwc, hwc->idx);
+
+	/* Enable the event. */
+	mipspmu->enable_event(hwc, hwc->idx);
+}
+
+static void mipspmu_stop(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (!mipspmu)
+		return;
+
+	if (!(hwc->state & PERF_HES_STOPPED)) {
+		/* We are working on a local event. */
+		mipspmu->disable_event(hwc->idx);
+		barrier();
+		mipspmu_event_update(event, hwc, hwc->idx);
+		hwc->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
+	}
+}
+
+static int mipspmu_add(struct perf_event *event, int flags)
 {
 	struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
 	struct hw_perf_event *hwc = &event->hw;
-	int idx = hwc->idx;
+	int idx;
+	int err = 0;
 
+	perf_pmu_disable(event->pmu);
 
-	WARN_ON(idx < 0 || idx >= mipspmu->num_counters);
+	/* To look for a free counter for this event. */
+	idx = mipspmu->alloc_counter(cpuc, hwc);
+	if (idx < 0) {
+		err = idx;
+		goto out;
+	}
 
-	/* We are working on a local event. */
+	/*
+	 * If there is an event in the counter we are going to use then
+	 * make sure it is disabled.
+	 */
+	event->hw.idx = idx;
 	mipspmu->disable_event(idx);
+	cpuc->events[idx] = event;
 
-	barrier();
-
-	mipspmu_event_update(event, hwc, idx);
-	cpuc->events[idx] = NULL;
-	clear_bit(idx, cpuc->used_mask);
+	hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
+	if (flags & PERF_EF_START)
+		mipspmu_start(event, PERF_EF_RELOAD);
 
+	/* Propagate our changes to the userspace mapping. */
 	perf_event_update_userpage(event);
+
+out:
+	perf_pmu_enable(event->pmu);
+	return err;
 }
 
-static void mipspmu_unthrottle(struct perf_event *event)
+static void mipspmu_del(struct perf_event *event, int flags)
 {
+	struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
 	struct hw_perf_event *hwc = &event->hw;
+	int idx = hwc->idx;
 
-	mipspmu->enable_event(hwc, hwc->idx);
+	WARN_ON(idx < 0 || idx >= mipspmu->num_counters);
+
+	mipspmu_stop(event, PERF_EF_UPDATE);
+	cpuc->events[idx] = NULL;
+	clear_bit(idx, cpuc->used_mask);
+
+	perf_event_update_userpage(event);
 }
 
 static void mipspmu_read(struct perf_event *event)
@@ -270,12 +293,17 @@ static void mipspmu_read(struct perf_event *event)
 	mipspmu_event_update(event, hwc, hwc->idx);
 }
 
-static struct pmu pmu = {
-	.enable		= mipspmu_enable,
-	.disable	= mipspmu_disable,
-	.unthrottle	= mipspmu_unthrottle,
-	.read		= mipspmu_read,
-};
+static void mipspmu_enable(struct pmu *pmu)
+{
+	if (mipspmu)
+		mipspmu->start();
+}
+
+static void mipspmu_disable(struct pmu *pmu)
+{
+	if (mipspmu)
+		mipspmu->stop();
+}
 
 static atomic_t active_events = ATOMIC_INIT(0);
 static DEFINE_MUTEX(pmu_reserve_mutex);
@@ -318,6 +346,82 @@ static void mipspmu_free_irq(void)
 		perf_irq = save_perf_irq;
 }
 
+/*
+ * mipsxx/rm9000/loongson2 have different performance counters, they have
+ * specific low-level init routines.
+ */
+static void reset_counters(void *arg);
+static int __hw_perf_event_init(struct perf_event *event);
+
+static void hw_perf_event_destroy(struct perf_event *event)
+{
+	if (atomic_dec_and_mutex_lock(&active_events,
+				&pmu_reserve_mutex)) {
+		/*
+		 * We must not call the destroy function with interrupts
+		 * disabled.
+		 */
+		on_each_cpu(reset_counters,
+			(void *)(long)mipspmu->num_counters, 1);
+		mipspmu_free_irq();
+		mutex_unlock(&pmu_reserve_mutex);
+	}
+}
+
+static int mipspmu_event_init(struct perf_event *event)
+{
+	int err = 0;
+
+	switch (event->attr.type) {
+	case PERF_TYPE_RAW:
+	case PERF_TYPE_HARDWARE:
+	case PERF_TYPE_HW_CACHE:
+		break;
+
+	default:
+		return -ENOENT;
+	}
+
+	if (!mipspmu || event->cpu >= nr_cpumask_bits ||
+		(event->cpu >= 0 && !cpu_online(event->cpu)))
+		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();
+
+		if (!err)
+			atomic_inc(&active_events);
+		mutex_unlock(&pmu_reserve_mutex);
+	}
+
+	if (err)
+		return err;
+
+	err = __hw_perf_event_init(event);
+	if (err)
+		hw_perf_event_destroy(event);
+
+	return err;
+}
+
+static struct pmu pmu = {
+	.pmu_enable	= mipspmu_enable,
+	.pmu_disable	= mipspmu_disable,
+	.event_init	= mipspmu_event_init,
+	.add		= mipspmu_add,
+	.del		= mipspmu_del,
+	.start		= mipspmu_start,
+	.stop		= mipspmu_stop,
+	.read		= mipspmu_read,
+};
+
 static inline unsigned int
 mipspmu_perf_event_encode(const struct mips_perf_event *pev)
 {
@@ -409,73 +513,6 @@ static int validate_group(struct perf_event *event)
 	return 0;
 }
 
-/*
- * mipsxx/rm9000/loongson2 have different performance counters, they have
- * specific low-level init routines.
- */
-static void reset_counters(void *arg);
-static int __hw_perf_event_init(struct perf_event *event);
-
-static void hw_perf_event_destroy(struct perf_event *event)
-{
-	if (atomic_dec_and_mutex_lock(&active_events,
-				&pmu_reserve_mutex)) {
-		/*
-		 * We must not call the destroy function with interrupts
-		 * disabled.
-		 */
-		on_each_cpu(reset_counters,
-			(void *)(long)mipspmu->num_counters, 1);
-		mipspmu_free_irq();
-		mutex_unlock(&pmu_reserve_mutex);
-	}
-}
-
-const struct pmu *hw_perf_event_init(struct perf_event *event)
-{
-	int err = 0;
-
-	if (!mipspmu || event->cpu >= nr_cpumask_bits ||
-		(event->cpu >= 0 && !cpu_online(event->cpu)))
-		return ERR_PTR(-ENODEV);
-
-	if (!atomic_inc_not_zero(&active_events)) {
-		if (atomic_read(&active_events) > MIPS_MAX_HWEVENTS) {
-			atomic_dec(&active_events);
-			return ERR_PTR(-ENOSPC);
-		}
-
-		mutex_lock(&pmu_reserve_mutex);
-		if (atomic_read(&active_events) == 0)
-			err = mipspmu_get_irq();
-
-		if (!err)
-			atomic_inc(&active_events);
-		mutex_unlock(&pmu_reserve_mutex);
-	}
-
-	if (err)
-		return ERR_PTR(err);
-
-	err = __hw_perf_event_init(event);
-	if (err)
-		hw_perf_event_destroy(event);
-
-	return err ? ERR_PTR(err) : &pmu;
-}
-
-void hw_perf_enable(void)
-{
-	if (mipspmu)
-		mipspmu->start();
-}
-
-void hw_perf_disable(void)
-{
-	if (mipspmu)
-		mipspmu->stop();
-}
-
 /* This is needed by specific irq handlers in perf_event_*.c */
 static void
 handle_associated_event(struct cpu_hw_events *cpuc,
diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
index fa00edc..c9406d6 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -1045,6 +1045,8 @@ init_hw_perf_events(void)
 			"CPU, irq %d%s\n", mipspmu->name, counters, irq,
 			irq < 0 ? " (share with timer interrupt)" : "");
 
+	perf_pmu_register(&pmu);
+
 	return 0;
 }
 arch_initcall(init_hw_perf_events);
-- 
1.7.1

Re: [PATCH 2/5] MIPS/Perf-events: Work with the new PMU interface

[Peter Zijlstra]

On Thu, 2010-11-18 at 14:56 +0800, Deng-Cheng Zhu wrote:
> 
> This is the MIPS part of the following commits by Peter Zijlstra:
> 
> a4eaf7f14675cb512d69f0c928055e73d0c6d252
>         perf: Rework the PMU methods
> 33696fc0d141bbbcb12f75b69608ea83282e3117
>         perf: Per PMU disable
> 24cd7f54a0d47e1d5b3de29e2456bfbd2d8447b7
>         perf: Reduce perf_disable() usage
> b0a873ebbf87bf38bf70b5e39a7cadc96099fa13
>         perf: Register PMU implementations
> 51b0fe39549a04858001922919ab355dee9bdfcf
>         perf: Deconstify struct pmu
> 
> 
I'll not ACK this as I've no clue how the MIPS PMU(s) work, but the
shape looks ok :-)

[PATCH 3/5] MIPS/Perf-events: Check event state in validate_event()

[Deng-Cheng Zhu]

Ignore events that are not for this PMU or are in off/error state.

Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
---
 arch/mips/kernel/perf_event.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
index 1ee44a3..9c6442a 100644
--- a/arch/mips/kernel/perf_event.c
+++ b/arch/mips/kernel/perf_event.c
@@ -486,7 +486,7 @@ static int validate_event(struct cpu_hw_events *cpuc,
 {
 	struct hw_perf_event fake_hwc = event->hw;
 
-	if (event->pmu && event->pmu != &pmu)
+	if (event->pmu != &pmu || event->state <= PERF_EVENT_STATE_OFF)
 		return 0;
 
 	return mipspmu->alloc_counter(cpuc, &fake_hwc) >= 0;
-- 
1.7.1

Re: [PATCH 3/5] MIPS/Perf-events: Check event state in validate_event()

[Will Deacon]

Hi Deng-Cheng,

On Thu, 2010-11-18 at 06:56 +0000, Deng-Cheng Zhu wrote:
> Ignore events that are not for this PMU or are in off/error state.
> 
Sorry I didn't see this before, thanks for pointing out that you
had included it for MIPS.

> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
> ---
>  arch/mips/kernel/perf_event.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
> index 1ee44a3..9c6442a 100644
> --- a/arch/mips/kernel/perf_event.c
> +++ b/arch/mips/kernel/perf_event.c
> @@ -486,7 +486,7 @@ static int validate_event(struct cpu_hw_events *cpuc,
>  {
>         struct hw_perf_event fake_hwc = event->hw;
> 
> -       if (event->pmu && event->pmu != &pmu)
> +       if (event->pmu != &pmu || event->state <= PERF_EVENT_STATE_OFF)
>                 return 0;
> 
>         return mipspmu->alloc_counter(cpuc, &fake_hwc) >= 0;

So this is the opposite of what we're doing on ARM. Our
approach is to ignore events that are OFF (or in the ERROR
state) or that belong to a different PMU. We do this by
allowing them to *pass* validation (i.e. by returning 1 above).
This means that we won't unconditionally fail a mixed event group.

x86 does something similar in the collect_events function.

Will

Re: [PATCH 3/5] MIPS/Perf-events: Check event state in validate_event()

[Peter Zijlstra]

On Fri, 2010-11-19 at 09:43 +0000, Will Deacon wrote:
> Hi Deng-Cheng,
> 
> On Thu, 2010-11-18 at 06:56 +0000, Deng-Cheng Zhu wrote:
> > Ignore events that are not for this PMU or are in off/error state.
> > 
> Sorry I didn't see this before, thanks for pointing out that you
> had included it for MIPS.
> 
> > Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
> > ---
> >  arch/mips/kernel/perf_event.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
> > index 1ee44a3..9c6442a 100644
> > --- a/arch/mips/kernel/perf_event.c
> > +++ b/arch/mips/kernel/perf_event.c
> > @@ -486,7 +486,7 @@ static int validate_event(struct cpu_hw_events *cpuc,
> >  {
> >         struct hw_perf_event fake_hwc = event->hw;
> > 
> > -       if (event->pmu && event->pmu != &pmu)
> > +       if (event->pmu != &pmu || event->state <= PERF_EVENT_STATE_OFF)
> >                 return 0;
> > 
> >         return mipspmu->alloc_counter(cpuc, &fake_hwc) >= 0;
> 
> So this is the opposite of what we're doing on ARM. Our
> approach is to ignore events that are OFF (or in the ERROR
> state) or that belong to a different PMU. We do this by
> allowing them to *pass* validation (i.e. by returning 1 above).
> This means that we won't unconditionally fail a mixed event group.
> 
> x86 does something similar in the collect_events function.

Right, note that the generic code only allows mixing with software
events, so simply accepting them is ok as software events give the
guarantee they're always schedulable.

Re: [PATCH 3/5] MIPS/Perf-events: Check event state in validate_event()

[Will Deacon]

On Fri, 2010-11-19 at 11:27 +0000, Peter Zijlstra wrote:
> > So this is the opposite of what we're doing on ARM. Our
> > approach is to ignore events that are OFF (or in the ERROR
> > state) or that belong to a different PMU. We do this by
> > allowing them to *pass* validation (i.e. by returning 1 above).
> > This means that we won't unconditionally fail a mixed event group.
> >
> > x86 does something similar in the collect_events function.
> 
> Right, note that the generic code only allows mixing with software
> events, so simply accepting them is ok as software events give the
> guarantee they're always schedulable.
> 
> 

Ok. Initially it was software events that I had in mind, but does
this constraint prevent you from grouping CPU events with events
for other PMUs within the system? For external L2 cache controllers
with their own PMUs, it would be desirable to group some L2 events
with L1 events on a different PMU.

If each PMU can validate its own events and ignore others then it
sounds like it should be straightforward...

Will

Re: [PATCH 3/5] MIPS/Perf-events: Check event state in validate_event()

[Peter Zijlstra]

On Fri, 2010-11-19 at 12:03 +0000, Will Deacon wrote:
> On Fri, 2010-11-19 at 11:27 +0000, Peter Zijlstra wrote:
> > > So this is the opposite of what we're doing on ARM. Our
> > > approach is to ignore events that are OFF (or in the ERROR
> > > state) or that belong to a different PMU. We do this by
> > > allowing them to *pass* validation (i.e. by returning 1 above).
> > > This means that we won't unconditionally fail a mixed event group.
> > >
> > > x86 does something similar in the collect_events function.
> > 
> > Right, note that the generic code only allows mixing with software
> > events, so simply accepting them is ok as software events give the
> > guarantee they're always schedulable.
> > 
> > 
> 
> Ok. Initially it was software events that I had in mind, but does
> this constraint prevent you from grouping CPU events with events
> for other PMUs within the system? For external L2 cache controllers
> with their own PMUs, it would be desirable to group some L2 events
> with L1 events on a different PMU.
> 
> If each PMU can validate its own events and ignore others then it
> sounds like it should be straightforward...

Getting them all scheduled on the hardware at the same time will be
'interesting'.. therefore we currently don't allow for this. The current
code would pretty much result in such a group being starved if there
were other contenders.

Re: [PATCH 3/5] MIPS/Perf-events: Check event state in validate_event()

[Deng-Cheng Zhu]

Ah, I see. Thanks for your explanation.

But by doing this, I think we need to modify validate_group() as well.
Consider a group which has all its events either not for this PMU or in
OFF/Error state. Then the last validate_event() in validate_group() does
not work. Right? So, how about the following:

diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
index 1ee44a3..4010bc0 100644
--- a/arch/mips/kernel/perf_event.c
+++ b/arch/mips/kernel/perf_event.c
@@ -486,8 +486,8 @@ static int validate_event(struct cpu_hw_events *cpuc,
 {
 	struct hw_perf_event fake_hwc = event->hw;

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

 	return mipspmu->alloc_counter(cpuc, &fake_hwc) >= 0;
 }
@@ -496,6 +496,7 @@ static int validate_group(struct perf_event *event)
 {
 	struct perf_event *sibling, *leader = event->group_leader;
 	struct cpu_hw_events fake_cpuc;
+	struct hw_perf_event fake_hwc = event->hw;

 	memset(&fake_cpuc, 0, sizeof(fake_cpuc));

@@ -507,10 +508,12 @@ static int validate_group(struct perf_event *event)
 			return -ENOSPC;
 	}

-	if (!validate_event(&fake_cpuc, event))
+	if (event->pmu != &pmu || event->state <= PERF_EVENT_STATE_OFF)
+		return -EINVAL;
+	else if (mipspmu->alloc_counter(&fake_cpuc, &fake_hwc) < 0)
 		return -ENOSPC;
-
-	return 0;
+	else
+		return 0;
 }

 /* This is needed by specific irq handlers in perf_event_*.c */


Thanks,

Deng-Cheng


2010/11/19 Will Deacon <will.deacon@arm.com>:
> Hi Deng-Cheng,
>
> On Thu, 2010-11-18 at 06:56 +0000, Deng-Cheng Zhu wrote:
>> Ignore events that are not for this PMU or are in off/error state.
>>
> Sorry I didn't see this before, thanks for pointing out that you
> had included it for MIPS.
>
>> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
>> ---
>>  arch/mips/kernel/perf_event.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
>> index 1ee44a3..9c6442a 100644
>> --- a/arch/mips/kernel/perf_event.c
>> +++ b/arch/mips/kernel/perf_event.c
>> @@ -486,7 +486,7 @@ static int validate_event(struct cpu_hw_events *cpuc,
>>  {
>>         struct hw_perf_event fake_hwc = event->hw;
>>
>> -       if (event->pmu && event->pmu != &pmu)
>> +       if (event->pmu != &pmu || event->state <= PERF_EVENT_STATE_OFF)
>>                 return 0;
>>
>>         return mipspmu->alloc_counter(cpuc, &fake_hwc) >= 0;
>
> So this is the opposite of what we're doing on ARM. Our
> approach is to ignore events that are OFF (or in the ERROR
> state) or that belong to a different PMU. We do this by
> allowing them to *pass* validation (i.e. by returning 1 above).
> This means that we won't unconditionally fail a mixed event group.
>
> x86 does something similar in the collect_events function.
>
> Will
>
> --
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.
>
>

Re: [PATCH 3/5] MIPS/Perf-events: Check event state in validate_event()

[Will Deacon]

On Fri, 2010-11-19 at 11:30 +0000, Deng-Cheng Zhu wrote:
> Ah, I see. Thanks for your explanation.
> 
> But by doing this, I think we need to modify validate_group() as well.
> Consider a group which has all its events either not for this PMU or in
> OFF/Error state. Then the last validate_event() in validate_group() does
> not work. Right? So, how about the following:

[...]

If none of the events are for this PMU, then our validate_group()
won't be called. If all the events are OFF/ERROR then I don't see
what's wrong with passing the validation.

Will

[PATCH 4/5] MIPS/Perf-events: Work with the new callchain interface

[Deng-Cheng Zhu]

This is the MIPS part of the following commits by Frederic Weisbecker:

f72c1a931e311bb7780fee19e41a89ac42cab50e
	perf: Factorize callchain context handling
56962b4449af34070bb1994621ef4f0265eed4d8
	perf: Generalize some arch callchain code
70791ce9ba68a5921c9905ef05d23f62a90bc10c
	perf: Generalize callchain_store()
c1a65932fd7216fdc9a0db8bbffe1d47842f862c
	perf: Drop unappropriate tests on arch callchains

Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
---
 arch/mips/kernel/perf_event.c |   63 ++++-------------------------------------
 1 files changed, 6 insertions(+), 57 deletions(-)

diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
index 9c6442a..345232a 100644
--- a/arch/mips/kernel/perf_event.c
+++ b/arch/mips/kernel/perf_event.c
@@ -533,21 +533,13 @@ handle_associated_event(struct cpu_hw_events *cpuc,
 #include "perf_event_mipsxx.c"
 
 /* Callchain handling code. */
-static inline void
-callchain_store(struct perf_callchain_entry *entry,
-		u64 ip)
-{
-	if (entry->nr < PERF_MAX_STACK_DEPTH)
-		entry->ip[entry->nr++] = ip;
-}
 
 /*
  * Leave userspace callchain empty for now. When we find a way to trace
  * the user stack callchains, we add here.
  */
-static void
-perf_callchain_user(struct pt_regs *regs,
-		    struct perf_callchain_entry *entry)
+void perf_callchain_user(struct perf_callchain_entry *entry,
+		    struct pt_regs *regs)
 {
 }
 
@@ -560,23 +552,21 @@ static void save_raw_perf_callchain(struct perf_callchain_entry *entry,
 	while (!kstack_end(sp)) {
 		addr = *sp++;
 		if (__kernel_text_address(addr)) {
-			callchain_store(entry, addr);
+			perf_callchain_store(entry, addr);
 			if (entry->nr >= PERF_MAX_STACK_DEPTH)
 				break;
 		}
 	}
 }
 
-static void
-perf_callchain_kernel(struct pt_regs *regs,
-		      struct perf_callchain_entry *entry)
+void perf_callchain_kernel(struct perf_callchain_entry *entry,
+		      struct pt_regs *regs)
 {
 	unsigned long sp = regs->regs[29];
 #ifdef CONFIG_KALLSYMS
 	unsigned long ra = regs->regs[31];
 	unsigned long pc = regs->cp0_epc;
 
-	callchain_store(entry, PERF_CONTEXT_KERNEL);
 	if (raw_show_trace || !__kernel_text_address(pc)) {
 		unsigned long stack_page =
 			(unsigned long)task_stack_page(current);
@@ -586,53 +576,12 @@ perf_callchain_kernel(struct pt_regs *regs,
 		return;
 	}
 	do {
-		callchain_store(entry, pc);
+		perf_callchain_store(entry, pc);
 		if (entry->nr >= PERF_MAX_STACK_DEPTH)
 			break;
 		pc = unwind_stack(current, &sp, pc, &ra);
 	} while (pc);
 #else
-	callchain_store(entry, PERF_CONTEXT_KERNEL);
 	save_raw_perf_callchain(entry, sp);
 #endif
 }
-
-static void
-perf_do_callchain(struct pt_regs *regs,
-		  struct perf_callchain_entry *entry)
-{
-	int is_user;
-
-	if (!regs)
-		return;
-
-	is_user = user_mode(regs);
-
-	if (!current || !current->pid)
-		return;
-
-	if (is_user && current->state != TASK_RUNNING)
-		return;
-
-	if (!is_user) {
-		perf_callchain_kernel(regs, entry);
-		if (current->mm)
-			regs = task_pt_regs(current);
-		else
-			regs = NULL;
-	}
-	if (regs)
-		perf_callchain_user(regs, entry);
-}
-
-static DEFINE_PER_CPU(struct perf_callchain_entry, pmc_irq_entry);
-
-struct perf_callchain_entry *
-perf_callchain(struct pt_regs *regs)
-{
-	struct perf_callchain_entry *entry = &__get_cpu_var(pmc_irq_entry);
-
-	entry->nr = 0;
-	perf_do_callchain(regs, entry);
-	return entry;
-}
-- 
1.7.1

Re: [PATCH 4/5] MIPS/Perf-events: Work with the new callchain interface

[Frederic Weisbecker]

On Thu, Nov 18, 2010 at 02:56:40PM +0800, Deng-Cheng Zhu wrote:
> This is the MIPS part of the following commits by Frederic Weisbecker:
> 
> f72c1a931e311bb7780fee19e41a89ac42cab50e
> 	perf: Factorize callchain context handling
> 56962b4449af34070bb1994621ef4f0265eed4d8
> 	perf: Generalize some arch callchain code
> 70791ce9ba68a5921c9905ef05d23f62a90bc10c
> 	perf: Generalize callchain_store()
> c1a65932fd7216fdc9a0db8bbffe1d47842f862c
> 	perf: Drop unappropriate tests on arch callchains
> 
> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
> ---


Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

Why did I miss this arch? I did a grep on HAVE_PERF_EVENT or something,
may be it hadn't it at that time?

Thanks!

Re: [PATCH 4/5] MIPS/Perf-events: Work with the new callchain interface

[Deng-Cheng Zhu]

Yes, MIPS Perf-events got added in recently and missed some important
commits.


Thanks!

Deng-Cheng


2010/11/19 Frederic Weisbecker <fweisbec@gmail.com>:
> On Thu, Nov 18, 2010 at 02:56:40PM +0800, Deng-Cheng Zhu wrote:
>> This is the MIPS part of the following commits by Frederic Weisbecker:
>>
>> f72c1a931e311bb7780fee19e41a89ac42cab50e
>>       perf: Factorize callchain context handling
>> 56962b4449af34070bb1994621ef4f0265eed4d8
>>       perf: Generalize some arch callchain code
>> 70791ce9ba68a5921c9905ef05d23f62a90bc10c
>>       perf: Generalize callchain_store()
>> c1a65932fd7216fdc9a0db8bbffe1d47842f862c
>>       perf: Drop unappropriate tests on arch callchains
>>
>> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
>> ---
>
>
> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
>
> Why did I miss this arch? I did a grep on HAVE_PERF_EVENT or something,
> may be it hadn't it at that time?
>
> Thanks!
>
>

[PATCH 5/5] MIPS/Perf-events: Use unsigned delta for right shift in event update

[Deng-Cheng Zhu]

Leverage the commit for ARM by Will Deacon:

446a5a8b1eb91a6990e5c8fe29f14e7a95b69132
	ARM: 6205/1: perf: ensure counter delta is treated as unsigned

Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
---
 arch/mips/kernel/perf_event.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
index 345232a..0f1cdf5 100644
--- a/arch/mips/kernel/perf_event.c
+++ b/arch/mips/kernel/perf_event.c
@@ -169,7 +169,7 @@ static void mipspmu_event_update(struct perf_event *event,
 	unsigned long flags;
 	int shift = 64 - TOTAL_BITS;
 	s64 prev_raw_count, new_raw_count;
-	s64 delta;
+	u64 delta;
 
 again:
 	prev_raw_count = local64_read(&hwc->prev_count);
-- 
1.7.1

Re: [PATCH 5/5] MIPS/Perf-events: Use unsigned delta for right shift in event update

[Will Deacon]

On Thu, 2010-11-18 at 06:56 +0000, Deng-Cheng Zhu wrote:
> Leverage the commit for ARM by Will Deacon:
> 
> 446a5a8b1eb91a6990e5c8fe29f14e7a95b69132
>         ARM: 6205/1: perf: ensure counter delta is treated as unsigned
> 
> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
> ---
>  arch/mips/kernel/perf_event.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
> index 345232a..0f1cdf5 100644
> --- a/arch/mips/kernel/perf_event.c
> +++ b/arch/mips/kernel/perf_event.c
> @@ -169,7 +169,7 @@ static void mipspmu_event_update(struct perf_event *event,
>         unsigned long flags;
>         int shift = 64 - TOTAL_BITS;
>         s64 prev_raw_count, new_raw_count;
> -       s64 delta;
> +       u64 delta;
> 
>  again:
>         prev_raw_count = local64_read(&hwc->prev_count);
> --
> 1.7.1

Acked-by: Will Deacon <will.deacon@arm.com>

You might also want to look at commit 65b4711f if you based
the MIPS port on the old ARM code.

Thanks,

Will

Re: [PATCH 5/5] MIPS/Perf-events: Use unsigned delta for right shift in event update

[Deng-Cheng Zhu]

Thanks. The commit you pointed out was also in this patch set (#3).

But I think the return value should stick to 0, not 1. Something out of my
consideration?


Deng-Cheng


2010/11/18 Will Deacon <will.deacon@arm.com>:
> On Thu, 2010-11-18 at 06:56 +0000, Deng-Cheng Zhu wrote:
>> Leverage the commit for ARM by Will Deacon:
>>
>> 446a5a8b1eb91a6990e5c8fe29f14e7a95b69132
>>         ARM: 6205/1: perf: ensure counter delta is treated as unsigned
>>
>> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@gmail.com>
>> ---
>>  arch/mips/kernel/perf_event.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/mips/kernel/perf_event.c b/arch/mips/kernel/perf_event.c
>> index 345232a..0f1cdf5 100644
>> --- a/arch/mips/kernel/perf_event.c
>> +++ b/arch/mips/kernel/perf_event.c
>> @@ -169,7 +169,7 @@ static void mipspmu_event_update(struct perf_event *event,
>>         unsigned long flags;
>>         int shift = 64 - TOTAL_BITS;
>>         s64 prev_raw_count, new_raw_count;
>> -       s64 delta;
>> +       u64 delta;
>>
>>  again:
>>         prev_raw_count = local64_read(&hwc->prev_count);
>> --
>> 1.7.1
>
> Acked-by: Will Deacon <will.deacon@arm.com>
>
> You might also want to look at commit 65b4711f if you based
> the MIPS port on the old ARM code.
>
> Thanks,
>
> Will
>
> --
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.
>
>