Hi Ingo,
Please consider for inclusion in your rt tree.
This series of patches fixes boot and runntime errors/warnings for
powerpc (esp. 64 bit). This applies to linux-2.6.20, patch-2.6.20-rt8
and previous my patch set;
http://ozlabs.org/pipermail/linuxppc-dev/2007-March/032640.htmlhttp://lkml.org/lkml/2007/3/6/503
Compile and boot tested on celleb (Cell Reference set) for both
PREEMPT_RT=y and PREEMPT_NONE=y.
CONFIG_MCOUNT, CONFIG_LATENCY_TRACE and other tracing options nor
CONFIG_GENERIC_TIME, clockevents etc are not yet ported.
Comments and suggestions are welcome.
Thanks in advance.
-- owa
TOSHIBA, Software Engineering Center.
@@ -94,8 +94,11 @@ static void pte_free_submit(struct pte_fvoidpgtable_free_tlb(structmmu_gather*tlb,pgtable_free_tpgf){-/* This is safe since tlb_gather_mmu has disabled preemption */-cpumask_tlocal_cpumask=cpumask_of_cpu(smp_processor_id());+/*+*Thisissafesincetlb_gather_mmuhasdisabledpreemption.+*tlb->cpuissetbytlb_gather_mmuaswell.+*/+cpumask_tlocal_cpumask=cpumask_of_cpu(tlb->cpu);structpte_freelist_batch**batchp=&__get_cpu_var(pte_freelist_cur);if(atomic_read(&tlb->mm->mm_users)<2||
To fix the following boot time error by removing ack member added by
the rt patch.
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Processor 1 found.
Brought up 2 CPUs
------------[ cut here ]------------
kernel BUG at arch/powerpc/platforms/cell/interrupt.c:86!
pu 0x1: Vector: 700 (Program Check) at [c00000000fff3c80]
pc: c000000000033f9c: .iic_eoi+0x58/0x64
lr: c00000000009add8: .handle_percpu_irq+0xd4/0xf4
sp: c00000000fff3f00
msr: 9000000000021032
current = 0xc000000000fee040
paca = 0xc000000000509e80
pid = 0, comm = swapper
kernel BUG at arch/powerpc/platforms/cell/interrupt.c:86!
enter ? for help
[link register ] c00000000009add8 .handle_percpu_irq+0xd4/0xf4
[c00000000fff3f00] c00000000009ada8 .handle_percpu_irq+0xa4/0xf4 (unreliable)
[c00000000fff3f90] c000000000023bb8 .call_handle_irq+0x1c/0x2c
[c000000000ff7950] c00000000000c910 .do_IRQ+0xf8/0x1b8
[c000000000ff79f0] c000000000034f34 .cbe_system_reset_exception+0x74/0xb4
[c000000000ff7a70] c000000000022610 .system_reset_exception+0x40/0xe0
[c000000000ff7af0] c000000000003378 system_reset_common+0xf8/0x100
--- Exception: 100 (System Reset) at c000000000035008 .cbe_power_save+0x94/0xb0
[c000000000ff7e70] c000000000012030 .cpu_idle+0xc8/0x144
[c000000000ff7f00] c000000000026894 .start_secondary+0x150/0x174
[c000000000ff7f90] c000000000008364 .start_secondary_prolog+0xc/0x10
1:mon>
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
I found a pile of e-mail started by Sergei Shtylyov on linuxppc-dev regarding this.
Subject: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers
From: Sergei Shtylyov [off-list ref]
Date: Sun, 19 Nov 2006 22:43:34 +0300
Though I don't quite get the conclusion, the code does not work at least
on celleb when handle_percpu_irq is applied. Since the handle_percpu_irq calls
both .ask and .eoi and when ask is set to iic_eoi, then iic_eoi() is called twice
for one interrupt. It hits BUG_ON(iic->eoi_ptr < 0)!
Anthor workaround could be to add one more irq_chip structure for handle_percpu_irq
which does not have ack member...
Any comments?
Signed-off-by: Tsutomu Owa <redacted>
-- owa
diff -rup linux-rt8/arch/powerpc/platforms/cell/interrupt.c rt/arch/powerpc/platforms/cell/interrupt.c
i'm not an xmon expert, but maybe it might make more sense to first
disable preemption, then interrupts - otherwise you could be preempted
right after having disabled these interrupts (and be scheduled to
another CPU, etc.). What is the difference between local_irq_save() and
the above 'disable interrupts' sequence? If it's not the same and
xmon_core() relied on having hardirqs disabled then it might make sense
to do a local_irq_save() there, instead of a preempt_disable().
Ingo
i'm not an xmon expert, but maybe it might make more sense to first
disable preemption, then interrupts - otherwise you could be preempted
right after having disabled these interrupts (and be scheduled to
another CPU, etc.). What is the difference between local_irq_save() and
the above 'disable interrupts' sequence? If it's not the same and
xmon_core() relied on having hardirqs disabled then it might make sense
to do a local_irq_save() there, instead of a preempt_disable().
powerpc 64 bits nowadays does lazy HW masking, so local_irq_disable()
will not actually switch MSR_EE off. However, xmon needs that to happen
(though we have a nicer accessor to do it, I suspect some bitrot need
fixing in there, possibly already fixed in .21)
I agree that preempt_disable() should be put before the MSR tweaking
though.
Ben.
i'm not an xmon expert, but maybe it might make more sense to first
disable preemption, then interrupts - otherwise you could be preempted
right after having disabled these interrupts (and be scheduled to
another CPU, etc.). What is the difference between local_irq_save() and
the above 'disable interrupts' sequence? If it's not the same and
xmon_core() relied on having hardirqs disabled then it might make sense
to do a local_irq_save() there, instead of a preempt_disable().
powerpc 64 bits nowadays does lazy HW masking, so local_irq_disable()
will not actually switch MSR_EE off. However, xmon needs that to happen
(though we have a nicer accessor to do it, I suspect some bitrot need
fixing in there, possibly already fixed in .21)
I agree that preempt_disable() should be put before the MSR tweaking
though.
i'm not an xmon expert, but maybe it might make more sense to first
disable preemption, then interrupts - otherwise you could be preempted
right after having disabled these interrupts (and be scheduled to
another CPU, etc.). What is the difference between local_irq_save() and
the above 'disable interrupts' sequence? If it's not the same and
xmon_core() relied on having hardirqs disabled then it might make sense
to do a local_irq_save() there, instead of a preempt_disable().
Since relatively recently, powerpc does no longer actually disable
the hardware interrupts with local_irq_disable(), but rather sets
a per-cpu flag that will be checked if an actual interrupt comes
in as part of the critical section.
The mtmsr() sequence in xmon corresponds to hard_irq_disable()
and should probably changed to that, but then you still need
the extra preempt_disable() / preempt_enable().
I think you're right about the sequence having to be
1. preempt_disable()
2. hard_irq_disable()
3.
4. hard_irq_enable()
5. preempt_enable()
Arnd <><
It was but not for 2.6.21 timeframe due to people lack of time/bandwidth
among others :-)
I intend to try to spend some time before the 2.6.22 merge window to
gather those -rt related patches that can be merged already and push
them to powerpc.git.
(Note to mingo: that means that we might end up with patches both in
your tree and being push via powerpc, I hope that's not too much of a
problem).
I can't promise I'll have time to do much, but I'd like to do it, so
find me on irc every now and then to "poke" if you don't see anything
happening...
Cheers,
Ben.
It was but not for 2.6.21 timeframe due to people lack of time/bandwidth
among others :-)
I was asking Ingo, basically. :-)
I intend to try to spend some time before the 2.6.22 merge window to
gather those -rt related patches that can be merged already and push
them to powerpc.git.
(Note to mingo: that means that we might end up with patches both in
your tree and being push via powerpc, I hope that's not too much of a
problem).
Well, this is happening every -rc1 now -- part of -rt getting merged to mainline.
I can't promise I'll have time to do much, but I'd like to do it, so
find me on irc every now and then to "poke" if you don't see anything
happening...
My purpose was only to get this into -rt patch for now.
Convert the spinlocks in the PowerPC interrupt related code into the raw ones,
also convert the PURR and PMC related spinlocks...
which says what you did, but gives NO CLUE about why this might be a
good thing to do - what problem it fixes or what desirable outcome it
produces. I will not apply patches with inadequate descriptions.
Paul.
Convert the spinlocks in the PowerPC interrupt related code into the raw ones,
also convert the PURR and PMC related spinlocks...
which says what you did, but gives NO CLUE about why this might be a
good thing to do - what problem it fixes or what desirable outcome it
produces. I will not apply patches with inadequate descriptions.
As I said, this was intended for the -rt patch, hence the question was for Ingo. I CC'ed the list just to keep people here in a loop.
From: Paul Mackerras <hidden> Date: 2007-03-07 19:42:07
Sergei Shtylyov writes:
As I said, this was intended for the -rt patch, hence the question was for
Ingo. I CC'ed the list just to keep people here in a loop.
OK, fair enough, but I still think the patch description was
inadequate. In the -rt context, I would at least expect to see some
explanation as to why those particular locks needed to be converted.
Paul.
From: Sergei Shtylyov <hidden> Date: 2007-03-07 21:21:41
Hello.
Paul Mackerras wrote:
quoted
As I said, this was intended for the -rt patch, hence the question was for Ingo. I CC'ed the list just to keep people here in a loop.
OK, fair enough, but I still think the patch description was
inadequate. In the -rt context, I would at least expect to see some
explanation as to why those particular locks needed to be converted.
I've floowed up to my patch with such explanation. In the context of an-rt patch itself, it was just too clear, hence I didn't go into explanations in the patch itself. :-)
--- Exception: 100 (System Reset) at c000000000035008 .cbe_power_save+0x94/0xb0
[c000000000ff7e70] c000000000012030 .cpu_idle+0xc8/0x144
[c000000000ff7f00] c000000000026894 .start_secondary+0x150/0x174
[c000000000ff7f90] c000000000008364 .start_secondary_prolog+0xc/0x10
1:mon>
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - I found a pile of e-mail started by Sergei Shtylyov on linuxppc-dev regarding this.
Subject: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers
From: Sergei Shtylyov [off-list ref]
Date: Sun, 19 Nov 2006 22:43:34 +0300
Though I don't quite get the conclusion, the code does not work at least
on celleb when handle_percpu_irq is applied. Since the handle_percpu_irq calls
Hmmm, I was under impression that I was fixing fasteoi flow case... Sorry if it broke something but it really shouldn't even have been there at all. :-/
both .ask and .eoi and when ask is set to iic_eoi, then iic_eoi() is called twice
for one interrupt. It hits BUG_ON(iic->eoi_ptr < 0)!
Anthor workaround could be to add one more irq_chip structure for handle_percpu_irq
which does not have ack member...
Any comments?
Well, I've told Ingo long ago that he shouldn't add that to the -rt patch (it's been refused from the very start but then got "restored" along with previously dropped genTOD patches).
From: Paul Mackerras <hidden> Date: 2007-03-07 21:31:46
Sergei Shtylyov writes:
I've floowed up to my patch with such explanation. In the context of an-rt
patch itself, it was just too clear, hence I didn't go into explanations in
the patch itself. :-)
Well, it might be clear, to you, now, with the context in your head.
But if such a patch is to go into a git tree, and somebody comes along
in 3 years time and wants to know exactly why you made that change
(and maybe that somebody is you :), then they will need more detail -
such as how you came to the conclusion that those locks and no others
needed to be changed, for instance.
At least give some of the reasoning behind your choice of which locks
to convert, so that in future, if the patch turns out to have
introduced a bug somehow, the person debugging it can either identify
that there was a flaw in your logic, or else understand something that
you have seen that they missed.
Paul.
From: Bill Huey (hui) <hidden> Date: 2007-03-08 00:44:09
On Thu, Mar 08, 2007 at 08:30:43AM +1100, Paul Mackerras wrote:
Sergei Shtylyov writes:
quoted
I've floowed up to my patch with such explanation. In the context of an-rt
patch itself, it was just too clear, hence I didn't go into explanations in
the patch itself. :-)
Well, it might be clear, to you, now, with the context in your head.
But if such a patch is to go into a git tree, and somebody comes along
in 3 years time and wants to know exactly why you made that change
(and maybe that somebody is you :), then they will need more detail -
such as how you came to the conclusion that those locks and no others
needed to be changed, for instance.
At least give some of the reasoning behind your choice of which locks
to convert, so that in future, if the patch turns out to have
introduced a bug somehow, the person debugging it can either identify
that there was a flaw in your logic, or else understand something that
you have seen that they missed.
Paul,
It has to do with how locking is done in the -rt patch itself. It's probably
before the time of general maintainers since the -rt patch hasn't been fully
merged, but I agree a document needs to be written outlining what needs to
be changed to spinlocks and what locks can be emulated with the rtmutex.c/rt.c
logic. There aren't that many people that know specifically unless they've
tried to map out chunks of the Linux kernel for this purpose in the first
place. I only know because of my own parallel effort to get the kernel to be
preemptive (the old mmLinux project that I abandoned for Ingo's stuff).
Generally, things that run within interrupt contexts need to be spinlocks.
The interrupt controller is one of those things obviously, the timer interrupt
for practical reasons such as performance and other places so that locking is
outside of direct control and scope of the scheduler. Of course the scheduler's
runqueues needs to be spinlocked for the reasons above otherwise your system
is stuck with a kind chicken and the egg problem interacting with the scheduler.
The places that need to be reverted to raw spinlocks are generally either
acquired by function calls that allocate the spinlock at a terminal of the
kernel's lock graph or isolated from other callers completely (parts of the
timer for logic for instance). It's all about the collision of various lock
(preemptive and non-preemptive) subtrees and how to avoid scheduling within
atomic violations that lead to deadlocks. The -rt patch gets arbitrary
preemption abilities by shrinking the non-preemptive sub-tree bit to the bare
essentials of what will let a system to run yet still preserve all of
the expected locking semantics of a critical section.
Otherwise everything by default is backed by a blocking rtmutex identity
to provide for correct preemptivity behavior within critical sections. That
is why these reverts are needed to restore the mathematical correctness of
the kernel's locking structures.
I hope this is helpful.
bill
At Wed, 07 Mar 2007 17:26:50 +0300,
Sergei Shtylyov wrote:
Tsutomu OWA wrote:
quoted
CONFIG_MCOUNT, CONFIG_LATENCY_TRACE and other tracing options nor
CONFIG_GENERIC_TIME,
There is PowerPC genTOD patch and it's incorporated into -rt (don't know
it works for Cell) but it breaks TOD vsyscalls. Several months ago I've posted
patches removing them for the time being:
quoted
clockevents etc are not yet ported.
Note that there *is* PowerPC clockevents driver already (don't know if it
works for Cell) -- it just never got merged to -rt:
I should have written like "... are not yet ported by myself."
anyway, thanks for the info.
-- owa
From: Paul Mackerras <hidden> Date: 2007-03-08 03:27:12
Bill Huey (hui) writes:
The places that need to be reverted to raw spinlocks are generally either
acquired by function calls that allocate the spinlock at a terminal of the
kernel's lock graph or isolated from other callers completely (parts of the
timer for logic for instance). It's all about the collision of various lock
(preemptive and non-preemptive) subtrees and how to avoid scheduling within
atomic violations that lead to deadlocks. The -rt patch gets arbitrary
preemption abilities by shrinking the non-preemptive sub-tree bit to the bare
essentials of what will let a system to run yet still preserve all of
the expected locking semantics of a critical section.
Thanks; that's an interesting explanation.
It misses the point of what I was saying to Sergei, though, which was
*not* "I don't understand your patch", it was "if this patch goes into
a git tree, someone coming along in 3 years time won't understand the
patch." In other words I was ranting about the need for a decent
description to accompany the patch itself, so it would go into the
permanent record.
Regards,
Paul.
From: Bill Huey (hui) <hidden> Date: 2007-03-08 04:01:13
On Thu, Mar 08, 2007 at 02:26:47PM +1100, Paul Mackerras wrote:
Bill Huey (hui) writes:
quoted
The places that need to be reverted to raw spinlocks are generally either
acquired by function calls that allocate the spinlock at a terminal of the
kernel's lock graph or isolated from other callers completely (parts of the
timer for logic for instance). It's all about the collision of various lock
(preemptive and non-preemptive) subtrees and how to avoid scheduling within
atomic violations that lead to deadlocks. The -rt patch gets arbitrary
preemption abilities by shrinking the non-preemptive sub-tree bit to the bare
essentials of what will let a system to run yet still preserve all of
the expected locking semantics of a critical section.
Thanks; that's an interesting explanation.
It misses the point of what I was saying to Sergei, though, which was
*not* "I don't understand your patch", it was "if this patch goes into
a git tree, someone coming along in 3 years time won't understand the
patch." In other words I was ranting about the need for a decent
description to accompany the patch itself, so it would go into the
permanent record.
Yeah, I think it's a a fear and uncertainly about the technical details about
the patch. That is why folks CC Ingo and company to get either a kind of
confirmation that this is ok along with comments. There are very few folks
that really understand the basic principals of the patch in this community
and that's not going to change any time soon. The mystery, paranoia (FUD)
and criticism surrounding it can make folks a bit shy.
I'll talk to you and Ben about it if we all get to OLS again. :)
bill
Argh, I've missed this one! :-(
But shouldn't we also add !need_resched_delayed() to another place below?
if (ppc_md.power_save) {
[...]
if (!need_resched() && !cpu_should_die())
WBR, Sergei
Hi,
At Fri, 16 Mar 2007 22:20:27 +0300,
Sergei Shtylyov wrote:
Argh, I've missed this one! :-(
But shouldn't we also add !need_resched_delayed() to another place below?
if (ppc_md.power_save) {
[...]
if (!need_resched() && !cpu_should_die())
Thanks for pointing it out. Yes, it looks like needed.
-- owa