LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] armpmu: broadcast overflow irq on multi-core system having one muxed SPI for PMU.
@ 2018-05-10  8:36 Hoeun Ryu
  2018-05-10 10:21 ` Mark Rutland
  0 siblings, 1 reply; 5+ messages in thread
From: Hoeun Ryu @ 2018-05-10  8:36 UTC (permalink / raw)
  To: Will Deacon, Mark Rutland; +Cc: Hoeun Ryu, linux-arm-kernel, linux-kernel

From: Hoeun Ryu <hoeun.ryu@lge.com>

 On some SoCs like i.MX6DL/QL have only one muxed SPI for multi-core system.
On the systems, a CPU can be interrupted by overflow irq but it is possible that
the overflow actually occurs on another CPU.
 This patch broadcasts the irq using smp_call_function() so that other CPUs can
check and handle their overflows by themselves when a overflow doesn't actually
occur on the interrupted CPU.

 Local irq is enabled and preemption is disabled temporarily to call
smp_call_function_many() in armpmu_dispatch_irq() as the smp_call_function_many()
doesn't allow to be called with irq-disabled.

 The callback for smp_call_function_many() is __armpmu_handle_irq() and the
function calls armpmu->handle_irq() with an invalid irq_num because
smp_call_func_t has only one parameter and armpmu pointer is handed over by the
pointer. It can be a problem if irq_num parameter is used by handlers but no
handler uses the irq parameter for now. We could have another approach removing
irq_num argument itself in handle_irq() function.

Signed-off-by: Hoeun Ryu <hoeun.ryu@lge.com>
---
 drivers/perf/arm_pmu.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 3 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index 1a0d340..3d65e44 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -322,6 +322,29 @@ validate_group(struct perf_event *event)
 	return 0;
 }
 
+static void __armpmu_handle_irq(void *dev)
+{
+	struct arm_pmu *armpmu;
+	u64 start_clock, finish_clock;
+	irqreturn_t ret;
+
+	armpmu = *(void **)dev;
+	start_clock = sched_clock();
+	/*
+	 * irq_num should not be used by the handler, we don't have irq_num for
+	 * the first place. There is no handler using the irq_num argument for now.
+	 * smp_call_func_t has one function argument and irq number cannot be handed
+	 * over to this callback because we need dev pointer here.
+	 * If you need valid irq_num, you need to declare a wrapper struct having
+	 * irq_num and dev pointer.
+	 */
+	ret = armpmu->handle_irq(-1, armpmu);
+	if (ret == IRQ_HANDLED) {
+		finish_clock = sched_clock();
+		perf_sample_event_took(finish_clock - start_clock);
+	}
+}
+
 static irqreturn_t armpmu_dispatch_irq(int irq, void *dev)
 {
 	struct arm_pmu *armpmu;
@@ -340,9 +363,31 @@ static irqreturn_t armpmu_dispatch_irq(int irq, void *dev)
 
 	start_clock = sched_clock();
 	ret = armpmu->handle_irq(irq, armpmu);
-	finish_clock = sched_clock();
-
-	perf_sample_event_took(finish_clock - start_clock);
+	/*
+	 * The handler just returns with IRQ_NONE when it checks the overflow
+	 * and the overflow doesn't occur on the CPU.
+	 *
+	 * Some SoCs like i.MX6 have one muxed SPI on multi-core system.
+	 * On the systems , the irq should be broadcasted to other CPUs so that the
+	 * CPUs can check their own PMU overflow.
+	 */
+	if (ret == IRQ_HANDLED) {
+		finish_clock = sched_clock();
+		perf_sample_event_took(finish_clock - start_clock);
+	} else if (ret == IRQ_NONE) {
+		struct cpumask mask;
+
+		cpumask_copy(&mask, cpu_online_mask);
+		cpumask_clear_cpu(raw_smp_processor_id(), &mask);
+		if (!cpumask_empty(&mask)) {
+			/* smp_call_function cannot be called with irq disabled */
+			local_irq_enable();
+			preempt_disable();
+			smp_call_function_many(&mask, __armpmu_handle_irq, dev, 0);
+			preempt_enable();
+			local_irq_disable();
+		}
+	}
 	return ret;
 }
 
-- 
2.1.4

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] armpmu: broadcast overflow irq on multi-core system having one muxed SPI for PMU.
  2018-05-10  8:36 [PATCH] armpmu: broadcast overflow irq on multi-core system having one muxed SPI for PMU Hoeun Ryu
@ 2018-05-10 10:21 ` Mark Rutland
  2018-05-10 23:20   ` 류호은
  0 siblings, 1 reply; 5+ messages in thread
From: Mark Rutland @ 2018-05-10 10:21 UTC (permalink / raw)
  To: Hoeun Ryu; +Cc: Will Deacon, Hoeun Ryu, linux-arm-kernel, linux-kernel

Hi,
On Thu, May 10, 2018 at 05:36:17PM +0900, Hoeun Ryu wrote:
> From: Hoeun Ryu <hoeun.ryu@lge.com>
> 
>  On some SoCs like i.MX6DL/QL have only one muxed SPI for multi-core system.
> On the systems, a CPU can be interrupted by overflow irq but it is possible that
> the overflow actually occurs on another CPU.

Muxing the PMU IRQs is a really broken system design, and there's no
good way of supporting it.

What we should do for such systems is:

* Add a flag to the DT to describe that the IRQs are muxed, as this
  cannot be probed.

* Add hrtimer code to periodically update the counters, to avoid
  overflow (e.g. as we do in the l2x0 PMU).

* Reject sampling for such systems, as this cannot be done reliably or
  efficiently.

NAK to broadcasting the IRQ -- there are a number of issues with the
general approach.

We should update the PMU probing code to warn when we have fewer IRQs
than CPUs, and fail gracefully to the above.

[...]

>  static irqreturn_t armpmu_dispatch_irq(int irq, void *dev)
>  {

> +			/* smp_call_function cannot be called with irq disabled */
> +			local_irq_enable();
> +			preempt_disable();
> +			smp_call_function_many(&mask, __armpmu_handle_irq, dev, 0);
> +			preempt_enable();
> +			local_irq_disable();

For many reasons, this sequence is not safe.

It is not safe to enable IRQs in irq handlers. Please never do this.

Thus it's also never safe to call smp_call_function*() in IRQ handlers.

Futher, If you ever encounter a case where you need to avoid preemption
across enabling IRQs, preemption must be disabled *before* enabling
IRQs.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] armpmu: broadcast overflow irq on multi-core system having one muxed SPI for PMU.
  2018-05-10 10:21 ` Mark Rutland
