linux-kernel - Re: [PATCH 0/7] preempt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 10 Sep 2013 18:45:19 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Ingo Molnar <mingo@...nel.org>, Andi Kleen <ak@...ux.intel.com>,
	Peter Anvin <hpa@...or.com>,
	Mike Galbraith <bitbucket@...ine.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Arjan van de Ven <arjan@...ux.intel.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>
Subject: Re: [PATCH 0/7] preempt_count rework -v2

On Tue, Sep 10, 2013 at 09:34:52AM -0700, Linus Torvalds wrote:
> On Tue, Sep 10, 2013 at 6:56 AM, Ingo Molnar <mingo@...nel.org> wrote:
> >
> > +static __always_inline bool __preempt_count_dec_and_test(void)
> > +{
> > +       unsigned char c;
> > +
> > +       asm ("decl " __percpu_arg(0) "; sete %1"
> > +                       : "+m" (__preempt_count), "=qm" (c));
> > +
> > +       return c != 0;
> > +}
> >
> > And that's where the sete and test originates from.
> 
> We could make this use "asm goto" instead.
> 
> An "asm goto" cannot have outputs, but this particular one doesn't
> _need_ outputs. You could mark the preempt_count memory as an input,
> and then have a memory clobber. I think you need the memory clobber
> anyway for that preempt-count thing.
> 
> So I _think_ something like
> 
> static __always_inline bool __preempt_count_dec_and_test(void)
> {
>        asm goto("decl " __percpu_arg(0) "\n\t"
>                 "je %l[became_zero]"
>                        : :"m" (__preempt_count):"memory":became_zero);
>        return 0;
> became_zero:
>        return 1;
> }

The usage site:

#define preempt_enable() \
do { \
	barrier(); \
	if (unlikely(preempt_count_dec_and_test())) \
		__preempt_schedule(); \
} while (0)

Already includes the barrier explicitly, so do we still need the memory
clobber in that asm goto thing?

That said, your change results in:

  ffffffff8106f420 <kick_process>:
  ffffffff8106f420:       55                      push   %rbp
  ffffffff8106f421:       65 ff 04 25 e0 b7 00    incl   %gs:0xb7e0
  ffffffff8106f428:       00 
  ffffffff8106f429:       48 89 e5                mov    %rsp,%rbp
  ffffffff8106f42c:       48 8b 47 08             mov    0x8(%rdi),%rax
  ffffffff8106f430:       8b 50 18                mov    0x18(%rax),%edx
  ffffffff8106f433:       65 8b 04 25 1c b0 00    mov    %gs:0xb01c,%eax
  ffffffff8106f43a:       00 
  ffffffff8106f43b:       39 c2                   cmp    %eax,%edx
  ffffffff8106f43d:       74 1b                   je     ffffffff8106f45a <kick_process+0x3a>
  ffffffff8106f43f:       89 d1                   mov    %edx,%ecx
  ffffffff8106f441:       48 c7 c0 00 2c 01 00    mov    $0x12c00,%rax
  ffffffff8106f448:       48 8b 0c cd a0 bc cb    mov    -0x7e344360(,%rcx,8),%rcx
  ffffffff8106f44f:       81 
  ffffffff8106f450:       48 3b bc 08 00 08 00    cmp    0x800(%rax,%rcx,1),%rdi
  ffffffff8106f457:       00 
  ffffffff8106f458:       74 26                   je     ffffffff8106f480 <kick_process+0x60>
* ffffffff8106f45a:       65 ff 0c 25 e0 b7 00    decl   %gs:0xb7e0
  ffffffff8106f461:       00 
* ffffffff8106f462:       74 0c                   je     ffffffff8106f470 <kick_process+0x50>
  ffffffff8106f464:       5d                      pop    %rbp
  ffffffff8106f465:       c3                      retq   
  ffffffff8106f466:       66 2e 0f 1f 84 00 00    nopw   %cs:0x0(%rax,%rax,1)
  ffffffff8106f46d:       00 00 00 
* ffffffff8106f470:       e8 9b b6 f9 ff          callq  ffffffff8100ab10 <___preempt_schedule>
  ffffffff8106f475:       5d                      pop    %rbp
  ffffffff8106f476:       c3                      retq   
  ffffffff8106f477:       66 0f 1f 84 00 00 00    nopw   0x0(%rax,%rax,1)
  ffffffff8106f47e:       00 00 
  ffffffff8106f480:       89 d7                   mov    %edx,%edi
  ffffffff8106f482:       ff 15 b8 e0 ba 00       callq  *0xbae0b8(%rip)        # ffffffff81c1d540 <smp_ops+0x20>
  ffffffff8106f488:       eb d0                   jmp    ffffffff8106f45a <kick_process+0x3a>
  ffffffff8106f48a:       66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)


Which is indeed perfect. So should I go 'fix' the other _and_test()
functions we have to do this same thing?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/