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]
Message-Id: <1154137151.19722.63.camel@localhost.localdomain>
Date:	Fri, 28 Jul 2006 21:39:11 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Chuck Ebbert <76306.1226@...puserve.com>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...l.org>,
	Linus Torvalds <torvalds@...l.org>
Subject: Re: [patch] i386: switch_to(): misplaced parentheses

On Tue, 2006-07-25 at 16:15 -0400, Chuck Ebbert wrote:
> Recent changes in i386 __switch_to() have a misplaced closing
> parenthesis causing an unlikely() to terminate early.
> 
> Signed-off-by: Chuck Ebbert <76306.1226@...puserve.com>
> 
> --- 2.6.18-rc2-32.orig/arch/i386/kernel/process.c
> +++ 2.6.18-rc2-32/arch/i386/kernel/process.c
> @@ -690,8 +690,8 @@ struct task_struct fastcall * __switch_t
>  	/*
>  	 * Now maybe handle debug registers and/or IO bitmaps
>  	 */
> -	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
> -	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
> +	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW)
> +	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP)))
>  		__switch_to_xtra(next_p, tss);
>  
>  	disable_tsc(prev_p, next_p);

Unlikely's with or's are kind of ambiguous.  An 'and' makes sense but
or's don't.  Because a branch is going to happen anyway.  Just to test
this out, I made a little function and tried out different types of
parenthesis placements.

Here: (the original placement)

	#define unlikely(x) __builtin_expect(!!(x), 0)

	int switch_to(int x, int y)
	{
		int a = 0;
		if (unlikely(x==24) || (y==83))
			a=10;
		return a;
	}


and now yours:

	#define unlikely(x) __builtin_expect(!!(x), 0)

	int switch_to(int x, int y)
	{
		int a = 0;
		if (unlikely(x==24 || y==83))
			a=10;
		return a;
	}

The original gave me this: gcc -O2 -c switch.c

objdump -Dr switch.o

00000000 <switch_to>:
   0:   55                      push   %ebp
   1:   89 e5                   mov    %esp,%ebp
   3:   83 7d 08 18             cmpl   $0x18,0x8(%ebp)
   7:   74 0a                   je     13 <switch_to+0x13>
   9:   31 c0                   xor    %eax,%eax
   b:   83 7d 0c 53             cmpl   $0x53,0xc(%ebp)
   f:   74 02                   je     13 <switch_to+0x13>
  11:   5d                      pop    %ebp
  12:   c3                      ret
  13:   5d                      pop    %ebp
  14:   b8 0a 00 00 00          mov    $0xa,%eax
  19:   c3                      ret


and then yours:  gcc -O2 -c switch_alt.c

objdump -Dr switch_alt.o

00000000 <switch_to>:
   0:   55                      push   %ebp
   1:   89 e5                   mov    %esp,%ebp
   3:   83 7d 08 18             cmpl   $0x18,0x8(%ebp)
   7:   74 0a                   je     13 <switch_to+0x13>
   9:   31 c0                   xor    %eax,%eax
   b:   83 7d 0c 53             cmpl   $0x53,0xc(%ebp)
   f:   74 02                   je     13 <switch_to+0x13>
  11:   5d                      pop    %ebp
  12:   c3                      ret
  13:   5d                      pop    %ebp
  14:   b8 0a 00 00 00          mov    $0xa,%eax
  19:   c3                      ret


They are identical!!!

This is not surprising since you still need to test the other OR case if
it fails, which you are saying it will.  So saying it is unlikely is
meaningless.  Likely's are good for 'or' and unlikely's are good for
'and'.  But not the other way around.

|| and && short circuit when they can.

So the statement of A || B is:

TEST A
JUMP_IF_TRUE true
TEST B
JUMP_IF_FALSE false
true:
 do true statement
JUMP out:
false:
 do else statement
out:

Saying TEST A is likely to fail really doesn't help the situation.
Saying it will likely pass will. Then we could do this:

TEST A
JUMP_IF_FALSE test2
true:
 do true statement
JUMP out:
false:
  do false statement
jump out:
test2:
TEST B
JUMP_IF_TRUE true
JUMP false:
out:

Where here we can move the true after the first compare and prevent the
conditional branch.

So what I'm saying is that unlikely(a || b) has really no good meaning.

Out of curiosity, I removed the unlikely altogether and here's what I
got.

gcc -O2 -c switch_none.c

00000000 <switch_to>:
   0:   55                      push   %ebp
   1:   89 e5                   mov    %esp,%ebp
   3:   83 7d 08 18             cmpl   $0x18,0x8(%ebp)
   7:   74 0a                   je     13 <switch_to+0x13>
   9:   31 c0                   xor    %eax,%eax
   b:   83 7d 0c 53             cmpl   $0x53,0xc(%ebp)
   f:   74 02                   je     13 <switch_to+0x13>
  11:   5d                      pop    %ebp
  12:   c3                      ret
  13:   5d                      pop    %ebp
  14:   b8 0a 00 00 00          mov    $0xa,%eax
  19:   c3                      ret


Ha! still the same.

Is that unlikely there really do any good?

-- Steve

-
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