Interrupt issue on m68k platform and some fix

Interrupt issue on m68k platform and some fix

[Lanttor]

[-- Attachment #1: Type: text/plain, Size: 5894 bytes --]

Hi,

I find one interrupt issue on my coldfire platform on linux-2.6.29
version (mcf54451evb board, mmu, NFS as rootfs).

After system boot up, I do following test:
on target board, run command: nc -l -p 3333 > /dev/null &
on host PC, run command: dd if=/dev/zero | nc 10.192.208.230 (target ip)
3333

and then I run top command on target board to check, which is shown as
follows:
CPU:  0.5% usr  0.0% sys  0.0% nice  0.0% idle  0.0% io 99.4% irq  0.0%
softirq
Load average: 0.39 0.10 0.03
  PID  PPID USER     STAT   VSZ %MEM %CPU COMMAND
  502   498 root     R     2560  2.0 97.8 nc -l -p 3333
    3     2 root     SW<      0  0.0  1.3 [ksoftirqd/0]
  503   498 root     R     2832  2.2  0.4 top
  492     1 root     S     3952  3.1  0.2 nmbd -s /etc/samba/smb.conf
  495     1 root     S     6144  4.8  0.0 smbd -s /etc/samba/smb.conf
  499   495 root     S     6144  4.8  0.0 smbd -s /etc/samba/smb.conf
  498     1 root     S     3056  2.4  0.0 -/bin/sh
  485     1 root     S     2864  2.2  0.0 /usr/sbin/inetd
    1     0 root     S     2824  2.2  0.0 init      
  468     1 root     S     2824  2.2  0.0 /sbin/syslogd
  470     1 root     S     2824  2.2  0.0 /sbin/klogd
  488     1 nobody   S     2672  2.1  0.0 /usr/sbin/boa -c /etc
  297     1 root     S <   2472  1.9  0.0 udevd --daemon
  282     2 root     SW<      0  0.0  0.0 [rpciod/0]
  143     2 root     SW<      0  0.0  0.0 [nfsiod]
    6     2 root     SW<      0  0.0  0.0 [khelper]
    2     0 root     SW<      0  0.0  0.0 [kthreadd]
    4     2 root     SW<      0  0.0  0.0 [watchdog/0]
    5     2 root     SW<      0  0.0  0.0 [events/0]
   53     2 root     SW<      0  0.0  0.0 [kblockd/0]

The irq time takes up most cpu time, but "softirq" and "sys" time is 0%.

I run /proc/stat to check, which is shown as follows:

[root@freescale /]# cat /proc/stat
cpu  316 0 0 53838 409 14467 0 0 0
cpu0 316 0 0 53838 409 14467 0 0 0
intr 636215 2 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 2 0 0 0 0 0
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 00
ctxt 281405
btime 0
processes 499
procs_running 2
procs_blocked 0

It's indeed that the statistic time number of "sys" and "softirq" is zero.


I tracked the source codes, and find the root cause is in following
codes (the Entry function for interrupt) in the entry.S:

ENTRY(inthandler)
        SAVE_ALL_INT
        GET_CURRENT(%d0)
       * addql   #1,%curptr@(TASK_INFO+TINFO_PREEMPT+1)*
        /* put exception # in d0 */
        movel   %sp@(PT_VECTOR),%d0
        swap    %d0                     /* extract bits 25:18 */
        lsrl    #2,%d0
        andl    #0x0ff,%d0

        movel   %sp,%sp@-
        movel   %d0,%sp@-               /* put vector # on stack */
auto_irqhandler_fixup = . + 2
        jbsr    process_int             /* process the IRQ */
        addql   #8,%sp                  /* pop parameters off stack */
        *subql   #1,%curptr@(TASK_INFO+TINFO_PREEMPT+1)*
        jeq     ret_from_last_interrupt
2:      RESTORE_ALL

        ALIGN
ret_from_last_interrupt:
        moveb   %sp@(PT_SR),%d0
        andl    #(~ALLOWINT>>8)&0xff,%d0
        jne     2b

        /* check if we need to do software interrupts */
        tstl    irq_stat+CPUSTAT_SOFTIRQ_PENDING
        jeq     .Lret_from_exception
        pea     ret_from_exception
        jra     do_softirq

My understanding of "*addql   #1,%curptr@(TASK_INFO+TINFO_PREEMPT+1)*"
is that its meaning is equal to
"current_thread_info()->preempt_count += HARDIRQ_OFFSET", which shows
now it's interrupt context. And the
meaning of "*subql   #1,%curptr@(TASK_INFO+TINFO_PREEMPT+1)*" should be
opposite.

However, it seems these two line codes doesn't take their responsibility
from above test result (I don't know why).
I comment these two lines and add "irq_enter() / irq_exit()" function
pairs into the process_int function (process_int() is the interrupt
handler).

My understanding the function of irq_enter()/irq_exit() is the same as
those two line assembly codes).  Attached is my patch.

Following is my test result with my patch:

on target board, run command: nc -l -p 3333 > /dev/null &
on host PC, run command: dd if=/dev/zero | nc 10.192.208.230 (target ip)
3333
run top command on target:
CPU:  0.9% usr 24.1% sys  0.0% nice  0.0% idle  0.0% io 18.2% irq 56.6%
softirq
Load average: 0.28 0.12 0.06
  PID  PPID USER     STAT   VSZ %MEM %CPU COMMAND
  498   495 root     R     2560  2.0 99.2 nc -l -p 3333
  499   495 root     R     2832  2.2  0.4 top
  489     1 root     S     3952  3.1  0.2 nmbd -s /etc/samba/smb.conf
  492     1 root     S     6144  4.8  0.0 smbd -s /etc/samba/smb.conf
  496   492 root     S     6144  4.8  0.0 smbd -s /etc/samba/smb.conf
  495     1 root     S     3056  2.4  0.0 -/bin/sh
  482     1 root     S     2864  2.2  0.0 /usr/sbin/inetd
    1     0 root     S     2824  2.2  0.0 init      
  465     1 root     S     2824  2.2  0.0 /sbin/syslogd
  467     1 root     S     2824  2.2  0.0 /sbin/klogd
  485     1 nobody   S     2672  2.1  0.0 /usr/sbin/boa -c /etc
  297     1 root     S <   2472  1.9  0.0 udevd --daemon
  282     2 root     SW<      0  0.0  0.0 [rpciod/0]
  143     2 root     SW<      0  0.0  0.0 [nfsiod]
    6     2 root     SW<      0  0.0  0.0 [khelper]
    2     0 root     SW<      0  0.0  0.0 [kthreadd]
    3     2 root     SW<      0  0.0  0.0 [ksoftirqd/0]
    4     2 root     SW<      0  0.0  0.0 [watchdog/0]
    5     2 root     SW<      0  0.0  0.0 [events/0]
   53     2 root     SW<      0  0.0  0.0 [kblockd/0]

run cat /proc/stat on target:
[root@freescale /]# cat /proc/stat
cpu  281 0 3565 34268 475 2597 7452 0 0
cpu0 281 0 3565 34268 475 2597 7452 0 0
intr 522453 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 0
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 00
ctxt 18910
btime 0
processes 497
procs_running 2
procs_blocked 0


Any idea for this issue?

Regards,
Lanttor



[-- Attachment #2: Fix-interrupt-contexting-bug-on-mcf5445x.patch --]
[-- Type: text/x-diff, Size: 1214 bytes --]

diff --git a/arch/m68k/coldfire/entry.S b/arch/m68k/coldfire/entry.S
index 3fc1df8..cfd9349 100644
--- a/arch/m68k/coldfire/entry.S
+++ b/arch/m68k/coldfire/entry.S
@@ -232,7 +232,6 @@ do_delayed_trace:
 ENTRY(inthandler)
 	SAVE_ALL_INT
 	GET_CURRENT(%d0)
-	addql	#1,%curptr@(TASK_INFO+TINFO_PREEMPT+1)
 	/* put exception # in d0 */
 	movel	%sp@(PT_VECTOR),%d0
 	swap	%d0			/* extract bits 25:18 */
@@ -244,7 +243,6 @@ ENTRY(inthandler)
 auto_irqhandler_fixup = . + 2
 	jbsr	process_int		/* process the IRQ */
 	addql	#8,%sp			/* pop parameters off stack */
-	subql	#1,%curptr@(TASK_INFO+TINFO_PREEMPT+1)
 	jeq	ret_from_last_interrupt
 2:	RESTORE_ALL
 
diff --git a/arch/m68k/coldfire/ints.c b/arch/m68k/coldfire/ints.c
index 3316463..85ce27e 100644
--- a/arch/m68k/coldfire/ints.c
+++ b/arch/m68k/coldfire/ints.c
@@ -100,6 +100,7 @@ asmlinkage void process_int(unsigned long vec, struct pt_regs *fp)
 	old_regs = set_irq_regs(fp);
 	kstat_cpu(0).irqs[vec]++;
 
+	irq_enter();
 	node = irq_list[vec];
 	if (!node)
 		handle_badint(fp);
@@ -109,6 +110,7 @@ asmlinkage void process_int(unsigned long vec, struct pt_regs *fp)
 			node = node->next;
 		} while (node);
 	}
