Messages in this thread Patch in this message | | | Date | Thu, 20 Aug 2009 08:12:20 +0100 | From | "Jan Beulich" <> | Subject | Re: [PATCH] x86: make use of inc/dec conditional |
| |
>>> "H. Peter Anvin" <hpa@zytor.com> 19.08.09 18:48 >>> >On 08/19/2009 12:48 AM, Jan Beulich wrote: >> According to gcc's instruction selection, inc/dec can be used without >> penalty on most CPU models, but should be avoided on others. Hence we >> should have a config option controlling the use of inc/dec, and >> respective abstraction macros to avoid making the resulting code too >> ugly. There are a few instances of inc/dec that must be retained in >> assembly code, due to that code's dependency on the instruction not >> changing the carry flag. > >One thing: I doubt it matters one measurable iota when it comes to >locked operations.
Okay, I think I agree to this point.
>Furthermore: > >- "decl %2 ;\n" >+ _ASM_DECL "%2 ;\n" > "jne 1b ;\n" > "adcl $0, %0 ;\n" > >It looks to me that the carry flag is live across the dec there. The
Indeed, I overlooked that when going through and checking for the CF-is-live instances.
>other csum code look scary to me too. > >The rest of them look technically okay, but you're bloating them by two >bytes (one byte in 64-bit mode) for every instance. You may want to >consider if any particular instance is more icache-critical than >stall-critical. This is probably more of a concern for inlines than for >regular single-instance code like the string operations.
So the background really is that I wanted to introduce a percpu_inc() operation subsequently (here with the goal to reduce code size by one byte in a couple of places - initially just for inc_irq_stat(), didn't look for other potential users), but then realized that it wouldn't be nice to unconditionally introduce a possible stall here. Hence I went and first created said config option, and then also went through and identified the uses of inc/dec that could be replaced based on that config option.
Jan
--- head-2009-07-28.orig/arch/x86/include/asm/hardirq.h 2009-08-10 14:39:11.000000000 +0200 +++ head-2009-07-28/arch/x86/include/asm/hardirq.h 2009-07-29 12:26:40.000000000 +0200 @@ -35,7 +35,7 @@ DECLARE_PER_CPU_SHARED_ALIGNED(irq_cpust #define __ARCH_IRQ_STAT -#define inc_irq_stat(member) percpu_add(irq_stat.member, 1) +#define inc_irq_stat(member) percpu_inc(irq_stat.member) #define local_softirq_pending() percpu_read(irq_stat.__softirq_pending) --- head-2009-07-28.orig/arch/x86/include/asm/percpu.h 2009-08-10 14:39:11.000000000 +0200 +++ head-2009-07-28/arch/x86/include/asm/percpu.h 2009-08-10 14:41:46.000000000 +0200 @@ -165,6 +165,29 @@ do { \ ret__; \ }) +#define percpu_unary_op(op, var) \ +({ \ + switch (sizeof(var)) { \ + case 1: \ + asm(op "b "__percpu_arg(0) \ + : "+m" (var)); \ + break; \ + case 2: \ + asm(op "w "__percpu_arg(0) \ + : "+m" (var)); \ + break; \ + case 4: \ + asm(op "l "__percpu_arg(0) \ + : "+m" (var)); \ + break; \ + case 8: \ + asm(op "q "__percpu_arg(0) \ + : "+m" (var)); \ + break; \ + default: __bad_percpu_size(); \ + } \ +}) + #define percpu_read(var) percpu_from_op("mov", per_cpu__##var) #define percpu_write(var, val) percpu_to_op("mov", per_cpu__##var, val) #define percpu_add(var, val) percpu_to_op("add", per_cpu__##var, val) @@ -176,6 +199,11 @@ do { \ #if defined(CONFIG_X86_XADD) || defined(CONFIG_X86_64) #define percpu_xadd(var, val) percpu_xchg_op("xadd", per_cpu__##var, val) #endif +#ifdef CONFIG_X86_INCDEC +#define percpu_inc(var) percpu_unary_op("inc", per_cpu__##var) +#else +#define percpu_inc(var) percpu_to_op("add", per_cpu__##var, 1) +#endif /* This is not atomic against other CPUs -- CPU preemption needs to be off */ #define x86_test_and_clear_bit_percpu(bit, var) \
| |