@ 2018-05-10 23:20   ` 류호은
  2018-05-11 10:39     ` Mark Rutland
  0 siblings, 1 reply; 5+ messages in thread
From: 류호은 @ 2018-05-10 23:20 UTC (permalink / raw)
  To: 'Mark Rutland', 'Hoeun Ryu'
  Cc: 'Will Deacon', linux-arm-kernel, linux-kernel

Thank you for the reply.

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> Sent: Thursday, May 10, 2018 7:21 PM
> To: Hoeun Ryu <hoeun.ryu@lge.com.com>
> Cc: Will Deacon <will.deacon@arm.com>; Hoeun Ryu <hoeun.ryu@lge.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] armpmu: broadcast overflow irq on multi-core system
> having one muxed SPI for PMU.
> 
> Hi,
> On Thu, May 10, 2018 at 05:36:17PM +0900, Hoeun Ryu wrote:
> > From: Hoeun Ryu <hoeun.ryu@lge.com>
> >
> >  On some SoCs like i.MX6DL/QL have only one muxed SPI for multi-core
> system.
> > On the systems, a CPU can be interrupted by overflow irq but it is
> > possible that the overflow actually occurs on another CPU.
> 
> Muxing the PMU IRQs is a really broken system design, and there's no good
> way of supporting it.

Yes. I agree with that. I scratched my head when I found the system has one
muxed SPI.

> 
> What we should do for such systems is:
> 
> * Add a flag to the DT to describe that the IRQs are muxed, as this
>   cannot be probed.
> 
> * Add hrtimer code to periodically update the counters, to avoid
>   overflow (e.g. as we do in the l2x0 PMU).
> 
> * Reject sampling for such systems, as this cannot be done reliably or
>   efficiently.
> 
> NAK to broadcasting the IRQ -- there are a number of issues with the
> general approach.
> 

