Re: [GIT PULL] percpu, cpumask, x86 updates for v2.6.30

Re: [GIT PULL] percpu, cpumask, x86 updates for v2.6.30

[Linus Torvalds]

On Sat, 28 Mar 2009, Ingo Molnar wrote:
> 
> Below is this tree merged against latest -git, resolving two new 
> conflicts. I forgot to mention that the percpu/MM changes are fine 
> by Andrew too.

Ingo, I think you ia64 stuff was broken. It used "vector" to get to the 
irq descriptor. Which is not the same as an "irq" at all.

The reason I noticed is that it clashed with the ia64 pull, which had 
fixed this up.

I tried to fix it all up. But I'm just about the last person in the 
universe who wants to compile ia64, so I have not tested, or even been 
able to see that it compiles.  Somebody should double-check it. 

		Linus

Re: [GIT PULL] percpu, cpumask, x86 updates for v2.6.30

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 28 Mar 2009, Ingo Molnar wrote:
> > 
> > Below is this tree merged against latest -git, resolving two new 
> > conflicts. I forgot to mention that the percpu/MM changes are 
> > fine by Andrew too.
> 
> Ingo, I think you ia64 stuff was broken. It used "vector" to get 
> to the irq descriptor. Which is not the same as an "irq" at all.
> 
> The reason I noticed is that it clashed with the ia64 pull, which 
> had fixed this up.
> 
> I tried to fix it all up. But I'm just about the last person in 
> the universe who wants to compile ia64, so I have not tested, or 
> even been able to see that it compiles.  Somebody should 
> double-check it.

Sure, i'm on it, i'll check your resolution of 
arch/ia64/kernel/irq_ia64.c.

	Ingo

Re: [GIT PULL] percpu, cpumask, x86 updates for v2.6.30

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > Ingo, I think you ia64 stuff was broken. It used "vector" to get 
> > to the irq descriptor. Which is not the same as an "irq" at all.
> > 
> > The reason I noticed is that it clashed with the ia64 pull, 
> > which had fixed this up.

that's the fallout of:

  d2287f5: irq: update all arches for new irq_desc, fix

