* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Wang Sheng-Hui @ 2012-05-03 5:51 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <1336018961.2653.11.camel@pasglop>
On 2012年05月03日 12:22, Benjamin Herrenschmidt wrote:
>
>> It should not as __check_irq_replay() should always be called
>> with interrupts hard disabled... Do you see any code path
>> where that is not the case ?
>
> More specifically, your backtrace seems to indicate that
> __check_irq_repay() was called from arch_local_irq_restore() which
> should have done this before calling __check_irq_replay():
>
> if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
> __hard_irq_disable();
>
> Now, the only possibility that I can see for an interrupt to come in
> and trip the problem you observed would be if for some reason we
> had irq_happened set to PACA_IRQ_HARD_DIS while interrupts were
> not hard disabled.
I have a chance to notice that the value is 0x05, not just 0x01.
So I think this is not the case.
>
> Can you try if removing the test (and thus unconditionally calling
> __hard_irq_disable()) fixes the problem for you ?
>
> If that is the case, then we need to audit the code to figure out how we
> can end up with that bit in irq_happened set and interrupts hard
> enabled.
>
> Something like may_hard_irq_enable() shouldn't cause it since it should
> only be called while hard disabled but adding a check in there might be
> worth it (something like WARN_ON(mfmsr() & MSR_EE)).
>
> Cheers,
> Ben.
>
>> Cheers,
>> Ben.
>>
>>> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
>>> ---
>>> arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
>>> 1 files changed, 30 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>>> index 5ec1b23..3d48b23 100644
>>> --- a/arch/powerpc/kernel/irq.c
>>> +++ b/arch/powerpc/kernel/irq.c
>>> @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
>>> */
>>> notrace unsigned int __check_irq_replay(void)
>>> {
>>> + unsigned int ret_val;
>>> /*
>>> * We use local_paca rather than get_paca() to avoid all
>>> * the debug_smp_processor_id() business in this low level
>>> * function
>>> */
>>> - unsigned char happened = local_paca->irq_happened;
>>> + unsigned char happened, irq_happened;
>>> + happened = irq_happened = local_paca->irq_happened;
>>>
>>> /* Clear bit 0 which we wouldn't clear otherwise */
>>> - local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
>>> + irq_happened &= ~PACA_IRQ_HARD_DIS;
>>>
>>> /*
>>> * Force the delivery of pending soft-disabled interrupts on PS3.
>>> @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
>>> * decrementer itself rather than the paca irq_happened field
>>> * in case we also had a rollover while hard disabled
>>> */
>>> - local_paca->irq_happened &= ~PACA_IRQ_DEC;
>>> - if (decrementer_check_overflow())
>>> - return 0x900;
>>> + irq_happened &= ~PACA_IRQ_DEC;
>>> + if (decrementer_check_overflow()) {
>>> + ret_val = 0x900;
>>> + goto replay;
>>> + }
>>>
>>> /* Finally check if an external interrupt happened */
>>> - local_paca->irq_happened &= ~PACA_IRQ_EE;
>>> - if (happened & PACA_IRQ_EE)
>>> - return 0x500;
>>> + irq_happened &= ~PACA_IRQ_EE;
>>> + if (happened & PACA_IRQ_EE) {
>>> + ret_val = 0x500;
>>> + goto replay;
>>> + }
>>>
>>> #ifdef CONFIG_PPC_BOOK3E
>>> /* Finally check if an EPR external interrupt happened
>>> * this bit is typically set if we need to handle another
>>> * "edge" interrupt from within the MPIC "EPR" handler
>>> */
>>> - local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
>>> - if (happened & PACA_IRQ_EE_EDGE)
>>> - return 0x500;
>>> + irq_happened &= ~PACA_IRQ_EE_EDGE;
>>> + if (happened & PACA_IRQ_EE_EDGE) {
>>> + ret_val = 0x500;
>>> + goto replay;
>>> + }
>>>
>>> - local_paca->irq_happened &= ~PACA_IRQ_DBELL;
>>> - if (happened & PACA_IRQ_DBELL)
>>> - return 0x280;
>>> + irq_happened &= ~PACA_IRQ_DBELL;
>>> + if (happened & PACA_IRQ_DBELL) {
>>> + ret_val = 0x280;
>>> + goto replay;
>>> + }
>>> #endif /* CONFIG_PPC_BOOK3E */
>>>
>>> /* There should be nothing left ! */
>>> - BUG_ON(local_paca->irq_happened != 0);
>>> + BUG_ON(irq_happened != 0);
>>> + ret_val = 0;
>>>
>>> - return 0;
>>> +replay:
>>> + local_paca->irq_happened = irq_happened;
>>> +
>>> + return ret_val;
>>> }
>>>
>>> notrace void arch_local_irq_restore(unsigned long en)
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>
>
^ permalink raw reply
* [PATCH] powerpc/windfarm: fix compiler warning
From: Stephen Rothwell @ 2012-05-03 4:34 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: ppc-dev
[-- Attachment #1: Type: text/plain, Size: 1206 bytes --]
Fixes this warning:
drivers/macintosh/windfarm_pm91.c: In function 'wf_smu_cpu_fans_tick':
drivers/macintosh/windfarm_pm91.c:237:2: warning: passing argument 1 of 'wf_sensor_get' from incompatible pointer type
drivers/macintosh/windfarm.h:124:19: note: expected 'struct wf_sensor *' but argument is of type 'struct wf_sensor **'
Introduced by commit 33e6820b767a ("powerpc/windfarm: Add useful
accessors").
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
drivers/macintosh/windfarm_pm91.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/macintosh/windfarm_pm91.c b/drivers/macintosh/windfarm_pm91.c
index e18002b..7653603 100644
--- a/drivers/macintosh/windfarm_pm91.c
+++ b/drivers/macintosh/windfarm_pm91.c
@@ -234,7 +234,7 @@ static void wf_smu_cpu_fans_tick(struct wf_smu_cpu_fans_state *st)
return;
}
- rc = wf_sensor_get(&sensor_cpu_power, &power);
+ rc = wf_sensor_get(sensor_cpu_power, &power);
if (rc) {
printk(KERN_WARNING "windfarm: CPU power sensor error %d\n",
rc);
--
1.7.10.280.gaa39
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Benjamin Herrenschmidt @ 2012-05-03 4:26 UTC (permalink / raw)
To: Wang Sheng-Hui
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <4FA1EE2C.6050201@gmail.com>
On Thu, 2012-05-03 at 10:32 +0800, Wang Sheng-Hui wrote:
> > It should not as __check_irq_replay() should always be called
> > with interrupts hard disabled... Do you see any code path
> > where that is not the case ?
>
> Since __check_irq_replay() should always be called with interrupts
> hard disabled, I think it's harmless to use local var here.
No, that would be papering over the real problem. All oprofile does is
trigger perfmon interrupts (which act as some kind of NMI when
soft-disabled but should be masked by MSR:EE when hard disabled).
So there's a deeper issue here that we need to understand before we can
propose a fix. IE. It should not have happened.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Benjamin Herrenschmidt @ 2012-05-03 4:22 UTC (permalink / raw)
To: Wang Sheng-Hui
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <1336011306.2653.3.camel@pasglop>
> It should not as __check_irq_replay() should always be called
> with interrupts hard disabled... Do you see any code path
> where that is not the case ?
More specifically, your backtrace seems to indicate that
__check_irq_repay() was called from arch_local_irq_restore() which
should have done this before calling __check_irq_replay():
if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
__hard_irq_disable();
Now, the only possibility that I can see for an interrupt to come in
and trip the problem you observed would be if for some reason we
had irq_happened set to PACA_IRQ_HARD_DIS while interrupts were
not hard disabled.
Can you try if removing the test (and thus unconditionally calling
__hard_irq_disable()) fixes the problem for you ?
If that is the case, then we need to audit the code to figure out how we
can end up with that bit in irq_happened set and interrupts hard
enabled.
Something like may_hard_irq_enable() shouldn't cause it since it should
only be called while hard disabled but adding a check in there might be
worth it (something like WARN_ON(mfmsr() & MSR_EE)).
Cheers,
Ben.
> Cheers,
> Ben.
>
> > Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
> > ---
> > arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
> > 1 files changed, 30 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> > index 5ec1b23..3d48b23 100644
> > --- a/arch/powerpc/kernel/irq.c
> > +++ b/arch/powerpc/kernel/irq.c
> > @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
> > */
> > notrace unsigned int __check_irq_replay(void)
> > {
> > + unsigned int ret_val;
> > /*
> > * We use local_paca rather than get_paca() to avoid all
> > * the debug_smp_processor_id() business in this low level
> > * function
> > */
> > - unsigned char happened = local_paca->irq_happened;
> > + unsigned char happened, irq_happened;
> > + happened = irq_happened = local_paca->irq_happened;
> >
> > /* Clear bit 0 which we wouldn't clear otherwise */
> > - local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
> > + irq_happened &= ~PACA_IRQ_HARD_DIS;
> >
> > /*
> > * Force the delivery of pending soft-disabled interrupts on PS3.
> > @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
> > * decrementer itself rather than the paca irq_happened field
> > * in case we also had a rollover while hard disabled
> > */
> > - local_paca->irq_happened &= ~PACA_IRQ_DEC;
> > - if (decrementer_check_overflow())
> > - return 0x900;
> > + irq_happened &= ~PACA_IRQ_DEC;
> > + if (decrementer_check_overflow()) {
> > + ret_val = 0x900;
> > + goto replay;
> > + }
> >
> > /* Finally check if an external interrupt happened */
> > - local_paca->irq_happened &= ~PACA_IRQ_EE;
> > - if (happened & PACA_IRQ_EE)
> > - return 0x500;
> > + irq_happened &= ~PACA_IRQ_EE;
> > + if (happened & PACA_IRQ_EE) {
> > + ret_val = 0x500;
> > + goto replay;
> > + }
> >
> > #ifdef CONFIG_PPC_BOOK3E
> > /* Finally check if an EPR external interrupt happened
> > * this bit is typically set if we need to handle another
> > * "edge" interrupt from within the MPIC "EPR" handler
> > */
> > - local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
> > - if (happened & PACA_IRQ_EE_EDGE)
> > - return 0x500;
> > + irq_happened &= ~PACA_IRQ_EE_EDGE;
> > + if (happened & PACA_IRQ_EE_EDGE) {
> > + ret_val = 0x500;
> > + goto replay;
> > + }
> >
> > - local_paca->irq_happened &= ~PACA_IRQ_DBELL;
> > - if (happened & PACA_IRQ_DBELL)
> > - return 0x280;
> > + irq_happened &= ~PACA_IRQ_DBELL;
> > + if (happened & PACA_IRQ_DBELL) {
> > + ret_val = 0x280;
> > + goto replay;
> > + }
> > #endif /* CONFIG_PPC_BOOK3E */
> >
> > /* There should be nothing left ! */
> > - BUG_ON(local_paca->irq_happened != 0);
> > + BUG_ON(irq_happened != 0);
> > + ret_val = 0;
> >
> > - return 0;
> > +replay:
> > + local_paca->irq_happened = irq_happened;
> > +
> > + return ret_val;
> > }
> >
> > notrace void arch_local_irq_restore(unsigned long en)
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Wang Sheng-Hui @ 2012-05-03 2:32 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <1336011306.2653.3.camel@pasglop>
On 2012年05月03日 10:15, Benjamin Herrenschmidt wrote:
> On Thu, 2012-05-03 at 09:53 +0800, Wang Sheng-Hui wrote:
>> local_paca->irq_happened may be changed asychronously.
>>
>> In my test env (IBM Power 9117-MMA), I installed the RHEL6.2 with the shipped
>> oprofile. Then I run into kernel v3.4-rc4, setup/start oprofile and start the
>> LTP test suite.
>>
>> In a short while, the system would crash. Seems that oprofile may change
>> the irq_happened.
>
> .../...
>
>> Use local var instead of local_paca->irq_happened directly in this function here.
>>
>> Please check this patch. Any comments are welcome.
>
> It should not as __check_irq_replay() should always be called
> with interrupts hard disabled... Do you see any code path
> where that is not the case ?
Since __check_irq_replay() should always be called with interrupts hard disabled,
I think it's harmless to use local var here.
>
> Cheers,
> Ben.
>
>> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
>> ---
>> arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
>> 1 files changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>> index 5ec1b23..3d48b23 100644
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
>> */
>> notrace unsigned int __check_irq_replay(void)
>> {
>> + unsigned int ret_val;
>> /*
>> * We use local_paca rather than get_paca() to avoid all
>> * the debug_smp_processor_id() business in this low level
>> * function
>> */
>> - unsigned char happened = local_paca->irq_happened;
>> + unsigned char happened, irq_happened;
>> + happened = irq_happened = local_paca->irq_happened;
>>
>> /* Clear bit 0 which we wouldn't clear otherwise */
>> - local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
>> + irq_happened &= ~PACA_IRQ_HARD_DIS;
>>
>> /*
>> * Force the delivery of pending soft-disabled interrupts on PS3.
>> @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
>> * decrementer itself rather than the paca irq_happened field
>> * in case we also had a rollover while hard disabled
>> */
>> - local_paca->irq_happened &= ~PACA_IRQ_DEC;
>> - if (decrementer_check_overflow())
>> - return 0x900;
>> + irq_happened &= ~PACA_IRQ_DEC;
>> + if (decrementer_check_overflow()) {
>> + ret_val = 0x900;
>> + goto replay;
>> + }
>>
>> /* Finally check if an external interrupt happened */
>> - local_paca->irq_happened &= ~PACA_IRQ_EE;
>> - if (happened & PACA_IRQ_EE)
>> - return 0x500;
>> + irq_happened &= ~PACA_IRQ_EE;
>> + if (happened & PACA_IRQ_EE) {
>> + ret_val = 0x500;
>> + goto replay;
>> + }
>>
>> #ifdef CONFIG_PPC_BOOK3E
>> /* Finally check if an EPR external interrupt happened
>> * this bit is typically set if we need to handle another
>> * "edge" interrupt from within the MPIC "EPR" handler
>> */
>> - local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
>> - if (happened & PACA_IRQ_EE_EDGE)
>> - return 0x500;
>> + irq_happened &= ~PACA_IRQ_EE_EDGE;
>> + if (happened & PACA_IRQ_EE_EDGE) {
>> + ret_val = 0x500;
>> + goto replay;
>> + }
>>
>> - local_paca->irq_happened &= ~PACA_IRQ_DBELL;
>> - if (happened & PACA_IRQ_DBELL)
>> - return 0x280;
>> + irq_happened &= ~PACA_IRQ_DBELL;
>> + if (happened & PACA_IRQ_DBELL) {
>> + ret_val = 0x280;
>> + goto replay;
>> + }
>> #endif /* CONFIG_PPC_BOOK3E */
>>
>> /* There should be nothing left ! */
>> - BUG_ON(local_paca->irq_happened != 0);
>> + BUG_ON(irq_happened != 0);
>> + ret_val = 0;
>>
>> - return 0;
>> +replay:
>> + local_paca->irq_happened = irq_happened;
>> +
>> + return ret_val;
>> }
>>
>> notrace void arch_local_irq_restore(unsigned long en)
>
>
^ permalink raw reply
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Wang Sheng-Hui @ 2012-05-03 2:27 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <1336011306.2653.3.camel@pasglop>
On 2012年05月03日 10:15, Benjamin Herrenschmidt wrote:
> On Thu, 2012-05-03 at 09:53 +0800, Wang Sheng-Hui wrote:
>> local_paca->irq_happened may be changed asychronously.
>>
>> In my test env (IBM Power 9117-MMA), I installed the RHEL6.2 with the shipped
>> oprofile. Then I run into kernel v3.4-rc4, setup/start oprofile and start the
>> LTP test suite.
>>
>> In a short while, the system would crash. Seems that oprofile may change
>> the irq_happened.
>
> .../...
>
>> Use local var instead of local_paca->irq_happened directly in this function here.
>>
>> Please check this patch. Any comments are welcome.
>
> It should not as __check_irq_replay() should always be called
> with interrupts hard disabled... Do you see any code path
> where that is not the case ?
This is the only case.
I have run LTP test suite on my system without oprofile over 24 hours
with 3.4-rc4 kernel.
Then I started oprofile, and the system crashed quickly.
I wonder if oprofile does some special changes with the running.
But I'm not familiar with the internal of oprofile.
I tried to change BUG_ON to WARN_ON, and got lots of warnning messages
in dmesg. So I changed it to local var here.
>
> Cheers,
> Ben.
>
>> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
>> ---
>> arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
>> 1 files changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>> index 5ec1b23..3d48b23 100644
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
>> */
>> notrace unsigned int __check_irq_replay(void)
>> {
>> + unsigned int ret_val;
>> /*
>> * We use local_paca rather than get_paca() to avoid all
>> * the debug_smp_processor_id() business in this low level
>> * function
>> */
>> - unsigned char happened = local_paca->irq_happened;
>> + unsigned char happened, irq_happened;
>> + happened = irq_happened = local_paca->irq_happened;
>>
>> /* Clear bit 0 which we wouldn't clear otherwise */
>> - local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
>> + irq_happened &= ~PACA_IRQ_HARD_DIS;
>>
>> /*
>> * Force the delivery of pending soft-disabled interrupts on PS3.
>> @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
>> * decrementer itself rather than the paca irq_happened field
>> * in case we also had a rollover while hard disabled
>> */
>> - local_paca->irq_happened &= ~PACA_IRQ_DEC;
>> - if (decrementer_check_overflow())
>> - return 0x900;
>> + irq_happened &= ~PACA_IRQ_DEC;
>> + if (decrementer_check_overflow()) {
>> + ret_val = 0x900;
>> + goto replay;
>> + }
>>
>> /* Finally check if an external interrupt happened */
>> - local_paca->irq_happened &= ~PACA_IRQ_EE;
>> - if (happened & PACA_IRQ_EE)
>> - return 0x500;
>> + irq_happened &= ~PACA_IRQ_EE;
>> + if (happened & PACA_IRQ_EE) {
>> + ret_val = 0x500;
>> + goto replay;
>> + }
>>
>> #ifdef CONFIG_PPC_BOOK3E
>> /* Finally check if an EPR external interrupt happened
>> * this bit is typically set if we need to handle another
>> * "edge" interrupt from within the MPIC "EPR" handler
>> */
>> - local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
>> - if (happened & PACA_IRQ_EE_EDGE)
>> - return 0x500;
>> + irq_happened &= ~PACA_IRQ_EE_EDGE;
>> + if (happened & PACA_IRQ_EE_EDGE) {
>> + ret_val = 0x500;
>> + goto replay;
>> + }
>>
>> - local_paca->irq_happened &= ~PACA_IRQ_DBELL;
>> - if (happened & PACA_IRQ_DBELL)
>> - return 0x280;
>> + irq_happened &= ~PACA_IRQ_DBELL;
>> + if (happened & PACA_IRQ_DBELL) {
>> + ret_val = 0x280;
>> + goto replay;
>> + }
>> #endif /* CONFIG_PPC_BOOK3E */
>>
>> /* There should be nothing left ! */
>> - BUG_ON(local_paca->irq_happened != 0);
>> + BUG_ON(irq_happened != 0);
>> + ret_val = 0;
>>
>> - return 0;
>> +replay:
>> + local_paca->irq_happened = irq_happened;
>> +
>> + return ret_val;
>> }
>>
>> notrace void arch_local_irq_restore(unsigned long en)
>
>
^ permalink raw reply
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Benjamin Herrenschmidt @ 2012-05-03 2:15 UTC (permalink / raw)
To: Wang Sheng-Hui
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <4FA1E527.1090807@gmail.com>
On Thu, 2012-05-03 at 09:53 +0800, Wang Sheng-Hui wrote:
> local_paca->irq_happened may be changed asychronously.
>
> In my test env (IBM Power 9117-MMA), I installed the RHEL6.2 with the shipped
> oprofile. Then I run into kernel v3.4-rc4, setup/start oprofile and start the
> LTP test suite.
>
> In a short while, the system would crash. Seems that oprofile may change
> the irq_happened.
.../...
> Use local var instead of local_paca->irq_happened directly in this function here.
>
> Please check this patch. Any comments are welcome.
It should not as __check_irq_replay() should always be called
with interrupts hard disabled... Do you see any code path
where that is not the case ?
Cheers,
Ben.
> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
> ---
> arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
> 1 files changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 5ec1b23..3d48b23 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
> */
> notrace unsigned int __check_irq_replay(void)
> {
> + unsigned int ret_val;
> /*
> * We use local_paca rather than get_paca() to avoid all
> * the debug_smp_processor_id() business in this low level
> * function
> */
> - unsigned char happened = local_paca->irq_happened;
> + unsigned char happened, irq_happened;
> + happened = irq_happened = local_paca->irq_happened;
>
> /* Clear bit 0 which we wouldn't clear otherwise */
> - local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
> + irq_happened &= ~PACA_IRQ_HARD_DIS;
>
> /*
> * Force the delivery of pending soft-disabled interrupts on PS3.
> @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
> * decrementer itself rather than the paca irq_happened field
> * in case we also had a rollover while hard disabled
> */
> - local_paca->irq_happened &= ~PACA_IRQ_DEC;
> - if (decrementer_check_overflow())
> - return 0x900;
> + irq_happened &= ~PACA_IRQ_DEC;
> + if (decrementer_check_overflow()) {
> + ret_val = 0x900;
> + goto replay;
> + }
>
> /* Finally check if an external interrupt happened */
> - local_paca->irq_happened &= ~PACA_IRQ_EE;
> - if (happened & PACA_IRQ_EE)
> - return 0x500;
> + irq_happened &= ~PACA_IRQ_EE;
> + if (happened & PACA_IRQ_EE) {
> + ret_val = 0x500;
> + goto replay;
> + }
>
> #ifdef CONFIG_PPC_BOOK3E
> /* Finally check if an EPR external interrupt happened
> * this bit is typically set if we need to handle another
> * "edge" interrupt from within the MPIC "EPR" handler
> */
> - local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
> - if (happened & PACA_IRQ_EE_EDGE)
> - return 0x500;
> + irq_happened &= ~PACA_IRQ_EE_EDGE;
> + if (happened & PACA_IRQ_EE_EDGE) {
> + ret_val = 0x500;
> + goto replay;
> + }
>
> - local_paca->irq_happened &= ~PACA_IRQ_DBELL;
> - if (happened & PACA_IRQ_DBELL)
> - return 0x280;
> + irq_happened &= ~PACA_IRQ_DBELL;
> + if (happened & PACA_IRQ_DBELL) {
> + ret_val = 0x280;
> + goto replay;
> + }
> #endif /* CONFIG_PPC_BOOK3E */
>
> /* There should be nothing left ! */
> - BUG_ON(local_paca->irq_happened != 0);
> + BUG_ON(irq_happened != 0);
> + ret_val = 0;
>
> - return 0;
> +replay:
> + local_paca->irq_happened = irq_happened;
> +
> + return ret_val;
> }
>
> notrace void arch_local_irq_restore(unsigned long en)
^ permalink raw reply
* [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Wang Sheng-Hui @ 2012-05-03 1:53 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Milton Miller, Grant Likely,
Stephen Rothwell, Anton Blanchard, linuxppc-dev, linux-kernel
local_paca->irq_happened may be changed asychronously.
In my test env (IBM Power 9117-MMA), I installed the RHEL6.2 with the shipped
oprofile. Then I run into kernel v3.4-rc4, setup/start oprofile and start the
LTP test suite.
In a short while, the system would crash. Seems that oprofile may change
the irq_happened.
======================================================================
KERNEL: /boot/vmlinux-3.4.0-rc4-00104-gaf3a3ab
DUMPFILE: vmcore [PARTIAL DUMP]
CPUS: 10
DATE: Fri Apr 27 18:54:34 2012
UPTIME: 00:02:34
LOAD AVERAGE: 0.60, 0.27, 0.10
TASKS: 369
NODENAME: feastlp3.upt.austin.ibm.com
RELEASE: 3.4.0-rc4-00104-gaf3a3ab
VERSION: #4 SMP Fri Apr 27 03:13:43 CDT 2012
MACHINE: ppc64 (4704 Mhz)
MEMORY: 9.8 GB
PANIC: "kernel BUG at /usr/src/kernels/linux/arch/powerpc/kernel/irq.c:188!"
PID: 0
COMMAND: "swapper/4"
TASK: c0000002694e3cc0 (1 of 10) [THREAD_INFO: c0000002694f8000]
CPU: 4
STATE: TASK_RUNNING (PANIC)
crash> bt
PID: 0 TASK: c0000002694e3cc0 CPU: 4 COMMAND: "swapper/4"
#0 [c00000026ffcb6e0] .crash_kexec at c0000000000f22e8
#1 [c00000026ffcb8e0] .oops_end at c00000000060aed8
#2 [c00000026ffcb980] ._exception at c000000000020900
#3 [c00000026ffcbb40] program_check_common at c0000000000053b4
Breakpoint trap [700] exception frame:
R0: 0000000000000001 R1: c00000026ffcbe30 R2: c000000000edd170
R3: 0000000000000500 R4: 0000000000000000 R5: 00000000000007fd
R6: 000000000124a180 R7: 003450cf9bd1233b R8: 0000000000940000
R9: c000000003400c00 R10: 0000000000000001 R11: 0000000000000000
R12: 0000000000000002 R13: c000000003400c00 R14: c0000002694fbf90
R15: 0000000002000040 R16: 0000000000000004 R17: 0000000000000000
R18: 0000000000000000 R19: 0000000000000000 R20: c000000000f42100
R21: 0000000000000000 R22: c000000000955b80 R23: c000000000955b80
R24: 000000000000000a R25: 0000000000000004 R26: c0000002694f8100
R27: c00000026ffc8000 R28: 0000000000000000 R29: c000000000f42100
R30: c000000000e60810 R31: 0000000000000040
NIP: c00000000000ea9c MSR: 8000000000029032 OR3: c00000000000ea3c
CTR: c000000000063e40 LR: c000000000010578 XER: 0000000000000000
CCR: 0000000028000048 MQ: 0000000000000000 DAR: c000000001295d00
DSISR: 0000000000000000 Syscall Result: 0000000000000000
#4 [c00000026ffcbe30] .__check_irq_replay at c00000000000ea9c
[Link Register ] [c00000026ffcbe30] .arch_local_irq_restore at c000000000010578
#5 [c00000026ffcbea0] .__do_softirq at c000000000085724
#6 [c00000026ffcbf90] .call_do_softirq at c000000000022928
#7 [c0000002694fb8d0] .do_softirq at c0000000000106c8
#8 [c0000002694fb970] .irq_exit at c000000000085414
#9 [c0000002694fb9f0] .do_IRQ at c0000000000100a4
#10 [c0000002694fbab0] hardware_interrupt_common at c0000000000038c0
Hardware Interrupt [501] exception frame:
R0: 0000000000000001 R1: c0000002694fbda0 R2: c000000000edd170
R3: 0000000000000000 R4: 0000000000000000 R5: 0000000000000000
R6: 00000000000000e0 R7: 003450cf9bd1233b R8: 0000000000940000
R9: ffffffffffffffff R10: 0000000000243694 R11: 0000000000000001
R12: 0000000000000002 R13: c000000003400c00
NIP: c0000000000105b4 MSR: 8000000000009032 OR3: 0000000000000c00
CTR: c0000000004de3a0 LR: c0000000000105b4 XER: 0000000000000000
CCR: 0000000044000044 MQ: 0000000000000001 DAR: c0000000012990b0
DSISR: c0000002694fbce0 Syscall Result: 0000000000000000
#11 [c0000002694fbda0] .arch_local_irq_restore at c0000000000105b4 (unreliable)
#12 [c0000002694fbe10] .cpu_idle at c000000000017d20
#13 [c0000002694fbed0] .start_secondary at c00000000061a934
#14 [c0000002694fbf90] .start_secondary_prolog at c00000000000936c
Use local var instead of local_paca->irq_happened directly in this function here.
Please check this patch. Any comments are welcome.
Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
---
arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
1 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 5ec1b23..3d48b23 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
*/
notrace unsigned int __check_irq_replay(void)
{
+ unsigned int ret_val;
/*
* We use local_paca rather than get_paca() to avoid all
* the debug_smp_processor_id() business in this low level
* function
*/
- unsigned char happened = local_paca->irq_happened;
+ unsigned char happened, irq_happened;
+ happened = irq_happened = local_paca->irq_happened;
/* Clear bit 0 which we wouldn't clear otherwise */
- local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
+ irq_happened &= ~PACA_IRQ_HARD_DIS;
/*
* Force the delivery of pending soft-disabled interrupts on PS3.
@@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
* decrementer itself rather than the paca irq_happened field
* in case we also had a rollover while hard disabled
*/
- local_paca->irq_happened &= ~PACA_IRQ_DEC;
- if (decrementer_check_overflow())
- return 0x900;
+ irq_happened &= ~PACA_IRQ_DEC;
+ if (decrementer_check_overflow()) {
+ ret_val = 0x900;
+ goto replay;
+ }
/* Finally check if an external interrupt happened */
- local_paca->irq_happened &= ~PACA_IRQ_EE;
- if (happened & PACA_IRQ_EE)
- return 0x500;
+ irq_happened &= ~PACA_IRQ_EE;
+ if (happened & PACA_IRQ_EE) {
+ ret_val = 0x500;
+ goto replay;
+ }
#ifdef CONFIG_PPC_BOOK3E
/* Finally check if an EPR external interrupt happened
* this bit is typically set if we need to handle another
* "edge" interrupt from within the MPIC "EPR" handler
*/
- local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
- if (happened & PACA_IRQ_EE_EDGE)
- return 0x500;
+ irq_happened &= ~PACA_IRQ_EE_EDGE;
+ if (happened & PACA_IRQ_EE_EDGE) {
+ ret_val = 0x500;
+ goto replay;
+ }
- local_paca->irq_happened &= ~PACA_IRQ_DBELL;
- if (happened & PACA_IRQ_DBELL)
- return 0x280;
+ irq_happened &= ~PACA_IRQ_DBELL;
+ if (happened & PACA_IRQ_DBELL) {
+ ret_val = 0x280;
+ goto replay;
+ }
#endif /* CONFIG_PPC_BOOK3E */
/* There should be nothing left ! */
- BUG_ON(local_paca->irq_happened != 0);
+ BUG_ON(irq_happened != 0);
+ ret_val = 0;
- return 0;
+replay:
+ local_paca->irq_happened = irq_happened;
+
+ return ret_val;
}
notrace void arch_local_irq_restore(unsigned long en)
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] net/pasemi: fix compiler warning
From: David Miller @ 2012-05-03 0:53 UTC (permalink / raw)
To: sfr; +Cc: olof, netdev, netdev, linuxppc-dev
In-Reply-To: <20120503105146.83f0a4a074dafad0443b875d@canb.auug.org.au>
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 3 May 2012 10:51:46 +1000
> Fix this compiler warning (on PowerPC) by not marking a parameter as
> const:
>
> drivers/net/ethernet/pasemi/pasemi_mac.c: In function 'pasemi_mac_replenish_rx_ring':
> drivers/net/ethernet/pasemi/pasemi_mac.c:646:3: warning: passing argument 1 of 'netdev_alloc_skb' discards qualifiers from pointer target type
> include/linux/skbuff.h:1706:31: note: expected 'struct net_device *' but argument is of type 'const struct net_device *'
>
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Pradeep A. Dalvi <netdev@pradeepdalvi.com>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
Applied, thanks.
^ permalink raw reply
* [PATCH] net/pasemi: fix compiler warning
From: Stephen Rothwell @ 2012-05-03 0:51 UTC (permalink / raw)
To: David Miller, netdev; +Cc: Olof Johansson, Pradeep A. Dalvi, ppc-dev
[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]
Fix this compiler warning (on PowerPC) by not marking a parameter as
const:
drivers/net/ethernet/pasemi/pasemi_mac.c: In function 'pasemi_mac_replenish_rx_ring':
drivers/net/ethernet/pasemi/pasemi_mac.c:646:3: warning: passing argument 1 of 'netdev_alloc_skb' discards qualifiers from pointer target type
include/linux/skbuff.h:1706:31: note: expected 'struct net_device *' but argument is of type 'const struct net_device *'
Cc: Olof Johansson <olof@lixom.net>
Cc: Pradeep A. Dalvi <netdev@pradeepdalvi.com>
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
drivers/net/ethernet/pasemi/pasemi_mac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
The warning was introduced by commit dae2e9f430c4 ("netdev: ethernet
dev_alloc_skb to netdev_alloc_skb").
diff --git a/drivers/net/ethernet/pasemi/pasemi_mac.c b/drivers/net/ethernet/pasemi/pasemi_mac.c
index ddc95b0..e559dfa 100644
--- a/drivers/net/ethernet/pasemi/pasemi_mac.c
+++ b/drivers/net/ethernet/pasemi/pasemi_mac.c
@@ -623,7 +623,7 @@ static void pasemi_mac_free_rx_resources(struct pasemi_mac *mac)
mac->rx = NULL;
}
-static void pasemi_mac_replenish_rx_ring(const struct net_device *dev,
+static void pasemi_mac_replenish_rx_ring(struct net_device *dev,
const int limit)
{
const struct pasemi_mac *mac = netdev_priv(dev);
--
1.7.10.280.gaa39
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Hugh Dickins @ 2012-05-03 0:24 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linuxppc-dev, Christoph Lameter, linux-kernel, Paul E. McKenney
In-Reply-To: <20120503001433.GO2450@linux.vnet.ibm.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2760 bytes --]
On Wed, 2 May 2012, Paul E. McKenney wrote:
> On Wed, May 02, 2012 at 03:54:24PM -0700, Hugh Dickins wrote:
> > On Wed, May 2, 2012 at 2:54 PM, Paul E. McKenney
> > <paulmck@linux.vnet.ibm.com> wrote:
> > > On Thu, May 03, 2012 at 07:20:15AM +1000, Benjamin Herrenschmidt wrote:
> > >> On Wed, 2012-05-02 at 13:25 -0700, Hugh Dickins wrote:
> > >> > Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> > >> > __rcu_read_unlock() are not safe to be using __this_cpu operations,
> > >> > the cpu may change in between the rmw's read and write: they should
> > >> > be using this_cpu operations (or, I put preempt_disable/enable in the
> > >> > __rcu_read_unlock below). __this_cpus there work out fine on x86,
> > >> > which was given good instructions to use; but not so well on PowerPC.
> > >> >
> > >> > I've been running successfully for an hour now with the patch below;
> > >> > but I expect you'll want to consider the tradeoffs, and may choose a
> > >> > different solution.
> > >>
> > >> Didn't Linus recently rant about these __this_cpu vs this_cpu nonsense ?
> > >>
> > >> I thought that was going out..
> > >
> > > Linus did rant about __raw_get_cpu_var() because it cannot use the x86
> > > %fs segement overrides a bit more than a month ago. The __this_cpu
> > > stuff is useful if you have preemption disabled -- avoids the extra
> > > layer of preempt_disable().
> > >
> > > Or was this a different rant?
> >
> > https://lkml.org/lkml/2011/11/29/321
> >
> > I think it ended up with Christoph removing the more egregious
> > variants, but this_cpu_that and __this_cpu_the_other remaining.
>
> Ah, thank you for the pointer.
>
> It would be nice to have the CPU transparency of x86 on other
> architectures, but from what I can see, that would require dedicating
> a register to this purpose -- and even then requires that the arch
> have indexed addressing modes. There are some other approaches, for
> example, having __this_cpu_that() be located at a special address that
> the scheduler treated as implicitly preempt_disable(). Or I suppose
> that the arch-specific trap-handling code could fake it. A little
> bit messy, but the ability to access a given CPU's per-CPU variable
> while running on that CPU does appear to have at least a couple of
> uses -- inlining RCU and also making preempt_disable() use per-CPU
> variables.
>
> In any case, I must confess that I feel quite silly about my series
> of patches. I have reverted them aside from a couple that did useful
> optimizations, and they should show up in -next shortly.
A wee bit sad, but thank you - it was an experiment worth trying,
and perhaps there will be reason to come back to it future.
Hugh
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Paul E. McKenney @ 2012-05-03 0:14 UTC (permalink / raw)
To: Hugh Dickins
Cc: linuxppc-dev, Christoph Lameter, linux-kernel, Paul E. McKenney
In-Reply-To: <CANsGZ6bw4JwMRgUriooDvFB=g-HkpyL8XJa9s47RPfOEPCMcpw@mail.gmail.com>
On Wed, May 02, 2012 at 03:54:24PM -0700, Hugh Dickins wrote:
> On Wed, May 2, 2012 at 2:54 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Thu, May 03, 2012 at 07:20:15AM +1000, Benjamin Herrenschmidt wrote:
> >> On Wed, 2012-05-02 at 13:25 -0700, Hugh Dickins wrote:
> >> > Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> >> > __rcu_read_unlock() are not safe to be using __this_cpu operations,
> >> > the cpu may change in between the rmw's read and write: they should
> >> > be using this_cpu operations (or, I put preempt_disable/enable in the
> >> > __rcu_read_unlock below). __this_cpus there work out fine on x86,
> >> > which was given good instructions to use; but not so well on PowerPC.
> >> >
> >> > I've been running successfully for an hour now with the patch below;
> >> > but I expect you'll want to consider the tradeoffs, and may choose a
> >> > different solution.
> >>
> >> Didn't Linus recently rant about these __this_cpu vs this_cpu nonsense ?
> >>
> >> I thought that was going out..
> >
> > Linus did rant about __raw_get_cpu_var() because it cannot use the x86
> > %fs segement overrides a bit more than a month ago. The __this_cpu
> > stuff is useful if you have preemption disabled -- avoids the extra
> > layer of preempt_disable().
> >
> > Or was this a different rant?
>
> https://lkml.org/lkml/2011/11/29/321
>
> I think it ended up with Christoph removing the more egregious
> variants, but this_cpu_that and __this_cpu_the_other remaining.
Ah, thank you for the pointer.
It would be nice to have the CPU transparency of x86 on other
architectures, but from what I can see, that would require dedicating
a register to this purpose -- and even then requires that the arch
have indexed addressing modes. There are some other approaches, for
example, having __this_cpu_that() be located at a special address that
the scheduler treated as implicitly preempt_disable(). Or I suppose
that the arch-specific trap-handling code could fake it. A little
bit messy, but the ability to access a given CPU's per-CPU variable
while running on that CPU does appear to have at least a couple of
uses -- inlining RCU and also making preempt_disable() use per-CPU
variables.
In any case, I must confess that I feel quite silly about my series
of patches. I have reverted them aside from a couple that did useful
optimizations, and they should show up in -next shortly.
Thanx, Paul
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Hugh Dickins @ 2012-05-02 22:54 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linuxppc-dev, Christoph Lameter, linux-kernel, Paul E. McKenney
In-Reply-To: <20120502215406.GL2450@linux.vnet.ibm.com>
On Wed, May 2, 2012 at 2:54 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Thu, May 03, 2012 at 07:20:15AM +1000, Benjamin Herrenschmidt wrote:
>> On Wed, 2012-05-02 at 13:25 -0700, Hugh Dickins wrote:
>> > Got it at last. =C2=A0Embarrassingly obvious. =C2=A0__rcu_read_lock() =
and
>> > __rcu_read_unlock() are not safe to be using __this_cpu operations,
>> > the cpu may change in between the rmw's read and write: they should
>> > be using this_cpu operations (or, I put preempt_disable/enable in the
>> > __rcu_read_unlock below). =C2=A0__this_cpus there work out fine on x86=
,
>> > which was given good instructions to use; but not so well on PowerPC.
>> >
>> > I've been running successfully for an hour now with the patch below;
>> > but I expect you'll want to consider the tradeoffs, and may choose a
>> > different solution.
>>
>> Didn't Linus recently rant about these __this_cpu vs this_cpu nonsense ?
>>
>> I thought that was going out..
>
> Linus did rant about __raw_get_cpu_var() because it cannot use the x86
> %fs segement overrides a bit more than a month ago. =C2=A0The __this_cpu
> stuff is useful if you have preemption disabled -- avoids the extra
> layer of preempt_disable().
>
> Or was this a different rant?
https://lkml.org/lkml/2011/11/29/321
I think it ended up with Christoph removing the more egregious
variants, but this_cpu_that and __this_cpu_the_other remaining.
Hugh
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Paul E. McKenney @ 2012-05-02 21:54 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Paul E. McKenney, linuxppc-dev, Hugh Dickins, linux-kernel
In-Reply-To: <1335993615.4088.1.camel@pasglop>
On Thu, May 03, 2012 at 07:20:15AM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2012-05-02 at 13:25 -0700, Hugh Dickins wrote:
> > Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> > __rcu_read_unlock() are not safe to be using __this_cpu operations,
> > the cpu may change in between the rmw's read and write: they should
> > be using this_cpu operations (or, I put preempt_disable/enable in the
> > __rcu_read_unlock below). __this_cpus there work out fine on x86,
> > which was given good instructions to use; but not so well on PowerPC.
> >
> > I've been running successfully for an hour now with the patch below;
> > but I expect you'll want to consider the tradeoffs, and may choose a
> > different solution.
>
> Didn't Linus recently rant about these __this_cpu vs this_cpu nonsense ?
>
> I thought that was going out..
Linus did rant about __raw_get_cpu_var() because it cannot use the x86
%fs segement overrides a bit more than a month ago. The __this_cpu
stuff is useful if you have preemption disabled -- avoids the extra
layer of preempt_disable().
Or was this a different rant?
Thanx, Paul
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Paul E. McKenney @ 2012-05-02 21:36 UTC (permalink / raw)
To: Hugh Dickins; +Cc: linuxppc-dev, linux-kernel, Paul E. McKenney
In-Reply-To: <20120502213238.GA12153@linux.vnet.ibm.com>
On Wed, May 02, 2012 at 02:32:38PM -0700, Paul E. McKenney wrote:
> On Wed, May 02, 2012 at 01:49:54PM -0700, Paul E. McKenney wrote:
> > On Wed, May 02, 2012 at 01:25:30PM -0700, Hugh Dickins wrote:
> > > On Tue, 1 May 2012, Paul E. McKenney wrote:
> > > > > > > > On Mon, 2012-04-30 at 15:37 -0700, Hugh Dickins wrote:
> > > > > > > > >
> > > > > > > > > BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
> > > > > > > > > in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
> > > > > > > > > Call Trace:
> > > > > > > > > [c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
> > > > > > > > > [c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
> > > > > > > > > [c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
> > > > > > > > > [c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
> > > > > > > > > [c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
> > > > > > > > > [c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
> > >
> > > Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> > > __rcu_read_unlock() are not safe to be using __this_cpu operations,
> > > the cpu may change in between the rmw's read and write: they should
> > > be using this_cpu operations (or, I put preempt_disable/enable in the
> > > __rcu_read_unlock below). __this_cpus there work out fine on x86,
> > > which was given good instructions to use; but not so well on PowerPC.
> >
> > Thank you very much for tracking this down!!!
> >
> > > I've been running successfully for an hour now with the patch below;
> > > but I expect you'll want to consider the tradeoffs, and may choose a
> > > different solution.
> >
> > The thing that puzzles me about this is that the normal path through
> > the scheduler would save and restore these per-CPU variables to and
> > from the task structure. There must be a sneak path through the
> > scheduler that I failed to account for.
>
> Sigh... I am slow today, I guess. The preemption could of course
> happen between the time that the task calculated the address of the
> per-CPU variable and the time that it modified it. If this happens,
> we are modifying some other CPU's per-CPU variable.
>
> Given that Linus saw no performance benefit from this patchset, I am
> strongly tempted to just drop this inlinable-__rcu_read_lock() series
> at this point.
>
> I suppose that the other option is to move preempt_count() to a
> per-CPU variable, then use the space in the task_info struct.
> But that didn't generate anywhere near as good of code...
But preempt_count() would suffer exactly the same problem. The address
is calculated, the task moves to some other CPU, and then the task
is messing with some other CPU's preemption counter. Blech.
Thanx, Paul
> > But given your good work, this should be easy for me to chase down
> > even on my x86-based laptop -- just convert from __this_cpu_inc() to a
> > read-inc-delay-write sequence. And check that the underlying variable
> > didn't change in the meantime. And dump an ftrace if it did. ;-)
> >
> > Thank you again, Hugh!
> >
> > Thanx, Paul
> >
> > > Hugh
> > >
> > > --- 3.4-rc4-next-20120427/include/linux/rcupdate.h 2012-04-28 09:26:38.000000000 -0700
> > > +++ testing/include/linux/rcupdate.h 2012-05-02 11:46:06.000000000 -0700
> > > @@ -159,7 +159,7 @@ DECLARE_PER_CPU(struct task_struct *, rc
> > > */
> > > static inline void __rcu_read_lock(void)
> > > {
> > > - __this_cpu_inc(rcu_read_lock_nesting);
> > > + this_cpu_inc(rcu_read_lock_nesting);
> > > barrier(); /* Keep code within RCU read-side critical section. */
> > > }
> > >
> > > --- 3.4-rc4-next-20120427/kernel/rcupdate.c 2012-04-28 09:26:40.000000000 -0700
> > > +++ testing/kernel/rcupdate.c 2012-05-02 11:44:13.000000000 -0700
> > > @@ -72,6 +72,7 @@ DEFINE_PER_CPU(struct task_struct *, rcu
> > > */
> > > void __rcu_read_unlock(void)
> > > {
> > > + preempt_disable();
> > > if (__this_cpu_read(rcu_read_lock_nesting) != 1)
> > > __this_cpu_dec(rcu_read_lock_nesting);
> > > else {
> > > @@ -83,13 +84,14 @@ void __rcu_read_unlock(void)
> > > barrier(); /* ->rcu_read_unlock_special load before assign */
> > > __this_cpu_write(rcu_read_lock_nesting, 0);
> > > }
> > > -#ifdef CONFIG_PROVE_LOCKING
> > > +#if 1 /* CONFIG_PROVE_LOCKING */
> > > {
> > > int rln = __this_cpu_read(rcu_read_lock_nesting);
> > >
> > > - WARN_ON_ONCE(rln < 0 && rln > INT_MIN / 2);
> > > + BUG_ON(rln < 0 && rln > INT_MIN / 2);
> > > }
> > > #endif /* #ifdef CONFIG_PROVE_LOCKING */
> > > + preempt_enable();
> > > }
> > > EXPORT_SYMBOL_GPL(__rcu_read_unlock);
> > >
> > > --- 3.4-rc4-next-20120427/kernel/sched/core.c 2012-04-28 09:26:40.000000000 -0700
> > > +++ testing/kernel/sched/core.c 2012-05-01 22:40:46.000000000 -0700
> > > @@ -2024,7 +2024,7 @@ asmlinkage void schedule_tail(struct tas
> > > {
> > > struct rq *rq = this_rq();
> > >
> > > - rcu_switch_from(prev);
> > > + /* rcu_switch_from(prev); */
> > > rcu_switch_to();
> > > finish_task_switch(rq, prev);
> > >
> > > @@ -7093,6 +7093,10 @@ void __might_sleep(const char *file, int
> > > "BUG: sleeping function called from invalid context at %s:%d\n",
> > > file, line);
> > > printk(KERN_ERR
> > > + "cpu=%d preempt_count=%x preempt_offset=%x rcu_nesting=%x nesting_save=%x\n",
> > > + raw_smp_processor_id(), preempt_count(), preempt_offset,
> > > + rcu_preempt_depth(), current->rcu_read_lock_nesting_save);
> > > + printk(KERN_ERR
> > > "in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n",
> > > in_atomic(), irqs_disabled(),
> > > current->pid, current->comm);
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at http://www.tux.org/lkml/
> > >
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Paul E. McKenney @ 2012-05-02 21:32 UTC (permalink / raw)
To: Hugh Dickins; +Cc: linuxppc-dev, linux-kernel, Paul E. McKenney
In-Reply-To: <20120502204954.GK2450@linux.vnet.ibm.com>
On Wed, May 02, 2012 at 01:49:54PM -0700, Paul E. McKenney wrote:
> On Wed, May 02, 2012 at 01:25:30PM -0700, Hugh Dickins wrote:
> > On Tue, 1 May 2012, Paul E. McKenney wrote:
> > > > > > > On Mon, 2012-04-30 at 15:37 -0700, Hugh Dickins wrote:
> > > > > > > >
> > > > > > > > BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
> > > > > > > > in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
> > > > > > > > Call Trace:
> > > > > > > > [c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
> > > > > > > > [c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
> > > > > > > > [c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
> > > > > > > > [c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
> > > > > > > > [c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
> > > > > > > > [c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
> >
> > Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> > __rcu_read_unlock() are not safe to be using __this_cpu operations,
> > the cpu may change in between the rmw's read and write: they should
> > be using this_cpu operations (or, I put preempt_disable/enable in the
> > __rcu_read_unlock below). __this_cpus there work out fine on x86,
> > which was given good instructions to use; but not so well on PowerPC.
>
> Thank you very much for tracking this down!!!
>
> > I've been running successfully for an hour now with the patch below;
> > but I expect you'll want to consider the tradeoffs, and may choose a
> > different solution.
>
> The thing that puzzles me about this is that the normal path through
> the scheduler would save and restore these per-CPU variables to and
> from the task structure. There must be a sneak path through the
> scheduler that I failed to account for.
Sigh... I am slow today, I guess. The preemption could of course
happen between the time that the task calculated the address of the
per-CPU variable and the time that it modified it. If this happens,
we are modifying some other CPU's per-CPU variable.
Given that Linus saw no performance benefit from this patchset, I am
strongly tempted to just drop this inlinable-__rcu_read_lock() series
at this point.
I suppose that the other option is to move preempt_count() to a
per-CPU variable, then use the space in the task_info struct.
But that didn't generate anywhere near as good of code...
Thanx, Paul
> But given your good work, this should be easy for me to chase down
> even on my x86-based laptop -- just convert from __this_cpu_inc() to a
> read-inc-delay-write sequence. And check that the underlying variable
> didn't change in the meantime. And dump an ftrace if it did. ;-)
>
> Thank you again, Hugh!
>
> Thanx, Paul
>
> > Hugh
> >
> > --- 3.4-rc4-next-20120427/include/linux/rcupdate.h 2012-04-28 09:26:38.000000000 -0700
> > +++ testing/include/linux/rcupdate.h 2012-05-02 11:46:06.000000000 -0700
> > @@ -159,7 +159,7 @@ DECLARE_PER_CPU(struct task_struct *, rc
> > */
> > static inline void __rcu_read_lock(void)
> > {
> > - __this_cpu_inc(rcu_read_lock_nesting);
> > + this_cpu_inc(rcu_read_lock_nesting);
> > barrier(); /* Keep code within RCU read-side critical section. */
> > }
> >
> > --- 3.4-rc4-next-20120427/kernel/rcupdate.c 2012-04-28 09:26:40.000000000 -0700
> > +++ testing/kernel/rcupdate.c 2012-05-02 11:44:13.000000000 -0700
> > @@ -72,6 +72,7 @@ DEFINE_PER_CPU(struct task_struct *, rcu
> > */
> > void __rcu_read_unlock(void)
> > {
> > + preempt_disable();
> > if (__this_cpu_read(rcu_read_lock_nesting) != 1)
> > __this_cpu_dec(rcu_read_lock_nesting);
> > else {
> > @@ -83,13 +84,14 @@ void __rcu_read_unlock(void)
> > barrier(); /* ->rcu_read_unlock_special load before assign */
> > __this_cpu_write(rcu_read_lock_nesting, 0);
> > }
> > -#ifdef CONFIG_PROVE_LOCKING
> > +#if 1 /* CONFIG_PROVE_LOCKING */
> > {
> > int rln = __this_cpu_read(rcu_read_lock_nesting);
> >
> > - WARN_ON_ONCE(rln < 0 && rln > INT_MIN / 2);
> > + BUG_ON(rln < 0 && rln > INT_MIN / 2);
> > }
> > #endif /* #ifdef CONFIG_PROVE_LOCKING */
> > + preempt_enable();
> > }
> > EXPORT_SYMBOL_GPL(__rcu_read_unlock);
> >
> > --- 3.4-rc4-next-20120427/kernel/sched/core.c 2012-04-28 09:26:40.000000000 -0700
> > +++ testing/kernel/sched/core.c 2012-05-01 22:40:46.000000000 -0700
> > @@ -2024,7 +2024,7 @@ asmlinkage void schedule_tail(struct tas
> > {
> > struct rq *rq = this_rq();
> >
> > - rcu_switch_from(prev);
> > + /* rcu_switch_from(prev); */
> > rcu_switch_to();
> > finish_task_switch(rq, prev);
> >
> > @@ -7093,6 +7093,10 @@ void __might_sleep(const char *file, int
> > "BUG: sleeping function called from invalid context at %s:%d\n",
> > file, line);
> > printk(KERN_ERR
> > + "cpu=%d preempt_count=%x preempt_offset=%x rcu_nesting=%x nesting_save=%x\n",
> > + raw_smp_processor_id(), preempt_count(), preempt_offset,
> > + rcu_preempt_depth(), current->rcu_read_lock_nesting_save);
> > + printk(KERN_ERR
> > "in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n",
> > in_atomic(), irqs_disabled(),
> > current->pid, current->comm);
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Benjamin Herrenschmidt @ 2012-05-02 21:20 UTC (permalink / raw)
To: Hugh Dickins
Cc: Paul E. McKenney, Paul E. McKenney, linuxppc-dev, linux-kernel
In-Reply-To: <alpine.LSU.2.00.1205021324430.24246@eggly.anvils>
On Wed, 2012-05-02 at 13:25 -0700, Hugh Dickins wrote:
> Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> __rcu_read_unlock() are not safe to be using __this_cpu operations,
> the cpu may change in between the rmw's read and write: they should
> be using this_cpu operations (or, I put preempt_disable/enable in the
> __rcu_read_unlock below). __this_cpus there work out fine on x86,
> which was given good instructions to use; but not so well on PowerPC.
>
> I've been running successfully for an hour now with the patch below;
> but I expect you'll want to consider the tradeoffs, and may choose a
> different solution.
Didn't Linus recently rant about these __this_cpu vs this_cpu nonsense ?
I thought that was going out..
Cheers,
Ben.
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Paul E. McKenney @ 2012-05-02 20:49 UTC (permalink / raw)
To: Hugh Dickins; +Cc: linuxppc-dev, linux-kernel, Paul E. McKenney
In-Reply-To: <alpine.LSU.2.00.1205021324430.24246@eggly.anvils>
On Wed, May 02, 2012 at 01:25:30PM -0700, Hugh Dickins wrote:
> On Tue, 1 May 2012, Paul E. McKenney wrote:
> > > > > > On Mon, 2012-04-30 at 15:37 -0700, Hugh Dickins wrote:
> > > > > > >
> > > > > > > BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
> > > > > > > in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
> > > > > > > Call Trace:
> > > > > > > [c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
> > > > > > > [c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
> > > > > > > [c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
> > > > > > > [c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
> > > > > > > [c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
> > > > > > > [c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
>
> Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> __rcu_read_unlock() are not safe to be using __this_cpu operations,
> the cpu may change in between the rmw's read and write: they should
> be using this_cpu operations (or, I put preempt_disable/enable in the
> __rcu_read_unlock below). __this_cpus there work out fine on x86,
> which was given good instructions to use; but not so well on PowerPC.
Thank you very much for tracking this down!!!
> I've been running successfully for an hour now with the patch below;
> but I expect you'll want to consider the tradeoffs, and may choose a
> different solution.
The thing that puzzles me about this is that the normal path through
the scheduler would save and restore these per-CPU variables to and
from the task structure. There must be a sneak path through the
scheduler that I failed to account for.
But given your good work, this should be easy for me to chase down
even on my x86-based laptop -- just convert from __this_cpu_inc() to a
read-inc-delay-write sequence. And check that the underlying variable
didn't change in the meantime. And dump an ftrace if it did. ;-)
Thank you again, Hugh!
Thanx, Paul
> Hugh
>
> --- 3.4-rc4-next-20120427/include/linux/rcupdate.h 2012-04-28 09:26:38.000000000 -0700
> +++ testing/include/linux/rcupdate.h 2012-05-02 11:46:06.000000000 -0700
> @@ -159,7 +159,7 @@ DECLARE_PER_CPU(struct task_struct *, rc
> */
> static inline void __rcu_read_lock(void)
> {
> - __this_cpu_inc(rcu_read_lock_nesting);
> + this_cpu_inc(rcu_read_lock_nesting);
> barrier(); /* Keep code within RCU read-side critical section. */
> }
>
> --- 3.4-rc4-next-20120427/kernel/rcupdate.c 2012-04-28 09:26:40.000000000 -0700
> +++ testing/kernel/rcupdate.c 2012-05-02 11:44:13.000000000 -0700
> @@ -72,6 +72,7 @@ DEFINE_PER_CPU(struct task_struct *, rcu
> */
> void __rcu_read_unlock(void)
> {
> + preempt_disable();
> if (__this_cpu_read(rcu_read_lock_nesting) != 1)
> __this_cpu_dec(rcu_read_lock_nesting);
> else {
> @@ -83,13 +84,14 @@ void __rcu_read_unlock(void)
> barrier(); /* ->rcu_read_unlock_special load before assign */
> __this_cpu_write(rcu_read_lock_nesting, 0);
> }
> -#ifdef CONFIG_PROVE_LOCKING
> +#if 1 /* CONFIG_PROVE_LOCKING */
> {
> int rln = __this_cpu_read(rcu_read_lock_nesting);
>
> - WARN_ON_ONCE(rln < 0 && rln > INT_MIN / 2);
> + BUG_ON(rln < 0 && rln > INT_MIN / 2);
> }
> #endif /* #ifdef CONFIG_PROVE_LOCKING */
> + preempt_enable();
> }
> EXPORT_SYMBOL_GPL(__rcu_read_unlock);
>
> --- 3.4-rc4-next-20120427/kernel/sched/core.c 2012-04-28 09:26:40.000000000 -0700
> +++ testing/kernel/sched/core.c 2012-05-01 22:40:46.000000000 -0700
> @@ -2024,7 +2024,7 @@ asmlinkage void schedule_tail(struct tas
> {
> struct rq *rq = this_rq();
>
> - rcu_switch_from(prev);
> + /* rcu_switch_from(prev); */
> rcu_switch_to();
> finish_task_switch(rq, prev);
>
> @@ -7093,6 +7093,10 @@ void __might_sleep(const char *file, int
> "BUG: sleeping function called from invalid context at %s:%d\n",
> file, line);
> printk(KERN_ERR
> + "cpu=%d preempt_count=%x preempt_offset=%x rcu_nesting=%x nesting_save=%x\n",
> + raw_smp_processor_id(), preempt_count(), preempt_offset,
> + rcu_preempt_depth(), current->rcu_read_lock_nesting_save);
> + printk(KERN_ERR
> "in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n",
> in_atomic(), irqs_disabled(),
> current->pid, current->comm);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Hugh Dickins @ 2012-05-02 20:25 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: linuxppc-dev, linux-kernel, Paul E. McKenney
In-Reply-To: <20120501232516.GR2441@linux.vnet.ibm.com>
On Tue, 1 May 2012, Paul E. McKenney wrote:
> > > > > On Mon, 2012-04-30 at 15:37 -0700, Hugh Dickins wrote:
> > > > > >
> > > > > > BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
> > > > > > in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
> > > > > > Call Trace:
> > > > > > [c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
> > > > > > [c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
> > > > > > [c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
> > > > > > [c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
> > > > > > [c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
> > > > > > [c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
Got it at last. Embarrassingly obvious. __rcu_read_lock() and
__rcu_read_unlock() are not safe to be using __this_cpu operations,
the cpu may change in between the rmw's read and write: they should
be using this_cpu operations (or, I put preempt_disable/enable in the
__rcu_read_unlock below). __this_cpus there work out fine on x86,
which was given good instructions to use; but not so well on PowerPC.
I've been running successfully for an hour now with the patch below;
but I expect you'll want to consider the tradeoffs, and may choose a
different solution.
Hugh
--- 3.4-rc4-next-20120427/include/linux/rcupdate.h 2012-04-28 09:26:38.000000000 -0700
+++ testing/include/linux/rcupdate.h 2012-05-02 11:46:06.000000000 -0700
@@ -159,7 +159,7 @@ DECLARE_PER_CPU(struct task_struct *, rc
*/
static inline void __rcu_read_lock(void)
{
- __this_cpu_inc(rcu_read_lock_nesting);
+ this_cpu_inc(rcu_read_lock_nesting);
barrier(); /* Keep code within RCU read-side critical section. */
}
--- 3.4-rc4-next-20120427/kernel/rcupdate.c 2012-04-28 09:26:40.000000000 -0700
+++ testing/kernel/rcupdate.c 2012-05-02 11:44:13.000000000 -0700
@@ -72,6 +72,7 @@ DEFINE_PER_CPU(struct task_struct *, rcu
*/
void __rcu_read_unlock(void)
{
+ preempt_disable();
if (__this_cpu_read(rcu_read_lock_nesting) != 1)
__this_cpu_dec(rcu_read_lock_nesting);
else {
@@ -83,13 +84,14 @@ void __rcu_read_unlock(void)
barrier(); /* ->rcu_read_unlock_special load before assign */
__this_cpu_write(rcu_read_lock_nesting, 0);
}
-#ifdef CONFIG_PROVE_LOCKING
+#if 1 /* CONFIG_PROVE_LOCKING */
{
int rln = __this_cpu_read(rcu_read_lock_nesting);
- WARN_ON_ONCE(rln < 0 && rln > INT_MIN / 2);
+ BUG_ON(rln < 0 && rln > INT_MIN / 2);
}
#endif /* #ifdef CONFIG_PROVE_LOCKING */
+ preempt_enable();
}
EXPORT_SYMBOL_GPL(__rcu_read_unlock);
--- 3.4-rc4-next-20120427/kernel/sched/core.c 2012-04-28 09:26:40.000000000 -0700
+++ testing/kernel/sched/core.c 2012-05-01 22:40:46.000000000 -0700
@@ -2024,7 +2024,7 @@ asmlinkage void schedule_tail(struct tas
{
struct rq *rq = this_rq();
- rcu_switch_from(prev);
+ /* rcu_switch_from(prev); */
rcu_switch_to();
finish_task_switch(rq, prev);
@@ -7093,6 +7093,10 @@ void __might_sleep(const char *file, int
"BUG: sleeping function called from invalid context at %s:%d\n",
file, line);
printk(KERN_ERR
+ "cpu=%d preempt_count=%x preempt_offset=%x rcu_nesting=%x nesting_save=%x\n",
+ raw_smp_processor_id(), preempt_count(), preempt_offset,
+ rcu_preempt_depth(), current->rcu_read_lock_nesting_save);
+ printk(KERN_ERR
"in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n",
in_atomic(), irqs_disabled(),
current->pid, current->comm);
^ permalink raw reply
* Please pull 'next' branch of 4xx tree
From: Josh Boyer @ 2012-05-02 13:51 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
Hi Ben,
A few patches from Suzie for 47x kexec/kdump support, and some MSI patches
from Mai La.
josh
The following changes since commit ec34a6814988f17506733c1e8b058ce46602591d:
powerpc: Remove old powerpc specific ptrace getregs/setregs calls
(2012-04-30 15:37:28 +1000)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/powerpc-4xx.git next
for you to fetch changes up to 9c6b2353dfb80ae843b831c03fc53ddc5c3949ff:
powerpc/44x: Add PCI MSI node for Maui APM821xx SoC and Bluestone
board in DTS (2012-05-03 08:58:21 -0400)
----------------------------------------------------------------
Mai La (2):
powerpc/44x: Fix PCI MSI support for Maui APM821xx SoC and Bluestone board
powerpc/44x: Add PCI MSI node for Maui APM821xx SoC and
Bluestone board in DTS
Suzuki Poulose (3):
powerpc/44x: Fix/Initialize PID to kernel PID before the TLB search
powerpc/47x: Kernel support for KEXEC
powerpc/47x: Enable CRASH_DUMP
arch/powerpc/Kconfig | 4 +-
arch/powerpc/boot/dts/bluestone.dts | 25 +++++
arch/powerpc/kernel/misc_32.S | 203 +++++++++++++++++++++++++++++++++--
arch/powerpc/platforms/44x/Kconfig | 2 +
arch/powerpc/sysdev/ppc4xx_msi.c | 42 +++++---
5 files changed, 252 insertions(+), 24 deletions(-)
^ permalink raw reply
* Re: [V2 2/5] powerpc: 40x: Add AHB, APB of_device_ids
From: Josh Boyer @ 2012-05-02 13:41 UTC (permalink / raw)
To: Tanmay Inamdar; +Cc: devicetree-discuss, linuxppc-dev, linux-kernel
In-Reply-To: <1333956052-25319-2-git-send-email-tinamdar@apm.com>
On Mon, Apr 9, 2012 at 3:20 AM, Tanmay Inamdar <tinamdar@apm.com> wrote:
> Adding of_device_id's for AHB and APB buses used in klondike (APM8018X) in
> platforms/40x/ppc40x_simple.c file
>
> Signed-off-by: Tanmay Inamdar <tinamdar@apm.com>
You should probably combine the first 3 patches in this series into one
patch. Otherwise you run the risk of changing the DTS to something the
kernel can't support. At the least, this change should be patch 1 in
the series, and you can probably combine the two DTS changes.
josh
^ permalink raw reply
* Re: [V2 5/5] powerpc: kernel: 16650 UART reg-shift support
From: Josh Boyer @ 2012-05-02 13:38 UTC (permalink / raw)
To: Tanmay Inamdar; +Cc: devicetree-discuss, linuxppc-dev, linux-kernel
In-Reply-To: <1333956052-25319-5-git-send-email-tinamdar@apm.com>
On Mon, Apr 9, 2012 at 3:20 AM, Tanmay Inamdar <tinamdar@apm.com> wrote:
> In APM8018X SOC, UART register address space has been relocated to 32-bit
> data boundaries for APB bus implementation.
> Current legacy_serial driver ignores the reg-shift property. This patch
> modifies legacy_serial.c and udbg_16550.c to work with above mentioned UA=
RTs.
>
> Signed-off-by: Tanmay Inamdar <tinamdar@apm.com>
> ---
> :100644 100644 8338aef... f5fc106... M =A0arch/powerpc/include/asm/udbg.h
> :100644 100644 bedd12e... d523b7d... M =A0arch/powerpc/kernel/legacy_seri=
al.c
> :100644 100644 6837f83... e0cb7dc... M =A0arch/powerpc/kernel/udbg_16550.=
c
> =A0arch/powerpc/include/asm/udbg.h =A0 =A0 | =A0 =A02 +-
> =A0arch/powerpc/kernel/legacy_serial.c | =A0 16 +++++---
> =A0arch/powerpc/kernel/udbg_16550.c =A0 =A0| =A0 64 +++++++++++++++++++++=
+------------
> =A03 files changed, 52 insertions(+), 30 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/udbg.h b/arch/powerpc/include/asm/u=
dbg.h
> index 8338aef..f5fc106 100644
> --- a/arch/powerpc/include/asm/udbg.h
> +++ b/arch/powerpc/include/asm/udbg.h
> @@ -29,7 +29,7 @@ extern void udbg_printf(const char *fmt, ...)
> =A0extern void udbg_progress(char *s, unsigned short hex);
>
> =A0extern void udbg_init_uart(void __iomem *comport, unsigned int speed,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned int clock);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned int clock, =
=A0unsigned int regshift);
> =A0extern unsigned int udbg_probe_uart_speed(void __iomem *comport,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0unsigned int clock);
>
> diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/le=
gacy_serial.c
> index bedd12e..d523b7d 100644
> --- a/arch/powerpc/kernel/legacy_serial.c
> +++ b/arch/powerpc/kernel/legacy_serial.c
> @@ -33,6 +33,7 @@ static struct legacy_serial_info {
> =A0 =A0 =A0 =A0unsigned int =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0clock;
> =A0 =A0 =A0 =A0int =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 irq_check_parent;
> =A0 =A0 =A0 =A0phys_addr_t =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 taddr;
> + =A0 =A0 =A0 unsigned int =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0regshif=
t;
> =A0} legacy_serial_infos[MAX_LEGACY_SERIAL_PORTS];
>
> =A0static struct __initdata of_device_id legacy_serial_parents[] =3D {
> @@ -42,6 +43,7 @@ static struct __initdata of_device_id legacy_serial_par=
ents[] =3D {
> =A0 =A0 =A0 =A0{.compatible =3D "ibm,opb",},
> =A0 =A0 =A0 =A0{.compatible =3D "simple-bus",},
> =A0 =A0 =A0 =A0{.compatible =3D "wrs,epld-localbus",},
> + =A0 =A0 =A0 {.compatible =3D "apm,apb",},
> =A0 =A0 =A0 =A0{},
> =A0};
>
> @@ -163,11 +165,6 @@ static int __init add_legacy_soc_port(struct device_=
node *np,
> =A0 =A0 =A0 =A0if (of_get_property(np, "clock-frequency", NULL) =3D=3D NU=
LL)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -1;
>
> - =A0 =A0 =A0 /* if reg-shift or offset, don't try to use it */
> - =A0 =A0 =A0 if ((of_get_property(np, "reg-shift", NULL) !=3D NULL) ||
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 (of_get_property(np, "reg-offset", NULL) !=
=3D NULL))
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -1;
> -
So we explicitly didn't support reg-shift before. I'm guessing there is
a reason for that, but I don't recall what. Ben?
Also, why do you need to use the legacy serial driver at all for this
SOC? As far as I remember, the OF serial driver should be sufficient.
> +static unsigned int reg_shift;
> +#define ns16550_offset(addr) (addr - (unsigned char *)udbg_comport)
> +
> =A0static struct NS16550 __iomem *udbg_comport;
>
> +static inline u8 serial_read(unsigned char *addr)
> +{
> + =A0 =A0 =A0 u32 offset =3D ns16550_offset(addr) << reg_shift;
> + =A0 =A0 =A0 return readb(udbg_comport + offset);
> +}
> +
> +static inline void serial_write(unsigned char *addr, char val)
> +{
> + =A0 =A0 =A0 u32 offset =3D ns16550_offset(addr) << reg_shift;
> + =A0 =A0 =A0 writeb(val, udbg_comport + offset);
> +}
> +
I don't think readb/writeb are correct here. Why did you switch to
using those instead of sticking with in_8/out_8?
josh
^ permalink raw reply
* [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Borislav Petkov @ 2012-05-02 13:30 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
Starting a new thread because the old one grew too big and
is out of my screen. Patch below is git-formatted from
git://git.infradead.org/users/mchehab/edac.git
> commit 447b7929e633027ffe131f2f8f246bba5690cee7
> Author: Mauro Carvalho Chehab <mchehab@redhat.com>
> Date: Wed Apr 18 15:20:50 2012 -0300
>
> edac: Change internal representation to work with layers
>
> Change the EDAC internal representation to work with non-csrow
> based memory controllers.
>
> There are lots of those memory controllers nowadays, and more
> are coming. So, the EDAC internal representation needs to be
> changed, in order to work with those memory controllers, while
> preserving backward compatibility with the old ones.
>
> The edac core was written with the idea that memory controllers
> are able to directly access csrows.
>
> This is not true for FB-DIMM and RAMBUS memory controllers.
>
> Also, some recent advanced memory controllers don't present a per-csrows
> view. Instead, they view memories as DIMMs, instead of ranks.
>
> So, change the allocation and error report routines to allow
> them to work with all types of architectures.
>
> This will allow the removal of several hacks with FB-DIMM and RAMBUS
> memory controllers.
>
> Also, several tests were done on different platforms using different
> x86 drivers.
>
> TODO: a multi-rank DIMMs are currently represented by multiple DIMM
> entries in struct dimm_info. That means that changing a label for one
> rank won't change the same label for the other ranks at the same DIMM.
> This bug is present since the beginning of the EDAC, so it is not a big
> deal. However, on several drivers, it is possible to fix this issue, but
> it should be a per-driver fix, as the csrow => DIMM arrangement may not
> be equal for all. So, don't try to fix it here yet.
>
> I tried to make this patch as short as possible, preceding it with
> several other patches that simplified the logic here. Yet, as the
> internal API changes, all drivers need changes. The changes are
> generally bigger in the drivers for FB-DIMMs.
<snip already reviewed stuff>
> /* Figure out the offsets of the various items from the start of an mc
> * structure. We want the alignment of each item to be at least as
> @@ -191,12 +253,28 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> * hardcode everything into a single struct.
> */
> mci = edac_align_ptr(&ptr, sizeof(*mci), 1);
> - csi = edac_align_ptr(&ptr, sizeof(*csi), nr_csrows);
> - chi = edac_align_ptr(&ptr, sizeof(*chi), nr_csrows * nr_chans);
> - dimm = edac_align_ptr(&ptr, sizeof(*dimm), nr_csrows * nr_chans);
> + layer = edac_align_ptr(&ptr, sizeof(*layer), n_layers);
> + csi = edac_align_ptr(&ptr, sizeof(*csi), tot_csrows);
> + chi = edac_align_ptr(&ptr, sizeof(*chi), tot_csrows * tot_channels);
> + dimm = edac_align_ptr(&ptr, sizeof(*dimm), tot_dimms);
> + count = 1;
> + for (i = 0; i < n_layers; i++) {
> + count *= layers[i].size;
> + debugf4("%s: errcount layer %d size %d\n", __func__, i, count);
> + ce_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
> + ue_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
> + tot_errcount += 2 * count;
> + }
> +
> + debugf4("%s: allocating %d error counters\n", __func__, tot_errcount);
> pvt = edac_align_ptr(&ptr, sz_pvt, 1);
> size = ((unsigned long)pvt) + sz_pvt;
>
> + debugf1("%s(): allocating %u bytes for mci data (%d %s, %d csrows/channels)\n",
> + __func__, size,
> + tot_dimms,
> + per_rank ? "ranks" : "dimms",
> + tot_csrows * tot_channels);
> mci = kzalloc(size, GFP_KERNEL);
> if (mci == NULL)
> return NULL;
> @@ -204,42 +282,101 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> /* Adjust pointers so they point within the memory we just allocated
> * rather than an imaginary chunk of memory located at address 0.
> */
> + layer = (struct edac_mc_layer *)(((char *)mci) + ((unsigned long)layer));
> csi = (struct csrow_info *)(((char *)mci) + ((unsigned long)csi));
> chi = (struct rank_info *)(((char *)mci) + ((unsigned long)chi));
> dimm = (struct dimm_info *)(((char *)mci) + ((unsigned long)dimm));
> + for (i = 0; i < n_layers; i++) {
> + mci->ce_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ce_per_layer[i]));
> + mci->ue_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ue_per_layer[i]));
> + }
> pvt = sz_pvt ? (((char *)mci) + ((unsigned long)pvt)) : NULL;
>
> /* setup index and various internal pointers */
> mci->mc_idx = edac_index;
> mci->csrows = csi;
> mci->dimms = dimm;
> + mci->tot_dimms = tot_dimms;
> mci->pvt_info = pvt;
> - mci->nr_csrows = nr_csrows;
> + mci->n_layers = n_layers;
> + mci->layers = layer;
> + memcpy(mci->layers, layers, sizeof(*layer) * n_layers);
> + mci->nr_csrows = tot_csrows;
> + mci->num_cschannel = tot_channels;
> + mci->mem_is_per_rank = per_rank;
>
> /*
> - * For now, assumes that a per-csrow arrangement for dimms.
> - * This will be latter changed.
> + * Fills the csrow struct
Fill
> */
> - dimm = mci->dimms;
> -
> - for (row = 0; row < nr_csrows; row++) {
> - csrow = &csi[row];
> - csrow->csrow_idx = row;
> - csrow->mci = mci;
> - csrow->nr_channels = nr_chans;
> - chp = &chi[row * nr_chans];
> - csrow->channels = chp;
> -
> - for (chn = 0; chn < nr_chans; chn++) {
> + for (row = 0; row < tot_csrows; row++) {
> + csr = &csi[row];
> + csr->csrow_idx = row;
> + csr->mci = mci;
> + csr->nr_channels = tot_channels;
> + chp = &chi[row * tot_channels];
> + csr->channels = chp;
> +
> + for (chn = 0; chn < tot_channels; chn++) {
> chan = &chp[chn];
> chan->chan_idx = chn;
> - chan->csrow = csrow;
> + chan->csrow = csr;
> + }
> + }
>
> - mci->csrows[row].channels[chn].dimm = dimm;
> - dimm->csrow = row;
> - dimm->csrow_channel = chn;
> - dimm++;
> - mci->nr_dimms++;
> + /*
> + * Fills the dimm struct
Fill
> + */
> + memset(&pos, 0, sizeof(pos));
> + row = 0;
> + chn = 0;
> + debugf4("%s: initializing %d %s\n", __func__, tot_dimms,
> + per_rank ? "ranks" : "dimms");
> + for (i = 0; i < tot_dimms; i++) {
> + chan = &csi[row].channels[chn];
> + dimm = EDAC_DIMM_PTR(layer, mci->dimms, n_layers,
> + pos[0], pos[1], pos[2]);
> + dimm->mci = mci;
> +
> + debugf2("%s: %d: %s%zd (%d:%d:%d): row %d, chan %d\n", __func__,
> + i, per_rank ? "rank" : "dimm", (dimm - mci->dimms),
> + pos[0], pos[1], pos[2], row, chn);
> +
> + /* Copy DIMM location */
> + for (j = 0; j < n_layers; j++)
> + dimm->location[j] = pos[j];
> +
> + /* Link it to the csrows old API data */
> + chan->dimm = dimm;
> + dimm->csrow = row;
> + dimm->cschannel = chn;
> +
> + /* Increment csrow location */
> + if (!rev_order) {
AFAICT, this rev_order is always false (in the final version of the
patches anyway) and if so, can be completely removed as an argument to
edac_mc_alloc() and also the code in the else-branch below.
> + for (j = n_layers - 1; j >= 0; j--)
> + if (!layers[j].is_virt_csrow)
> + break;
> + chn++;
> + if (chn == tot_channels) {
> + chn = 0;
> + row++;
> + }
> + } else {
> + for (j = n_layers - 1; j >= 0; j--)
> + if (layers[j].is_virt_csrow)
> + break;
> + row++;
> + if (row == tot_csrows) {
> + row = 0;
> + chn++;
> + }
> + }
> +
> + /* Increment dimm location */
> + for (j = n_layers - 1; j >= 0; j--) {
> + pos[j]++;
> + if (pos[j] < layers[j].size)
> + break;
> + pos[j] = 0;
> }
> }
>
> @@ -263,6 +400,57 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> */
> return mci;
> }
> +EXPORT_SYMBOL_GPL(new_edac_mc_alloc);
> +
> +/**
> + * edac_mc_alloc: Allocate and partially fills a struct mem_ctl_info structure
fill
> + * @edac_index: Memory controller number
How about 'mc_num' for a variable name instead then? I know, I know, it
was called like that originally but 'edac_index' is misleading.
> + * @n_layers: Nu
> +mber of layers at the MC hierarchy
needless '\n'
> + * layers: Describes each layer as seen by the Memory Controller
> + * @rev_order: Fills csrows/cs channels at the reverse order
you can drop that, as put above.
> + * @size_pvt: size of private storage needed
Capitalize: Size
> + *
> + *
> + * FIXME: drivers handle multi-rank memories on different ways: on some
in (remove "on")
> + * drivers, one multi-rank memory is mapped as one DIMM, while, on others,
and say: "Some drivers map multi-ranked DIMMs as one DIMM while others
as several DIMMs".
> + * a single multi-rank DIMM would be mapped into several "dimms".
> + *
> + * Non-csrow based drivers (like FB-DIMM and RAMBUS ones) will likely report
> + * such DIMMS properly, but the csrow-based ones will likely do the wrong
> + * thing, as two chip select values are used for dual-rank memories (and 4, for
> + * quad-rank ones). I suspect that this issue could be solved inside the EDAC
> + * core for SDRAM memories, but it requires further study at JEDEC JESD 21C.
This last paragraph sounds innacurately, especially the "likely" adverbs
make it even more confusing. The gist of what you're saying is already
present in the commit message anyway, so drop it here pls.
> + *
> + * In summary, solving this issue is not easy, as it requires a lot of testing.
Also superfluous and has nothing to do with edac_mc_alloc().
> + *
> + * Everything is kmalloc'ed as one big chunk - more efficient.
> + * Only can be used if all structures have the same lifetime - otherwisea
"It can only be used if ..."
> + * you have to allocate and initialize your own structures.
> + *
> + * Use edac_mc_free() to free mc structures allocated by this function.
> + *
> + * Returns:
> + * NULL allocation failed
> + * struct mem_ctl_info pointer
On failure: NULL
On success: struct mem_ctl_info.
> + */
> +
> +struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> + unsigned nr_chans, int edac_index)
> +{
> + unsigned n_layers = 2;
> + struct edac_mc_layer layers[n_layers];
> +
> + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> + layers[0].size = nr_csrows;
> + layers[0].is_virt_csrow = true;
> + layers[1].type = EDAC_MC_LAYER_CHANNEL;
> + layers[1].size = nr_chans;
> + layers[1].is_virt_csrow = false;
> +
> + return new_edac_mc_alloc(edac_index, ARRAY_SIZE(layers), layers,
> + false, sz_pvt);
> +}
> EXPORT_SYMBOL_GPL(edac_mc_alloc);
>
> /**
> @@ -528,7 +716,6 @@ EXPORT_SYMBOL(edac_mc_find);
> * edac_mc_add_mc: Insert the 'mci' structure into the mci global list and
> * create sysfs entries associated with mci structure
> * @mci: pointer to the mci structure to be added to the list
> - * @mc_idx: A unique numeric identifier to be assigned to the 'mci' structure.
> *
> * Return:
> * 0 Success
> @@ -555,6 +742,8 @@ int edac_mc_add_mc(struct mem_ctl_info *mci)
> edac_mc_dump_channel(&mci->csrows[i].
> channels[j]);
> }
> + for (i = 0; i < mci->tot_dimms; i++)
> + edac_mc_dump_dimm(&mci->dimms[i]);
> }
> #endif
> mutex_lock(&mem_ctls_mutex);
> @@ -712,261 +901,251 @@ int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci, unsigned long page)
> }
> EXPORT_SYMBOL_GPL(edac_mc_find_csrow_by_page);
>
> -/* FIXME - setable log (warning/emerg) levels */
> -/* FIXME - integrate with evlog: http://evlog.sourceforge.net/ */
> -void edac_mc_handle_ce(struct mem_ctl_info *mci,
> - unsigned long page_frame_number,
> - unsigned long offset_in_page, unsigned long syndrome,
> - int row, int channel, const char *msg)
> +const char *edac_layer_name[] = {
> + [EDAC_MC_LAYER_BRANCH] = "branch",
> + [EDAC_MC_LAYER_CHANNEL] = "channel",
> + [EDAC_MC_LAYER_SLOT] = "slot",
> + [EDAC_MC_LAYER_CHIP_SELECT] = "csrow",
> +};
> +EXPORT_SYMBOL_GPL(edac_layer_name);
> +
> +static void edac_increment_ce_error(struct mem_ctl_info *mci,
This could be abbreviated to edac_inc_ce_error()
> + bool enable_filter,
> + unsigned pos[EDAC_MAX_LAYERS])
Passing the whole array as an argument instead of only a pointer to it?
> {
> - unsigned long remapped_page;
> - char *label = NULL;
> - u32 grain;
> + int i, index = 0;
>
> - debugf3("MC%d: %s()\n", mci->mc_idx, __func__);
> + mci->ce_mc++;
>
> - /* FIXME - maybe make panic on INTERNAL ERROR an option */
> - if (row >= mci->nr_csrows || row < 0) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: row out of range "
> - "(%d >= %d)\n", row, mci->nr_csrows);
> - edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
> + if (!enable_filter) {
> + mci->ce_noinfo_count++;
> return;
> }
>
> - if (channel >= mci->csrows[row].nr_channels || channel < 0) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: channel out of range "
> - "(%d >= %d)\n", channel,
> - mci->csrows[row].nr_channels);
> - edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
> - return;
> - }
> -
> - label = mci->csrows[row].channels[channel].dimm->label;
> - grain = mci->csrows[row].channels[channel].dimm->grain;
> + for (i = 0; i < mci->n_layers; i++) {
> + if (pos[i] < 0)
> + break;
> + index += pos[i];
> + mci->ce_per_layer[i][index]++;
>
> - if (edac_mc_get_log_ce())
> - /* FIXME - put in DIMM location */
> - edac_mc_printk(mci, KERN_WARNING,
> - "CE page 0x%lx, offset 0x%lx, grain %d, syndrome "
> - "0x%lx, row %d, channel %d, label \"%s\": %s\n",
> - page_frame_number, offset_in_page,
> - grain, syndrome, row, channel,
> - label, msg);
> + if (i < mci->n_layers - 1)
> + index *= mci->layers[i + 1].size;
> + }
> +}
>
> - mci->ce_count++;
> - mci->csrows[row].ce_count++;
> - mci->csrows[row].channels[channel].dimm->ce_count++;
> - mci->csrows[row].channels[channel].ce_count++;
> +static void edac_increment_ue_error(struct mem_ctl_info *mci,
> + bool enable_filter,
> + unsigned pos[EDAC_MAX_LAYERS])
> +{
> + int i, index = 0;
>
> - if (mci->scrub_mode & SCRUB_SW_SRC) {
> - /*
> - * Some MC's can remap memory so that it is still available
> - * at a different address when PCI devices map into memory.
> - * MC's that can't do this lose the memory where PCI devices
> - * are mapped. This mapping is MC dependent and so we call
> - * back into the MC driver for it to map the MC page to
> - * a physical (CPU) page which can then be mapped to a virtual
> - * page - which can then be scrubbed.
> - */
> - remapped_page = mci->ctl_page_to_phys ?
> - mci->ctl_page_to_phys(mci, page_frame_number) :
> - page_frame_number;
> + mci->ue_mc++;
>
> - edac_mc_scrub_block(remapped_page, offset_in_page, grain);
> + if (!enable_filter) {
> + mci->ce_noinfo_count++;
> + return;
> }
> -}
> -EXPORT_SYMBOL_GPL(edac_mc_handle_ce);
>
> -void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci, const char *msg)
> -{
> - if (edac_mc_get_log_ce())
> - edac_mc_printk(mci, KERN_WARNING,
> - "CE - no information available: %s\n", msg);
> + for (i = 0; i < mci->n_layers; i++) {
> + if (pos[i] < 0)
> + break;
> + index += pos[i];
> + mci->ue_per_layer[i][index]++;
>
> - mci->ce_noinfo_count++;
> - mci->ce_count++;
> + if (i < mci->n_layers - 1)
> + index *= mci->layers[i + 1].size;
> + }
> }
> -EXPORT_SYMBOL_GPL(edac_mc_handle_ce_no_info);
>
> -void edac_mc_handle_ue(struct mem_ctl_info *mci,
> - unsigned long page_frame_number,
> - unsigned long offset_in_page, int row, const char *msg)
> +#define OTHER_LABEL " or "
> +void edac_mc_handle_error(const enum hw_event_mc_err_type type,
> + struct mem_ctl_info *mci,
> + const unsigned long page_frame_number,
> + const unsigned long offset_in_page,
> + const unsigned long syndrome,
> + const int layer0,
> + const int layer1,
> + const int layer2,
Instead of passing each layer as an arg, you can prepare the array pos[]
in each edac_mc_hanlde_*() and pass around a pointer to it - you need it
anyway in the edac_mc_inc*() functions.
> + const char *msg,
> + const char *other_detail,
> + const void *mcelog)
Also, this function is huuuge and begs to be splitted into small,
self-contained helpers.
> {
> - int len = EDAC_MC_LABEL_LEN * 4;
> - char labels[len + 1];
> - char *pos = labels;
> - int chan;
> - int chars;
> - char *label = NULL;
> + unsigned long remapped_page;
> + /* FIXME: too much for stack: move it to some pre-alocated area */
> + char detail[80], location[80];
> + char label[(EDAC_MC_LABEL_LEN + 1 + sizeof(OTHER_LABEL)) * mci->tot_dimms];
> + char *p;
> + int row = -1, chan = -1;
> + int pos[EDAC_MAX_LAYERS] = { layer0, layer1, layer2 };
> + int i;
> u32 grain;
> + bool enable_filter = false;
What does this enable_filter thing mean:
if (pos[i] >= 0)
enable_filter = true;
This absolutely needs explanation and better naming!
>
> debugf3("MC%d: %s()\n", mci->mc_idx, __func__);
>
> - /* FIXME - maybe make panic on INTERNAL ERROR an option */
> - if (row >= mci->nr_csrows || row < 0) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: row out of range "
> - "(%d >= %d)\n", row, mci->nr_csrows);
> - edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
> - return;
> - }
> -
> - grain = mci->csrows[row].channels[0].dimm->grain;
> - label = mci->csrows[row].channels[0].dimm->label;
> - chars = snprintf(pos, len + 1, "%s", label);
> - len -= chars;
> - pos += chars;
> -
> - for (chan = 1; (chan < mci->csrows[row].nr_channels) && (len > 0);
> - chan++) {
> - label = mci->csrows[row].channels[chan].dimm->label;
> - chars = snprintf(pos, len + 1, ":%s", label);
> - len -= chars;
> - pos += chars;
> + /* Check if the event report is consistent */
> + for (i = 0; i < mci->n_layers; i++) {
> + if (pos[i] >= (int)mci->layers[i].size) {
> + if (type == HW_EVENT_ERR_CORRECTED) {
> + p = "CE";
> + mci->ce_mc++;
> + } else {
> + p = "UE";
> + mci->ue_mc++;
Ok, mci->ce_mc and mci->ue_mc are being incremented here and the same
happens below again in the edac_inc*_{ce,ue}_error() functions below.
Why? Current layer is within valid elements count of current layer?
> + }
> + edac_mc_printk(mci, KERN_ERR,
> + "INTERNAL ERROR: %s value is out of range (%d >= %d)\n",
> + edac_layer_name[mci->layers[i].type],
> + pos[i], mci->layers[i].size);
> + /*
> + * Instead of just returning it, let's use what's
> + * known about the error. The increment routines and
> + * the DIMM filter logic will do the right thing by
> + * pointing the likely damaged DIMMs.
> + */
> + pos[i] = -1;
> + }
> + if (pos[i] >= 0)
> + enable_filter = true;
As above, what does that filter logic mean, where it is explained?
>
> - if (edac_mc_get_log_ue())
> - edac_mc_printk(mci, KERN_EMERG,
> - "UE page 0x%lx, offset 0x%lx, grain %d, row %d, "
> - "labels \"%s\": %s\n", page_frame_number,
> - offset_in_page, grain, row, labels, msg);
> -
> - if (edac_mc_get_panic_on_ue())
> - panic("EDAC MC%d: UE page 0x%lx, offset 0x%lx, grain %d, "
> - "row %d, labels \"%s\": %s\n", mci->mc_idx,
> - page_frame_number, offset_in_page,
> - grain, row, labels, msg);
> -
> - mci->ue_count++;
> - mci->csrows[row].ue_count++;
> -}
> -EXPORT_SYMBOL_GPL(edac_mc_handle_ue);
> + /*
> + * Get the dimm label/grain that applies to the match criteria.
> + * As the error algorithm may not be able to point to just one memory,
What exactly do you mean by "memory" here? DIMM, slot, rank? Please be
more specific.
> + * the logic here will get all possible labels that could pottentially
> + * be affected by the error.
> + * On FB-DIMM memory controllers, for uncorrected errors, it is common
> + * to have only the MC channel and the MC dimm (also called as "rank")
remove "as"
> + * but the channel is not known, as the memory is arranged in pairs,
> + * where each memory belongs to a separate channel within the same
> + * branch.
> + * It will also get the max grain, over the error match range
> + */
> + grain = 0;
> + p = label;
> + *p = '\0';
> + for (i = 0; i < mci->tot_dimms; i++) {
> + struct dimm_info *dimm = &mci->dimms[i];
>
> -void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci, const char *msg)
> -{
> - if (edac_mc_get_panic_on_ue())
> - panic("EDAC MC%d: Uncorrected Error", mci->mc_idx);
> + if (layer0 >= 0 && layer0 != dimm->location[0])
> + continue;
> + if (layer1 >= 0 && layer1 != dimm->location[1])
> + continue;
> + if (layer2 >= 0 && layer2 != dimm->location[2])
> + continue;
>
> - if (edac_mc_get_log_ue())
> - edac_mc_printk(mci, KERN_WARNING,
> - "UE - no information available: %s\n", msg);
> - mci->ue_noinfo_count++;
> - mci->ue_count++;
> -}
> -EXPORT_SYMBOL_GPL(edac_mc_handle_ue_no_info);
> + if (dimm->grain > grain)
> + grain = dimm->grain;
Pls move the "max grain" part of the comment over this lines so that one
knows what happens.
>
> -/*************************************************************
> - * On Fully Buffered DIMM modules, this help function is
> - * called to process UE events
> - */
> -void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci,
> - unsigned int csrow,
> - unsigned int channela,
> - unsigned int channelb, char *msg)
> -{
> - int len = EDAC_MC_LABEL_LEN * 4;
> - char labels[len + 1];
> - char *pos = labels;
> - int chars;
> - char *label;
> -
> - if (csrow >= mci->nr_csrows) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: row out of range (%d >= %d)\n",
> - csrow, mci->nr_csrows);
> - edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
> - return;
> + /*
> + * If the error is memory-controller wide, there's no sense
> + * on seeking for the affected DIMMs, as everything may be
"there's no need to seek for the affected DIMMs because the whole
channel/memory controller/... may be affected"
> + * affected. Also, don't show errors for non-filled dimm's.
for empty DIMM slots.
> + */
> + if (enable_filter && dimm->nr_pages) {
> + if (p != label) {
> + strcpy(p, OTHER_LABEL);
> + p += strlen(OTHER_LABEL);
> + }
> + strcpy(p, dimm->label);
> + p += strlen(p);
> + *p = '\0';
> +
> + /*
> + * get csrow/channel of the dimm, in order to allow
DIMM
> + * incrementing the compat API counters
> + */
> + debugf4("%s: %s csrows map: (%d,%d)\n",
> + __func__,
> + mci->mem_is_per_rank ? "rank" : "dimm",
> + dimm->csrow, dimm->cschannel);
newline
> + if (row == -1)
> + row = dimm->csrow;
> + else if (row >= 0 && row != dimm->csrow)
> + row = -2;
ditto
> + if (chan == -1)
> + chan = dimm->cschannel;
> + else if (chan >= 0 && chan != dimm->cschannel)
> + chan = -2;
> + }
> }
> -
> - if (channela >= mci->csrows[csrow].nr_channels) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: channel-a out of range "
> - "(%d >= %d)\n",
> - channela, mci->csrows[csrow].nr_channels);
> - edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
> - return;
newline here.
> + if (!enable_filter) {
> + strcpy(label, "any memory");
> + } else {
> + debugf4("%s: csrow/channel to increment: (%d,%d)\n",
> + __func__, row, chan);
> + if (p == label)
> + strcpy(label, "unknown memory");
> + if (type == HW_EVENT_ERR_CORRECTED) {
> + if (row >= 0) {
> + mci->csrows[row].ce_count++;
> + if (chan >= 0)
> + mci->csrows[row].channels[chan].ce_count++;
> + }
> + } else
> + if (row >= 0)
> + mci->csrows[row].ue_count++;
> }
>
> - if (channelb >= mci->csrows[csrow].nr_channels) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: channel-b out of range "
> - "(%d >= %d)\n",
> - channelb, mci->csrows[csrow].nr_channels);
> - edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
> - return;
> + /* Fill the RAM location data */
> + p = location;
> + for (i = 0; i < mci->n_layers; i++) {
> + if (pos[i] < 0)
> + continue;
newline.
> + p += sprintf(p, "%s %d ",
> + edac_layer_name[mci->layers[i].type],
> + pos[i]);
> }
>
> - mci->ue_count++;
> - mci->csrows[csrow].ue_count++;
> -
> - /* Generate the DIMM labels from the specified channels */
> - label = mci->csrows[csrow].channels[channela].dimm->label;
> - chars = snprintf(pos, len + 1, "%s", label);
> - len -= chars;
> - pos += chars;
> -
> - chars = snprintf(pos, len + 1, "-%s",
> - mci->csrows[csrow].channels[channelb].dimm->label);
> -
> - if (edac_mc_get_log_ue())
> - edac_mc_printk(mci, KERN_EMERG,
> - "UE row %d, channel-a= %d channel-b= %d "
> - "labels \"%s\": %s\n", csrow, channela, channelb,
> - labels, msg);
> -
> - if (edac_mc_get_panic_on_ue())
> - panic("UE row %d, channel-a= %d channel-b= %d "
> - "labels \"%s\": %s\n", csrow, channela,
> - channelb, labels, msg);
> -}
> -EXPORT_SYMBOL(edac_mc_handle_fbd_ue);
> + /* Memory type dependent details about the error */
> + if (type == HW_EVENT_ERR_CORRECTED)
> + snprintf(detail, sizeof(detail),
> + "page 0x%lx offset 0x%lx grain %d syndrome 0x%lx",
> + page_frame_number, offset_in_page,
> + grain, syndrome);
> + else
> + snprintf(detail, sizeof(detail),
> + "page 0x%lx offset 0x%lx grain %d",
> + page_frame_number, offset_in_page, grain);
> +
> + if (type == HW_EVENT_ERR_CORRECTED) {
> + if (edac_mc_get_log_ce())
> + edac_mc_printk(mci, KERN_WARNING,
> + "CE %s on %s (%s%s %s)\n",
> + msg, label, location,
> + detail, other_detail);
two back-to-back if-statements with the same conditional, pls merge
them. Better yet, this edac_mc_handle_error() is huuge, pls split its
functionality into well-abstracted helpers, for example one which deals
with HW_EVENT_ERR_CORRECTED, another with HW_EVENT_ERR_UNCORRECTED, etc.
> + edac_increment_ce_error(mci, enable_filter, pos);
> +
> + if (mci->scrub_mode & SCRUB_SW_SRC) {
> + /*
> + * Some MC's can remap memory so that it is still
memory controllers (called MCs below)
> + * available at a different address when PCI devices
> + * map into memory.
> + * MC's that can't do this lose the memory where PCI
this, lose...
> + * devices are mapped. This mapping is MC dependent
MC-dependent
> + * and so we call back into the MC driver for it to
> + * map the MC page to a physical (CPU) page which can
> + * then be mapped to a virtual page - which can then
> + * be scrubbed.
> + */
> + remapped_page = mci->ctl_page_to_phys ?
> + mci->ctl_page_to_phys(mci, page_frame_number) :
> + page_frame_number;
> +
> + edac_mc_scrub_block(remapped_page,
> + offset_in_page, grain);
> + }
The SCRUB_SW_SRC piece can be another function.
> + } else {
> + if (edac_mc_get_log_ue())
> + edac_mc_printk(mci, KERN_WARNING,
> + "UE %s on %s (%s%s %s)\n",
> + msg, label, location, detail, other_detail);
>
> -/*************************************************************
> - * On Fully Buffered DIMM modules, this help function is
> - * called to process CE events
> - */
> -void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci,
> - unsigned int csrow, unsigned int channel, char *msg)
> -{
> - char *label = NULL;
> + if (edac_mc_get_panic_on_ue())
> + panic("UE %s on %s (%s%s %s)\n",
> + msg, label, location, detail, other_detail);
>
> - /* Ensure boundary values */
> - if (csrow >= mci->nr_csrows) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: row out of range (%d >= %d)\n",
> - csrow, mci->nr_csrows);
> - edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
> - return;
> + edac_increment_ue_error(mci, enable_filter, pos);
> }
> - if (channel >= mci->csrows[csrow].nr_channels) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: channel out of range (%d >= %d)\n",
> - channel, mci->csrows[csrow].nr_channels);
> - edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
> - return;
> - }
> -
> - label = mci->csrows[csrow].channels[channel].dimm->label;
> -
> - if (edac_mc_get_log_ce())
> - /* FIXME - put in DIMM location */
> - edac_mc_printk(mci, KERN_WARNING,
> - "CE row %d, channel %d, label \"%s\": %s\n",
> - csrow, channel, label, msg);
> -
> - mci->ce_count++;
> - mci->csrows[csrow].ce_count++;
> - mci->csrows[csrow].channels[channel].dimm->ce_count++;
> - mci->csrows[csrow].channels[channel].ce_count++;
> }
> -EXPORT_SYMBOL(edac_mc_handle_fbd_ce);
> +EXPORT_SYMBOL_GPL(edac_mc_handle_error);
> diff --git a/include/linux/edac.h b/include/linux/edac.h
> index 3b8798d909da..2b66109bc301 100644
> --- a/include/linux/edac.h
> +++ b/include/linux/edac.h
> @@ -412,18 +412,20 @@ struct edac_mc_layer {
> /* FIXME: add the proper per-location error counts */
> struct dimm_info {
> char label[EDAC_MC_LABEL_LEN + 1]; /* DIMM label on motherboard */
> - unsigned memory_controller;
> - unsigned csrow;
> - unsigned csrow_channel;
> +
> + /* Memory location data */
> + unsigned location[EDAC_MAX_LAYERS];
> +
> + struct mem_ctl_info *mci; /* the parent */
>
> u32 grain; /* granularity of reported error in bytes */
> enum dev_type dtype; /* memory device type */
> enum mem_type mtype; /* memory dimm type */
> enum edac_type edac_mode; /* EDAC mode for this dimm */
>
> - u32 nr_pages; /* number of pages in csrow */
> + u32 nr_pages; /* number of pages on this dimm */
>
> - u32 ce_count; /* Correctable Errors for this dimm */
> + unsigned csrow, cschannel; /* Points to the old API data */
> };
>
> /**
> @@ -443,9 +445,10 @@ struct dimm_info {
> */
> struct rank_info {
> int chan_idx;
> - u32 ce_count;
> struct csrow_info *csrow;
> struct dimm_info *dimm;
> +
> + u32 ce_count; /* Correctable Errors for this csrow */
> };
>
> struct csrow_info {
> @@ -497,6 +500,11 @@ struct mcidev_sysfs_attribute {
> ssize_t (*store)(struct mem_ctl_info *, const char *,size_t);
> };
>
> +struct edac_hierarchy {
> + char *name;
> + unsigned nr;
> +};
What is that, unused leftovers?
> +
> /* MEMORY controller information structure
> */
> struct mem_ctl_info {
> @@ -541,13 +549,18 @@ struct mem_ctl_info {
> unsigned long (*ctl_page_to_phys) (struct mem_ctl_info * mci,
> unsigned long page);
> int mc_idx;
> - int nr_csrows;
> struct csrow_info *csrows;
> + unsigned nr_csrows, num_cschannel;
> +
> + /* Memory Controller hierarchy */
> + unsigned n_layers;
> + struct edac_mc_layer *layers;
> + bool mem_is_per_rank;
>
> /*
> * DIMM info. Will eventually remove the entire csrows_info some day
> */
> - unsigned nr_dimms;
> + unsigned tot_dimms;
> struct dimm_info *dimms;
>
> /*
> @@ -562,12 +575,15 @@ struct mem_ctl_info {
> const char *dev_name;
> char proc_name[MC_PROC_NAME_MAX_LEN + 1];
> void *pvt_info;
> - u32 ue_noinfo_count; /* Uncorrectable Errors w/o info */
> - u32 ce_noinfo_count; /* Correctable Errors w/o info */
> - u32 ue_count; /* Total Uncorrectable Errors for this MC */
> - u32 ce_count; /* Total Correctable Errors for this MC */
> + u32 ue_count; /* Total Uncorrectable Errors for this MC */
> + u32 ce_count; /* Total Correctable Errors for this MC */a
Why are you touching ue_count and ce_count, I don't see any differences
here?
> unsigned long start_time; /* mci load start time (in jiffies) */
>
> + /* drivers shouldn't access this struct directly */
Which struct, I only see unsigneds?
> + unsigned ce_noinfo_count, ue_noinfo_count;
> + unsigned ce_mc, ue_mc;
What are those counters?
> + u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
> +
> struct completion complete;
>
> /* edac sysfs device control */
> @@ -580,7 +596,7 @@ struct mem_ctl_info {
> * by the low level driver.
> *
> * Set by the low level driver to provide attributes at the
> - * controller level, same level as 'ue_count' and 'ce_count' above.
> + * controller level.
> * An array of structures, NULL terminated
> *
> * If attributes are desired, then set to array of attributes
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
^ permalink raw reply
* Re: Build regressions/improvements in v3.4-rc5
From: Geert Uytterhoeven @ 2012-05-02 9:39 UTC (permalink / raw)
To: Linux Kernel Development; +Cc: Chris Zankel, Linux/PPC Development
In-Reply-To: <alpine.DEB.2.00.1205021134570.1538@ayla.of.borg>
On Wed, 2 May 2012, Geert Uytterhoeven wrote:
> Below is the list of build error/warning regressions/improvements in
> v3.4-rc5[1] compared to v3.3[2].
>
> Too make this mail fit in the lkml limit, I deleted
> - 104 lines about __mcount_loc on sparc64
> - all warning improvements
>
> Summarized:
> - build errors: +107/-82
> - build warnings: +284/-999
>
> JFYI, when comparing v3.4-rc5 to v3.4-rc4[3], the summaries are:
> - build errors: +34/-2283
> - build warnings: +145/-681
powerpc-randconfig:
+ arch/powerpc/platforms/512x/mpc512x_shared.c: error: 'FSL_DIU_PORT_DVI' undeclared (first use in this function): => 189:9
+ arch/powerpc/platforms/512x/mpc512x_shared.c: error: parameter 1 ('port') has incomplete type: => 187:54, 83:56, 88:57, 69:56
+ arch/powerpc/platforms/512x/mpc512x_shared.c: error: return type is an incomplete type: => 187:1
+ drivers/virt/fsl_hypervisor.c: error: 'MSR_GS' undeclared (first use in this function): => 799:80
xtensa-allnoconfig:
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `__netif_schedule': => .text+0x48)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `alloc_etherdev_mqs': => .text+0x11c)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `consume_skb': => .text+0x4c)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `dev_alloc_skb': => .text+0x60)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `dev_close': => .text+0x84)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `eth_type_trans': => .text+0xac)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `eth_validate_addr': => .rodata+0x114)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `free_netdev': => .text+0x134)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `kfree_skb': => .text+0x70)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `netif_rx_ni': => .text+0x6c)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `register_netdevice': => .text+0x12c)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `rtnl_lock': => .text+0x128)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `rtnl_unlock': => .text+0x130)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `skb_put': => .text+0x64)
+ error: arch/xtensa/platforms/iss/built-in.o: undefined reference to `skb_trim': => .text+0x68)
+ error: kernel/built-in.o: undefined reference to `_sdata': => .text+0x8c0), .text+0x84c)
+ error: network.c: undefined reference to `__netif_schedule': => .text+0x3d7)
+ error: network.c: undefined reference to `alloc_etherdev_mqs': => .text+0x8c8)
+ error: network.c: undefined reference to `consume_skb': => .text+0x42e)
+ error: network.c: undefined reference to `dev_alloc_skb': => .text+0x4c8)
+ error: network.c: undefined reference to `dev_close': => .text+0x683)
+ error: network.c: undefined reference to `eth_type_trans': => .text+0x7db)
+ error: network.c: undefined reference to `free_netdev': => .text+0xad2)
+ error: network.c: undefined reference to `kfree_skb': => .text+0x548)
+ error: network.c: undefined reference to `netif_rx_ni': => .text+0x53e)
+ error: network.c: undefined reference to `register_netdevice': => .text+0xab7)
+ error: network.c: undefined reference to `rtnl_lock': => .text+0xaaf)
+ error: network.c: undefined reference to `rtnl_unlock': => .text+0xabf)
+ error: network.c: undefined reference to `skb_put': => .text+0x507)
+ error: network.c: undefined reference to `skb_trim': => .text+0x514)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Paul E. McKenney @ 2012-05-01 23:25 UTC (permalink / raw)
To: Hugh Dickins; +Cc: linuxppc-dev, linux-kernel, Paul E. McKenney
In-Reply-To: <alpine.LSU.2.00.1205011410360.28232@eggly.anvils>
On Tue, May 01, 2012 at 02:42:02PM -0700, Hugh Dickins wrote:
> On Tue, 1 May 2012, Paul E. McKenney wrote:
> > On Mon, Apr 30, 2012 at 10:10:06PM -0700, Hugh Dickins wrote:
> > > On Tue, 1 May 2012, Benjamin Herrenschmidt wrote:
> > > > On Mon, 2012-04-30 at 15:37 -0700, Hugh Dickins wrote:
> > > > >
> > > > > BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
> > > > > in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
> > > > > Call Trace:
> > > > > [c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
> > > > > [c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
> > > > > [c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
> > > > > [c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
> > > > > [c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
> > > > > [c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
> > > > >
>
> > My guess is that the following happened:
> >
> > 1. Task A is running, with its state in RCU's per-CPU variables.
> >
> > 2. Task A creates Task B and switches to it, but without invoking
> > schedule_tail() or schedule(). Task B is now running, with
> > Task A's state in RCU's per-CPU variables.
> >
> > 3. Task B switches context, saving Task A's per-CPU RCU variables
> > (with modifications by Task B, just for fun).
> >
> > 4. Task A starts running again, and loads obsolete versions of its
> > per-CPU RCU variables. This can cause rcu_read_unlock_do_special()
> > to be invoked at inappropriate times, which could cause
> > pretty arbitrary misbehavior.
> >
> > 5. Mismatched values for the RCU read-side nesting could cause
> > the read-side critical section to complete prematurely, which
> > could cause all manner of mischief. However, I would expect
> > this to trigger the WARN_ON_ONCE() in __rcu_read_unlock().
> >
> > Hmmm...
>
> I didn't find anything corresponding to that under arch/powerpc.
Nor did I. :-(
> There is something I found, that I had high hopes for, but sadly no,
> it does not fix it. I may be wrong, it's a long time since I thought
> about what happens in fork(). But I believe the rcu_switch_from(prev)
> you added to schedule_tail() is bogus: doesn't schedule_tail() more or
> less amount to a jump into schedule(), for the child to be as if it's
> emerging from the switch_to() in schedule()?
>
> So I think the rcu_switch_from(prev) has already been done by the prev
> task on the cpu, as it goes into switch_to() in schedule(). So at best
> you're duplicating that in schedule_tail(), and at worst (I don't know
> if the prev task can get far enough away for this to matter) you're
> messing with its state. Probably difficult to do any harm (those fields
> don't matter while it's on another cpu, and it has to get on another cpu
> for them to change), but it does look wrong to me.
I do believe that you are correct. /me wonders if it was really such
a good idea to tie RCU this closely to the scheduler...
I also agree that the chance of error is small. The parent would have
to be blocked for the child to have any probability of doing harm,
but we need the probability to be zero, which it does not appear to be.
I will semi-revert this change as you suggest.
> But commenting out that line did not fix my issues. And if you agree,
> you'll probably prefer, not to comment out the line, but revert back to
> rcu_switch_from(void) and just add the rcu_switch_to() to schedule_tail().
>
> Something else that I noticed in comments on rcu_switch_from() in
> sched.h (er, is sched.h really the right place for this code?): it says
> "if rcu_read_unlock_special is zero, then rcu_read_lock_nesting must
> also be zero" - shouldn't that say "non-zero" in each case?
No, but the variables should be reversed. It should read "Both
cases rely on the fact that if rcu_read_lock_nesting is zero, then
rcu_read_unlock_special must also be zero." Here are the two cases:
1. rcu_read_lock_nesting is zero: Then rcu_read_unlock_special
must also be zero, so there is no way to get to the
rcu_read_unlock_do_special() function. A scheduling-clock
interrupt might later set one of the rcu_read_unlock_special
bits, but only RCU_READ_UNLOCK_BLOCKED, which is OK because
rcu_read_unlock_do_special() does not mess with the scheduler
in this case.
2. rcu_read_lock_nesting is non-zero: Then the task is blocking
within an RCU read-side critical section, so none of the
scheduler's or architecture's read-side critical sections
can have the outermost rcu_read_unlock(), so the
rcu_read_unlock_do_special() function will not be invoked
in the first place.
Thank you for catching this!
> I must turn away from this right now, and come back to it later.
Thank you very much for all your help with this!
Thanx, Paul
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox