Patchwork powerpc/mpic: Disable preemption when calling mpic_processor_id()

login
register
mail settings
Submitter Scott Wood
Date Sept. 27, 2013, 12:18 a.m.
Message ID <1380241098-7561-1-git-send-email-scottwood@freescale.com>
Download mbox | patch
Permalink /patch/278323/
State Accepted
Commit 32dda05f4ec2b854b594bd91590c46c5197d77e1
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Scott Wood - Sept. 27, 2013, 12:18 a.m.
Otherwise, we get a debug traceback due to the use of
smp_processor_id() (or get_paca()) inside hard_smp_processor_id().
mpic_host_map() is just looking for a default CPU, so it doesn't matter
if we migrate after getting the CPU ID.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/sysdev/mpic.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
Kumar Gala - Sept. 27, 2013, 3:52 p.m.
On Sep 26, 2013, at 7:18 PM, Scott Wood wrote:

> Otherwise, we get a debug traceback due to the use of
> smp_processor_id() (or get_paca()) inside hard_smp_processor_id().
> mpic_host_map() is just looking for a default CPU, so it doesn't matter
> if we migrate after getting the CPU ID.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> arch/powerpc/sysdev/mpic.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> index 1be54fa..bdcb858 100644
> --- a/arch/powerpc/sysdev/mpic.c
> +++ b/arch/powerpc/sysdev/mpic.c
> @@ -1088,8 +1088,14 @@ static int mpic_host_map(struct irq_domain *h, unsigned int virq,
> 	 * is done here.
> 	 */
> 	if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
> +		int cpu;
> +
> +		preempt_disable();
> +		cpu = mpic_processor_id(mpic);
> +		preempt_enable();
> +

Any reason you didn't stick this inside of mpic_processor_id() ?

> 		mpic_set_vector(virq, hw);
> -		mpic_set_destination(virq, mpic_processor_id(mpic));
> +		mpic_set_destination(virq, cpu);
> 		mpic_irq_set_priority(virq, 8);
> 	}
> 
> -- 
> 1.8.1.2
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
Scott Wood - Sept. 27, 2013, 4:15 p.m.
On Fri, 2013-09-27 at 10:52 -0500, Kumar Gala wrote:
> On Sep 26, 2013, at 7:18 PM, Scott Wood wrote:
> 
> > Otherwise, we get a debug traceback due to the use of
> > smp_processor_id() (or get_paca()) inside hard_smp_processor_id().
> > mpic_host_map() is just looking for a default CPU, so it doesn't matter
> > if we migrate after getting the CPU ID.
> > 
> > Signed-off-by: Scott Wood <scottwood@freescale.com>
> > ---
> > arch/powerpc/sysdev/mpic.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> > index 1be54fa..bdcb858 100644
> > --- a/arch/powerpc/sysdev/mpic.c
> > +++ b/arch/powerpc/sysdev/mpic.c
> > @@ -1088,8 +1088,14 @@ static int mpic_host_map(struct irq_domain *h, unsigned int virq,
> > 	 * is done here.
> > 	 */
> > 	if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
> > +		int cpu;
> > +
> > +		preempt_disable();
> > +		cpu = mpic_processor_id(mpic);
> > +		preempt_enable();
> > +
> 
> Any reason you didn't stick this inside of mpic_processor_id() ?

Because the debug check might be valid for other callers and we don't
want to defeat it.  In this caller it's used only as a heuristic and
thus it doesn't matter if we re-enable preemption before using the
result.

-Scott
Kumar Gala - Sept. 27, 2013, 5:09 p.m.
On Sep 27, 2013, at 11:15 AM, Scott Wood wrote:

> On Fri, 2013-09-27 at 10:52 -0500, Kumar Gala wrote:
>> On Sep 26, 2013, at 7:18 PM, Scott Wood wrote:
>> 
>>> Otherwise, we get a debug traceback due to the use of
>>> smp_processor_id() (or get_paca()) inside hard_smp_processor_id().
>>> mpic_host_map() is just looking for a default CPU, so it doesn't matter
>>> if we migrate after getting the CPU ID.
>>> 
>>> Signed-off-by: Scott Wood <scottwood@freescale.com>
>>> ---
>>> arch/powerpc/sysdev/mpic.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
>>> index 1be54fa..bdcb858 100644
>>> --- a/arch/powerpc/sysdev/mpic.c
>>> +++ b/arch/powerpc/sysdev/mpic.c
>>> @@ -1088,8 +1088,14 @@ static int mpic_host_map(struct irq_domain *h, unsigned int virq,
>>> 	 * is done here.
>>> 	 */
>>> 	if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
>>> +		int cpu;
>>> +
>>> +		preempt_disable();
>>> +		cpu = mpic_processor_id(mpic);
>>> +		preempt_enable();
>>> +
>> 
>> Any reason you didn't stick this inside of mpic_processor_id() ?
> 
> Because the debug check might be valid for other callers and we don't
> want to defeat it.  In this caller it's used only as a heuristic and
> thus it doesn't matter if we re-enable preemption before using the
> result.

Gotcha, I think you should reword the commit message.  Reading it makes me think that preemption should be disabled for all mpic_processor_id() calls.

- k
Scott Wood - Sept. 27, 2013, 5:26 p.m.
On Fri, 2013-09-27 at 12:09 -0500, Kumar Gala wrote:
> On Sep 27, 2013, at 11:15 AM, Scott Wood wrote:
> 
> > On Fri, 2013-09-27 at 10:52 -0500, Kumar Gala wrote:
> >> On Sep 26, 2013, at 7:18 PM, Scott Wood wrote:
> >> 
> >>> Otherwise, we get a debug traceback due to the use of
> >>> smp_processor_id() (or get_paca()) inside hard_smp_processor_id().
> >>> mpic_host_map() is just looking for a default CPU, so it doesn't matter
> >>> if we migrate after getting the CPU ID.
> >>> 
> >>> Signed-off-by: Scott Wood <scottwood@freescale.com>
> >>> ---
> >>> arch/powerpc/sysdev/mpic.c | 8 +++++++-
> >>> 1 file changed, 7 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> >>> index 1be54fa..bdcb858 100644
> >>> --- a/arch/powerpc/sysdev/mpic.c
> >>> +++ b/arch/powerpc/sysdev/mpic.c
> >>> @@ -1088,8 +1088,14 @@ static int mpic_host_map(struct irq_domain *h, unsigned int virq,
> >>> 	 * is done here.
> >>> 	 */
> >>> 	if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
> >>> +		int cpu;
> >>> +
> >>> +		preempt_disable();
> >>> +		cpu = mpic_processor_id(mpic);
> >>> +		preempt_enable();
> >>> +
> >> 
> >> Any reason you didn't stick this inside of mpic_processor_id() ?
> > 
> > Because the debug check might be valid for other callers and we don't
> > want to defeat it.  In this caller it's used only as a heuristic and
> > thus it doesn't matter if we re-enable preemption before using the
> > result.
> 
> Gotcha, I think you should reword the commit message.  Reading it makes
> me think that preemption should be disabled for all mpic_processor_id()
> calls.

It should be disabled for all mpic_processor_id() calls -- but some
calls will need preemption to be disabled for longer than just the call
to mpic_processor_id().

I did mention in the commit message why mpic_host_map() is unusual.

BTW, I wonder why we don't use mpic_set_affinity (and thus
irq_choose_cpu), both here and in the non-MPIC_NO_RESET case.  Though I
usually notice IRQs being round robinned -- I guess userspace is writing
to the affinity in proc?  Or is something elsewhere in the kernel
calling set_affinity at some point?

Another thing I just noticed while looking at IRQ_DESTINATION-related
code, in mpic_teardown_this_cpu(), what stops it from removing the only
CPU that an interrupt is targeted at?

-Scott

Patch

diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index 1be54fa..bdcb858 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -1088,8 +1088,14 @@  static int mpic_host_map(struct irq_domain *h, unsigned int virq,
 	 * is done here.
 	 */
 	if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
+		int cpu;
+
+		preempt_disable();
+		cpu = mpic_processor_id(mpic);
+		preempt_enable();
+
 		mpic_set_vector(virq, hw);
-		mpic_set_destination(virq, mpic_processor_id(mpic));
+		mpic_set_destination(virq, cpu);
 		mpic_irq_set_priority(virq, 8);
 	}