it's this change:

        while (vector != IA64_SPURIOUS_INT_VECTOR) {
+               struct irq_desc *desc = irq_to_desc(vector);
+
                if (unlikely(IS_LOCAL_TLB_FLUSH(vector))) {
                        smp_local_flush_tlb();
-                       kstat_this_cpu.irqs[vector]++;
+                       kstat_incr_irqs_this_cpu(vector, desc);
                } else if (unlikely(IS_RESCHEDULE(vector)))
-                       kstat_this_cpu.irqs[vector]++;
+                       kstat_incr_irqs_this_cpu(vector, desc);

and the original code addressed kstat-irqs (which was a [NR_IRQS] 
array) via 'vector'.

And i think you are right that there is an irq<->vector mismatch 
here. I also think that ia64's use of [vector] there was probably 
buggy to begin with - the kstat array is really addressed by 'irq'.

With the irq_to_desc() change we did above it became even more 
incorrect.

It didnt crash though: this commit has been in linux-next for 2 
months. Maybe nobody noticed because it's "just" about IRQ 
statistics? I have no ia64 box to check though, and the code in 
arch/ia64/kernel/ivt.S looks rather cryptic.

> > 
> > I tried to fix it all up. But I'm just about the last person in 
> > the universe who wants to compile ia64, so I have not tested, or 
> > even been able to see that it compiles.  Somebody should 
> > double-check it.
> 
> Sure, i'm on it, i'll check your resolution of 
> arch/ia64/kernel/irq_ia64.c.

Your version builds fine on ia64 defconfig - and i think the 
resolution you did is correct.

Detail: i _think_ you could have picked the ia64-branch version 
instead of doing a manual resolution - i.e. could have kept the ia64 
version via the patch below. The reason is that IA64 does not 
support SPARSE_IRQ yet. But your resolution is cleaner and 
sparseirq-correct.

	Ingo

diff --git a/arch/ia64/kernel/irq_ia64.c b/arch/ia64/kernel/irq_ia64.c
index acc4d19..977a6ef 100644
--- a/arch/ia64/kernel/irq_ia64.c
+++ b/arch/ia64/kernel/irq_ia64.c
@@ -493,15 +493,16 @@ ia64_handle_irq (ia64_vector vector, struct pt_regs *regs)
 	saved_tpr = ia64_getreg(_IA64_REG_CR_TPR);
 	ia64_srlz_d();
 	while (vector != IA64_SPURIOUS_INT_VECTOR) {
+		struct irq_desc *desc;
 		int irq = local_vector_to_irq(vector);
-		struct irq_desc *desc = irq_to_desc(irq);
 
+		desc = irq_desc + irq;
 		if (unlikely(IS_LOCAL_TLB_FLUSH(vector))) {
 			smp_local_flush_tlb();
 			kstat_incr_irqs_this_cpu(irq, desc);
-		} else if (unlikely(IS_RESCHEDULE(vector))) {
+		} else if (unlikely(IS_RESCHEDULE(vector)))
 			kstat_incr_irqs_this_cpu(irq, desc);
-		} else {
+		else {
 			ia64_setreg(_IA64_REG_CR_TPR, vector);
 			ia64_srlz_d();
 
@@ -552,15 +553,16 @@ void ia64_process_pending_intr(void)
 	  * Perform normal interrupt style processing
 	  */
 	while (vector != IA64_SPURIOUS_INT_VECTOR) {
+		struct irq_desc *desc;
 		int irq = local_vector_to_irq(vector);
-		struct irq_desc *desc = irq_to_desc(irq);
+		desc = irq_desc + irq;
 
 		if (unlikely(IS_LOCAL_TLB_FLUSH(vector))) {
 			smp_local_flush_tlb();
 			kstat_incr_irqs_this_cpu(irq, desc);
-		} else if (unlikely(IS_RESCHEDULE(vector))) {
+		} else if (unlikely(IS_RESCHEDULE(vector)))
 			kstat_incr_irqs_this_cpu(irq, desc);
-		} else {
+		else {
 			struct pt_regs *old_regs = set_irq_regs(NULL);
 
 			ia64_setreg(_IA64_REG_CR_TPR, vector);

RE: [GIT PULL] percpu, cpumask, x86 updates for v2.6.30

[Luck, Tony]

Ingo,

Thanks for looking at this ... I'll try to do some sanity checks
on the IRQ stats to make sure that they make sense.

I do question this part:

> It didnt crash though: this commit has been in linux-next for 2
> months.

I've been building linux-next for ia64 at least a couple of
times a week ... and I didn't see the build error from these
changes there.  First time I saw it was when it showed up in Linus'
tree.  Can you check that all of your trees are in the list
to pull into linux-next?  Perhaps this one got missed?

-Tony

Re: [GIT PULL] percpu, cpumask, x86 updates for v2.6.30

[Ingo Molnar]

* Luck, Tony <tony.luck@intel.com> wrote:

> Ingo,
> 
> Thanks for looking at this ... I'll try to do some sanity checks
> on the IRQ stats to make sure that they make sense.
> 
> I do question this part:
> 
> > It didnt crash though: this commit has been in linux-next for 2 
> > months.
> 
> I've been building linux-next for ia64 at least a couple of times 
> a week ... and I didn't see the build error from these changes 
> there.  First time I saw it was when it showed up in Linus' tree.  
> Can you check that all of your trees are in the list to pull into 
> linux-next?  Perhaps this one got missed?

They are and were all there. There was no build problem here AFAICS 
- just a conflict which Linus saw and resolved [correctly AFAICS].

( And i build ia64 daily, against our full tree, so i'd have noticed
  any build breakage. )

If there was something here that shouldnt have happened then let me 
know and i'll trace back exactly how it happened and will prevent it 
in the future.

	Ingo

RE: [GIT PULL] percpu, cpumask, x86 updates for v2.6.30

[Luck, Tony]

> If there was something here that shouldnt have happened then let me
> know and i'll trace back exactly how it happened and will prevent it
> in the future.

Then perhaps it was the sequence in which things were released to
Linus?  Jes & both saw a problem in the ia64 build after pulling
from Linus on Friday.  He came up with a patch, which I incorporated
& sent to Linus[1] ... and thus the conflict occurred when more stuff
came from your tree.

So bisectability was broken by the sequence of patches that were
being fed into Linus' tree.

-Tony

[1] Note this is the only patch that I've submitted so far for
2.6.30 ... I have some other stuff that has been sitting in
linux-next ... but some of it isn't as well aged there as it
should be, so I'm holding it back for the latter part of the
merge window to make sure I don't break anything that has
followed better aging practices.