+	irq_exit();
 
 	set_irq_regs(old_regs);
 }

Re: Interrupt issue on m68k platform and some fix

[Andreas Schwab]

Lanttor <lanttor.guo@freescale.com> writes:

> My understanding of "*addql   #1,%curptr@(TASK_INFO+TINFO_PREEMPT+1)*"
> is that its meaning is equal to
> "current_thread_info()->preempt_count += HARDIRQ_OFFSET",

No, it isn't.  It is equivalent to
*(long *)((char *)&current_thread_info()->preempt_count + 1) += 1
which is of course completely bogus.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Interrupt issue on m68k platform and some fix

[Lanttor]

>
> No, it isn't.  It is equivalent to
> *(long *)((char *)&current_thread_info()->preempt_count + 1) += 1
> which is of course completely bogus.
Sorry, I think I paste the redundant character for the codes (no '*'
char at front or end of addql and subql instruction)
I paste it again:

ENTRY(inthandler)
SAVE_ALL_INT
GET_CURRENT(%d0)
addql #1,%curptr@(TASK_INFO+TINFO_PREEMPT+1)
/* put exception # in d0 */
movel %sp@(PT_VECTOR),%d0
swap %d0 /* extract bits 25:18 */
lsrl #2,%d0
andl #0x0ff,%d0

