linux-kernel - Re: [PATCH 3/4, v2] x86: enlightenment for ticket spin locks

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:	Wed, 30 Jun 2010 08:07:17 +0100
From:	"Jan Beulich" <JBeulich@...ell.com>
To:	"H. Peter Anvin" <hpa@...or.com>
Cc:	<jeremy.fitzhardinge@...rix.com>, <mingo@...e.hu>,
	<tglx@...utronix.de>, "Ky Srinivasan" <KSrinivasan@...ell.com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4, v2] x86: enlightenment for ticket spin locks -
	 eliminate NOPs introduced by first patch

>>> On 30.06.10 at 03:13, "H. Peter Anvin" <hpa@...or.com> wrote:
> On 06/29/2010 07:33 AM, Jan Beulich wrote:
>> Under the assumption that the nop-s added by the base ticket spinlock
>> enlightenment patch might be considered undesirable (or worse), here
>> is an optional patch to eliminate these nop-s again. This is done
>> through extending the memory operands of the inc instructions used for
>> unlocking ticket locks to the necessary size, using assembler and
>> linker features.
>> 
>> --- 2.6.35-rc3-virt-spinlocks.orig/arch/x86/include/asm/spinlock.h
>> +++ 2.6.35-rc3-virt-spinlocks/arch/x86/include/asm/spinlock.h
>> @@ -10,7 +10,6 @@
>>  
>>  #ifdef CONFIG_ENLIGHTEN_SPINLOCKS
>>  #include <asm/alternative.h>
>> -#include <asm/nops.h>
>>  /* Including asm/smp.h here causes a cyclic include dependency. */
>>  #include <asm/percpu.h>
>>  DECLARE_PER_CPU(int, cpu_number);
>> @@ -156,8 +155,7 @@ static __always_inline void __ticket_spi
>>  #else
>>  	unsigned int token;
>>  
>> -	alternative_io(UNLOCK_LOCK_PREFIX "incb %[lock]\n\t"
>> -		ASM_NOP3,
>> +	alternative_io(UNLOCK_LOCK_PREFIX "unary incb %[lock]\n\t",
>>  		ALTERNATIVE_TICKET_UNLOCK_HEAD
>>  		UNLOCK_LOCK_PREFIX "incb %[lock]\n\t"
>>  		"movzwl %[lock], %[token]\n\t"
>> @@ -228,8 +226,7 @@ static __always_inline void __ticket_spi
>>  #else
>>  	unsigned int token, tmp;
>>  
>> -	alternative_io(UNLOCK_LOCK_PREFIX "incw %[lock]\n\t"
>> -		ASM_NOP2,
>> +	alternative_io(UNLOCK_LOCK_PREFIX "unary incw %[lock]\n\t",
>>  		ALTERNATIVE_TICKET_UNLOCK_HEAD
>>  		UNLOCK_LOCK_PREFIX "incw %[lock]\n\t"
>>  		"movl %[lock], %[token]\n\t"
> 
> If you're stretching (bloating) them anyway, perhaps we should be using
> "add" instructions instead, with their better EFLAGS behavior?

Hmm, yes, that possibility I didn't even consider. Would have
the potential to get away without that admittedly ugly "unary"
assembler macro altogether, though at the price of growing all
instructions rather than just those that have a non-symbolic
and small displacement. Since unlock generally gets inlined, I'm
not certain this additional growth in code size would be
acceptable...

Please let me know, though before submitting an eventual third
version I'd appreciate knowing especially the first two patches
need further changes in order to get accepted.

Thanks, Jan

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