The second solution would be good if sampling is necessary even like those
systems.

Actually I'm working on FIQ available ARM32 system and trying to enable the
hard lockup detector by routing the PMU IRQ to FIQ.
Because of that, I really need the interrupt event if it is a muxed SPI,
beside I also need to make an dedicated IPI FIQ to broadcast the IRQ in
this approach.
What would you do if you were in the same situation ?

> We should update the PMU probing code to warn when we have fewer IRQs than
> CPUs, and fail gracefully to the above.
> 
> [...]
> 
> >  static irqreturn_t armpmu_dispatch_irq(int irq, void *dev)  {
> 
> > +			/* smp_call_function cannot be called with irq
> disabled */
> > +			local_irq_enable();
> > +			preempt_disable();
> > +			smp_call_function_many(&mask, __armpmu_handle_irq,
dev,
> 0);
> > +			preempt_enable();
> > +			local_irq_disable();
> 
> For many reasons, this sequence is not safe.
> 
> It is not safe to enable IRQs in irq handlers. Please never do this.

I was not sure it was safe to enable IRQs in irq handlers.
Thank you for pointing that.

> 
> Thus it's also never safe to call smp_call_function*() in IRQ handlers.

Yes. I found out that it is due to csd lock.

> 
> Futher, If you ever encounter a case where you need to avoid preemption
> across enabling IRQs, preemption must be disabled *before* enabling IRQs.

Ah, OK.
Enabling IRQs can cause scheduling tasks in the end of exception or other
scheduling points, right ?

> 
> Thanks,
> Mark.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] armpmu: broadcast overflow irq on multi-core system having one muxed SPI for PMU.
  2018-05-10 23:20   ` 류호은
@ 2018-05-11 10:39     ` Mark Rutland
  2018-05-14  2:26       ` Hoeun Ryu
  0 siblings, 1 reply; 5+ messages in thread
From: Mark Rutland @ 2018-05-11 10:39 UTC (permalink / raw)
  To: ��ȣ��
  Cc: 'Hoeun Ryu', 'Will Deacon',
	linux-arm-kernel, linux-kernel

On Fri, May 11, 2018 at 08:20:49AM +0900, ��ȣ�� wrote:
> Thank you for the reply.
> 
> > -----Original Message-----
> > From: Mark Rutland [mailto:mark.rutland@arm.com]
> > Sent: Thursday, May 10, 2018 7:21 PM
> > To: Hoeun Ryu <hoeun.ryu@lge.com.com>
> > Cc: Will Deacon <will.deacon@arm.com>; Hoeun Ryu <hoeun.ryu@lge.com>;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] armpmu: broadcast overflow irq on multi-core system
> > having one muxed SPI for PMU.

> > Muxing the PMU IRQs is a really broken system design, and there's no good
> > way of supporting it.

> > What we should do for such systems is:
> > 
> > * Add a flag to the DT to describe that the IRQs are muxed, as this
> >   cannot be probed.
> > 
> > * Add hrtimer code to periodically update the counters, to avoid
> >   overflow (e.g. as we do in the l2x0 PMU).
> > 
> > * Reject sampling for such systems, as this cannot be done reliably or
> >   efficiently.
> > 
> > NAK to broadcasting the IRQ -- there are a number of issues with the
> > general approach.
> 
> The second solution would be good if sampling is necessary even like those
> systems.

Please note that I mean *all* of the above. There would be no sampling
on systems with muxed PMU IRQs, since there's no correlation between
overflow events and the hrtimer interrupts -- the results of sampling
would be misleading.

> Actually I'm working on FIQ available ARM32 system and trying to enable the
> hard lockup detector by routing the PMU IRQ to FIQ.
> Because of that, I really need the interrupt event if it is a muxed SPI,
> beside I also need to make an dedicated IPI FIQ to broadcast the IRQ in
> this approach.
> What would you do if you were in the same situation ?

I don't think that this can work with a muxed IRQ, sorry.

It would be better to use some kind of timer.

[...]

> > Futher, If you ever encounter a case where you need to avoid preemption
> > across enabling IRQs, preemption must be disabled *before* enabling IRQs.
> 
> Ah, OK.
> Enabling IRQs can cause scheduling tasks in the end of exception or other
> scheduling points, right ?

Yes. If an IRQ was taken *between* enabling IRQs and disabling
preemption, preemption may occur as part of the exception return.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] armpmu: broadcast overflow irq on multi-core system having one muxed SPI for PMU.
  2018-05-11 10:39     ` Mark Rutland