movel %sp,%sp@-
movel %d0,%sp@- /* put vector # on stack */
auto_irqhandler_fixup = . + 2
jbsr process_int /* process the IRQ */
addql #8,%sp /* pop parameters off stack */
ret_from_interrupt:

subql #1,%curptr@(TASK_INFO+TINFO_PREEMPT+1)
jeq ret_from_last_interrupt
2: RESTORE_ALL

ALIGN
ret_from_last_interrupt:
moveb %sp@(PT_SR),%d0
andl #(~ALLOWINT>>8)&0xff,%d0
jne 2b

/* check if we need to do software interrupts */
tstl irq_stat+CPUSTAT_SOFTIRQ_PENDING
jeq .Lret_from_exception
pea ret_from_exception
jra do_softirq

For code "addql #1,%curptr@(TASK_INFO+TINFO_PREEMPT+1)",
My understanding is that
%curptr@(TASK_INFO+TINFO_PREEMPT) is the address of
current_thread_info()->preempt_count, and
%curptr@(TASK_INFO+TINFO_PREEMPT+1) point to the second byte of
preempt_count field (preempt_count field is 4 bytes).

So this code sets value 1 to the second byte of preempt_count field.

HARDIRQ_OFFSET equals (1 << HARDIRQ_SHIT) , and HARDIRQ_SHIFT value is
16, so I think this code equals
current_thread_info()->preempt_count += HARDIRQ_OFFSET

Am I wrong?

Regards,
Lanttor




------------------------------------------------------------------------
*From:* Andreas Schwab <schwab@linux-m68k.org>
*Sent:* 07/02/2009 4:24:35 PM +0800
*To:* Lanttor <lanttor.guo@freescale.com>
*CC:* linux-m68k@vger.kernel.org, uClinux development list
<uclinux-dev@uclinux.org>
*Subject:* Interrupt issue on m68k platform and some fix

> Lanttor <lanttor.guo@freescale.com> writes:
>
>   
>> My understanding of "*addql   #1,%curptr@(TASK_INFO+TINFO_PREEMPT+1)*"
>> is that its meaning is equal to
>> "current_thread_info()->preempt_count += HARDIRQ_OFFSET",
>>     
>
> No, it isn't.  It is equivalent to
> *(long *)((char *)&current_thread_info()->preempt_count + 1) += 1
> which is of course completely bogus.
>
> Andreas.
>
>   

Re: Interrupt issue on m68k platform and some fix

[Andreas Schwab]

Lanttor <lanttor.guo@freescale.com> writes:

> So this code sets value 1 to the second byte of preempt_count field.

No.  Byte would be addqb.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Interrupt issue on m68k platform and some fix

[Lanttor]

>
> No.  Byte would be addqb.
I think you're right, I am not familiar coldfire assembly and
instruction. I just touched it no long time.

addqb instruction is not supported on coldfire platform.

Which better way to replace it? I tried to use addql, but it proves that
it's incorrect.

I guess my patch could be better way to replace it now.

Regards,
Lanttor




------------------------------------------------------------------------
*From:* Andreas Schwab <schwab@linux-m68k.org>
*Sent:* 07/02/2009 5:05:59 PM +0800
*To:* Lanttor <lanttor.guo@freescale.com>
*CC:* linux-m68k@vger.kernel.org, uClinux development list
<uclinux-dev@uclinux.org>
*Subject:* Interrupt issue on m68k platform and some fix

