Re: [PATCH] Fix WARN_ON / WARN_ON_ONCE regression

[Andrew Morton <akpm@osdl.org>]

On Wed, 4 Oct 2006 12:47:00 -0400
Andrew James Wade <andrew.j.wade@gmail.com> wrote:

> On Tuesday 03 October 2006 23:32, Andrew Morton wrote:
> 
> > It might help, but we still don't know what's going on (I think).
> > 
> > I mean, if cache misses against __warn_once were sufficiently high for it
> > to affect performance, then __warn_once would be, err, in cache?
> 
> Yes, of course. I'm embarrassed.
> 
> I took a look at the generated code, and GCC is having difficulty
> optimizing WARN_ON_ONCE. Here is the start of __local_bh_enable:
> 
> 00000130 <__local_bh_enable>:
>  130:   83 ec 10                sub    $0x10,%esp
>  133:   8b 15 04 00 00 00       mov    0x4,%edx         <-+
>  139:   89 e0                   mov    %esp,%eax          |
>  13b:   25 00 e0 ff ff          and    $0xffffe000,%eax   | !!!
>  140:   8b 40 14                mov    0x14(%eax),%eax    |
>  143:   25 00 00 ff 0f          and    $0xfff0000,%eax    |

This is the evaluation of in_irq(): calculate `current', grab
current->thread_info->preempt_count.

Normally gcc does manage to CSE the value of current.

>  148:   85 d2                   test   %edx,%edx        <-+
>  14a:   74 04                   je     150 <__local_bh_enable+0x20>
>  14c:   85 c0                   test   %eax,%eax
>  14e:   75 35                   jne    185 <__local_bh_enable+0x55>
>  150:   89 e0                   mov    %esp,%eax
>  152:   8b 0d 00 00 00 00       mov    0x0,%ecx         <-+
>  158:   25 00 e0 ff ff          and    $0xffffe000,%eax   |

but this time it went and reevaluated it.

>  15d:   8b 40 14                mov    0x14(%eax),%eax    | !!!
>  160:   25 00 ff 00 00          and    $0xff00,%eax       |
>  165:   3d 00 01 00 00          cmp    $0x100,%eax        |
>  16a:   0f 94 c0                sete   %al                |
>  16d:   85 c9                   test   %ecx,%ecx        <-+
>  16f:   0f b6 c0                movzbl %al,%eax
>  172:   74 04                   je     178 <__local_bh_enable+0x48>
>  174:   85 c0                   test   %eax,%eax
>  176:   75 42                   jne    1ba <__local_bh_enable+0x8a>
>  178:   b8 00 01 00 00          mov    $0x100,%eax
>  17d:   83 c4 10                add    $0x10,%esp
>  180:   e9 fc ff ff ff          jmp    181 <__local_bh_enable+0x51>
>  185:   c7 44 24 0c 3e 00 00    movl   $0x3e,0xc(%esp)
>  ...
> 
> WARN_ON is better, but still has problems:
> 
> 000011a0 <do_exit>:
>     11a0:       55                      push   %ebp
>     11a1:       57                      push   %edi
>     11a2:       56                      push   %esi
>     11a3:       53                      push   %ebx
>     11a4:       83 ec 30                sub    $0x30,%esp
>     11a7:       89 44 24 18             mov    %eax,0x18(%esp)
>     11ab:       89 e0                   mov    %esp,%eax
>     11ad:       25 00 e0 ff ff          and    $0xffffe000,%eax
>     11b2:       8b 30                   mov    (%eax),%esi
>     11b4:       8b 86 58 0a 00 00       mov    0xa58(%esi),%eax
>     11ba:       89 44 24 2c             mov    %eax,0x2c(%esp)      <-+
>     11be:       8b 44 24 2c             mov    0x2c(%esp),%eax      <-+
>     11c2:       85 c0                   test   %eax,%eax              | !!!
>     11c4:       0f 85 65 07 00 00       jne    192f <do_exit+0x78f>   |
>     11ca:       8b 44 24 2c             mov    0x2c(%esp),%eax      <-+
>     11ce:       89 e0                   mov    %esp,%eax
>      ...

No, that's pretty much the same.  In the __local_bh_enable case we have the
evaluation of in_irq() as well as softirq_count().  do_exit() just has a
single WARN_ON().


> This is gcc (GCC) 4.0.3 (Ubuntu 4.0.3-1ubuntu5), -O2.
> 
> I don't know why this would show up as cache misses, but it does look
> like a compiler bug is being tickled.

The code could be better, but nope, there are no additional cache misses
there.

Still weird.