@ 2018-05-14  2:26       ` Hoeun Ryu
  0 siblings, 0 replies; 5+ messages in thread
From: Hoeun Ryu @ 2018-05-14  2:26 UTC (permalink / raw)
  To: 'Mark Rutland'
  Cc: 'Hoeun Ryu', 'Will Deacon',
	linux-arm-kernel, linux-kernel

Thank you for the review.
I understand your NACK.

But I'd like to just fix the part of smp_call_function() in the next version.
You can simply ignore it.

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> Sent: Friday, May 11, 2018 7:39 PM
> To: ��ȣ�� <hoeun.ryu@lge.com>
> Cc: 'Hoeun Ryu' <hoeun.ryu@lge.com.com>; 'Will Deacon'
> <will.deacon@arm.com>; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] armpmu: broadcast overflow irq on multi-core system
> having one muxed SPI for PMU.
> 
> On Fri, May 11, 2018 at 08:20:49AM +0900, ��ȣ�� wrote:
> > Thank you for the reply.
> >
> > > -----Original Message-----
> > > From: Mark Rutland [mailto:mark.rutland@arm.com]
> > > Sent: Thursday, May 10, 2018 7:21 PM
> > > To: Hoeun Ryu <hoeun.ryu@lge.com.com>
> > > Cc: Will Deacon <will.deacon@arm.com>; Hoeun Ryu <hoeun.ryu@lge.com>;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] armpmu: broadcast overflow irq on multi-core
> system
> > > having one muxed SPI for PMU.
> 
> > > Muxing the PMU IRQs is a really broken system design, and there's no
> good
> > > way of supporting it.
> 
> > > What we should do for such systems is:
> > >
> > > * Add a flag to the DT to describe that the IRQs are muxed, as this
> > >   cannot be probed.
> > >
> > > * Add hrtimer code to periodically update the counters, to avoid
> > >   overflow (e.g. as we do in the l2x0 PMU).
> > >
> > > * Reject sampling for such systems, as this cannot be done reliably or
> > >   efficiently.
> > >
> > > NAK to broadcasting the IRQ -- there are a number of issues with the
> > > general approach.
> >
> > The second solution would be good if sampling is necessary even like
> those
> > systems.
> 
> Please note that I mean *all* of the above. There would be no sampling
> on systems with muxed PMU IRQs, since there's no correlation between
> overflow events and the hrtimer interrupts -- the results of sampling
> would be misleading.
> 
> > Actually I'm working on FIQ available ARM32 system and trying to enable
> the
> > hard lockup detector by routing the PMU IRQ to FIQ.
> > Because of that, I really need the interrupt event if it is a muxed SPI,
> > beside I also need to make an dedicated IPI FIQ to broadcast the IRQ in
> > this approach.
> > What would you do if you were in the same situation ?
> 
> I don't think that this can work with a muxed IRQ, sorry.
> 
> It would be better to use some kind of timer.
> 
> [...]
> 
> > > Futher, If you ever encounter a case where you need to avoid
> preemption
> > > across enabling IRQs, preemption must be disabled *before* enabling
> IRQs.
> >
> > Ah, OK.
> > Enabling IRQs can cause scheduling tasks in the end of exception or
> other
> > scheduling points, right ?
> 
> Yes. If an IRQ was taken *between* enabling IRQs and disabling
> preemption, preemption may occur as part of the exception return.
> 
> Thanks,
> Mark.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-05-14  2:26 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-10  8:36 [PATCH] armpmu: broadcast overflow irq on multi-core system having one muxed SPI for PMU Hoeun Ryu
2018-05-10 10:21 ` Mark Rutland
2018-05-10 23:20   ` 류호은
2018-05-11 10:39     ` Mark Rutland
2018-05-14  2:26       ` Hoeun Ryu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).