> Lanttor <lanttor.guo@freescale.com> writes:
>
>   
>> So this code sets value 1 to the second byte of preempt_count field.
>>     
>
> No.  Byte would be addqb.
>
> Andreas.
>
>   

Re: Interrupt issue on m68k platform and some fix

[Roman Hodek]

> > So this code sets value 1 to the second byte of preempt_count field.
> 
> No.  Byte would be addqb.

Yep ;) But that's also not what's intended, I think.

Lanttor says he wants to add HARDIRQ_OFFSET:

> HARDIRQ_OFFSET equals (1 << HARDIRQ_SHIT) , and HARDIRQ_SHIFT value is
> 16, so I think this code equals

and 1<<16 == 0x00010000, so correct would be

  addl #0x00010000,%curptr@(TASK_INFO+TINFO_PREEMPT)

And this can be optimized to add 1 to the high word:

  addqw #1,%curptr@(TASK_INFO+TINFO_PREEMPT+2)

As 1 is small enough, you can use an addq statement, and the offset to
the memory location is 2.

The originally proposed "addlq #1, %curptr@(TASK_INFO+TINFO_PREEMPT+1)"
is misaligned and would case an exception. And "addlb #1,
%curptr@(TASK_INFO+TINFO_PREEMPT+1)" wouldn't yield the correct value
as long if the byte overflows ;)

> addqb instruction is not supported on coldfire platform.

Really? The addq instructions are contained in all kind of code, I
guess...

Roman

PS: Hi Andreas! It's quite some years ago, isn't it? ;)

Re: Interrupt issue on m68k platform and some fix

[Lanttor]

>
> and 1<<16 == 0x00010000, so correct would be
>
>   addl #0x00010000,%curptr@(TASK_INFO+TINFO_PREEMPT)
>
> And this can be optimized to add 1 to the high word:
>
>   addqw #1,%curptr@(TASK_INFO+TINFO_PREEMPT+2)
I checked the "Coldfire family programmer's Reference" and find that it
indeed does not support addqw and addqb instruction, it only support
addql instruction. And addl instruction can not be used for immediate data.

I guess coldfire instruction are not compatible m68k instruction.

So I need implement the function you mentioned above like this:

        movel   %curptr@(TASK_INFO+TINFO_PREEMPT),%d3
        addil   #0x10000,%d3
        movel   %d3,%curptr@(TASK_INFO+TINFO_PREEMPT)

I test it and it could work.

The other way to replace it is what I mentioned before: use irq_enter()
and irq_exit(), they take the same function.

> Really? The addq instructions are contained in all kind of code, I
> guess...
coldfire instruction are not compatible m68k instruction, so some m68k
kernel assembly code can not be used in coldfire platform directly.

Thanks,

Best Regards,
Lanttor




------------------------------------------------------------------------
*From:* roman@hodek.net (Roman Hodek)
*Sent:* 07/03/2009 4:25:32 AM +0800
*To:* Andreas Schwab <schwab@linux-m68k.org>
*CC:* lanttor.guo@freescale.com, linux-m68k@vger.kernel.org,
uclinux-dev@uclinux.org
*Subject:* Interrupt issue on m68k platform and some fix

>>> So this code sets value 1 to the second byte of preempt_count field.
>>>       
>> No.  Byte would be addqb.
>>     
>
> Yep ;) But that's also not what's intended, I think.
>
> Lanttor says he wants to add HARDIRQ_OFFSET:
>
>   
>> HARDIRQ_OFFSET equals (1 << HARDIRQ_SHIT) , and HARDIRQ_SHIFT value is
>> 16, so I think this code equals
>>     
>
> and 1<<16 == 0x00010000, so correct would be
>
>   addl #0x00010000,%curptr@(TASK_INFO+TINFO_PREEMPT)
>
> And this can be optimized to add 1 to the high word:
>
>   addqw #1,%curptr@(TASK_INFO+TINFO_PREEMPT+2)
>
> As 1 is small enough, you can use an addq statement, and the offset to
> the memory location is 2.
>
> The originally proposed "addlq #1, %curptr@(TASK_INFO+TINFO_PREEMPT+1)"
> is misaligned and would case an exception. And "addlb #1,
> %curptr@(TASK_INFO+TINFO_PREEMPT+1)" wouldn't yield the correct value
> as long if the byte overflows ;)
>
>   
>> addqb instruction is not supported on coldfire platform.
>>     
>
> Really? The addq instructions are contained in all kind of code, I
> guess...
>
> Roman
>
> PS: Hi Andreas! It's quite some years ago, isn't it? ;)
>
>