| Message ID | 1380241098-7561-1-git-send-email-scottwood@freescale.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 32dda05f4ec2b854b594bd91590c46c5197d77e1 |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
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
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
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
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
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); }
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(-)