Re: [PATCH 00/10] jump label: introduce very_[un]likely + cleanups +docs

[Ingo Molnar]

* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Thu, Feb 23, 2012 at 2:02 AM, Ingo Molnar <mingo@xxxxxxx> wrote:
> >
> > Having to take the address gives us type safety - i.e. it 
> > will not be possible to accidentally pass in a 
> > non-jump-label key and get it misinterpreted.
>
> [...]
> 
> So rename it already.

No problem - it's all in a separate subject-to-rebasing branch, 
with the naming patches at the end (to allow the naming 
discussion play out) - which I've just dropped.

> Ingo, stop with the stupid denialism.
> 
> NOBODY likes that name. NOBODY. It's wrong. It's stupid. It 
> sounds like a stronger "unlikely()", and it simply IS NOT.

At the risk of being flamed some more, where does the confusion 
stem from? Is the source of the confusion the fact that we 
cannot write arbitrary C conditions inside the unlikely() 
portion [like we can do with a regular unlikely()] - but are 
forced to use a specific syntax, a 'key'?

Otherwise, call me stupid, but when I look at the disassembly 
then the defining characteristic of this construct is that it is 
a distinctly stronger unlikely() slowpath branch:

Before:

ffffffff810441f0 <sys_getppid>:
ffffffff810441f0:       8b 05 8a 52 d8 00       mov    0xd8528a(%rip),%eax        # ffffffff81dc9480 <key>
ffffffff810441f6:       55                      push   %rbp
ffffffff810441f7:       48 89 e5                mov    %rsp,%rbp
ffffffff810441fa:       85 c0                   test   %eax,%eax
ffffffff810441fc:       75 27                   jne    ffffffff81044225 <sys_getppid+0x35>
ffffffff810441fe:       65 48 8b 04 25 c0 b6    mov    %gs:0xb6c0,%rax
ffffffff81044205:       00 00
ffffffff81044207:       48 8b 80 80 02 00 00    mov    0x280(%rax),%rax
ffffffff8104420e:       48 8b 80 b0 02 00 00    mov    0x2b0(%rax),%rax
ffffffff81044215:       48 8b b8 e8 02 00 00    mov    0x2e8(%rax),%rdi
ffffffff8104421c:       e8 2f da 00 00          callq  ffffffff81051c50 <pid_vnr>
ffffffff81044221:       5d                      pop    %rbp
ffffffff81044222:       48 98                   cltq
ffffffff81044224:       c3                      retq
ffffffff81044225:       48 c7 c7 13 53 98 81    mov    $0xffffffff81985313,%rdi
ffffffff8104422c:       31 c0                   xor    %eax,%eax
ffffffff8104422e:       e8 60 0f 6d 00          callq  ffffffff81715193 <printk>
ffffffff81044233:       eb c9                   jmp    ffffffff810441fe <sys_getppid+0xe>
ffffffff81044235:       66 66 2e 0f 1f 84 00    data32 nopw %cs:0x0(%rax,%rax,1)
ffffffff8104423c:       00 00 00 00

After:

ffffffff81044290 <sys_getppid>:
ffffffff81044290:       55                      push   %rbp
ffffffff81044291:       48 89 e5                mov    %rsp,%rbp
ffffffff81044294:       e9 00 00 00 00          jmpq   ffffffff81044299 <sys_getppid+0x9>
ffffffff81044299:       65 48 8b 04 25 c0 b6    mov    %gs:0xb6c0,%rax
ffffffff810442a0:       00 00
ffffffff810442a2:       48 8b 80 80 02 00 00    mov    0x280(%rax),%rax
ffffffff810442a9:       48 8b 80 b0 02 00 00    mov    0x2b0(%rax),%rax
ffffffff810442b0:       48 8b b8 e8 02 00 00    mov    0x2e8(%rax),%rdi
ffffffff810442b7:       e8 f4 d9 00 00          callq  ffffffff81051cb0 <pid_vnr>
ffffffff810442bc:       5d                      pop    %rbp
ffffffff810442bd:       48 98                   cltq
ffffffff810442bf:       c3                      retq
ffffffff810442c0:       48 c7 c7 e3 54 98 81    mov    $0xffffffff819854e3,%rdi
ffffffff810442c7:       31 c0                   xor    %eax,%eax
ffffffff810442c9:       e8 71 13 6d 00          callq  ffffffff8171563f <printk>
ffffffff810442ce:       eb c9                   jmp    ffffffff81044299 <sys_getppid+0x9>

The prior slowpath test:

ffffffff810441fa:       85 c0                   test   %eax,%eax
ffffffff810441fc:       75 27                   jne    ffffffff81044225 <sys_getppid+0x35>

became even faster and turned into a single NOP, making the 
fastpath even faster.

> The "type safety" argument seems bogus too. As far as I can 
> tell, it fails miserably if you test a void pointer for being 
> NULL.
> 
> Sure, you can fix that by doing crazy things to the interface, 
> but in the end, nothing changes the fact that 
> "very_unlikely()" as a name sounds like an emphatic version of 
> "unlikely()".
> 
> Rename it. Make it clear from the name that it is about these 
> static variables. Everything else seems to be named for that 
> "static_key" thing, so make the testing of it be named that 
> way too, instead of using a bad generic naming scheme that is 
> just confusing.

Well, the static_key naming is new, it came from me in the last 
patch (full patch attached below), so I guess you only disagree 
with the very_unlikely()/very_likely() confusion bit, but not 
with the static_key renaming?

My main gripe was the jump-label naming, which was a deeply 
implementational name that was actively confusing (to me).

So, a modified scheme would be:

        #include <linux/static_key.h>

        struct static_key key = STATIC_KEY_INIT_TRUE;

        if (static_key_false(&key))
                do unlikely code
        else
                do likely code

Or:

        if (static_key_true(&key))
                do likely code
        else
                do unlikely code

The static key is modified via:

        static_key_slow_inc(&key);
        ...
        static_key_slow_dec(&key);

Is that API fine? I'll rework the series to such an effect if 
everyone agrees.

Thanks,

	Ingo