x86: percpu_to_op() misses memory and flags clobbers
Subsystems (5)
PATCH 1/1 x86: percpu_to_op() misses memory and flags clobbers
While playing with new percpu_{read|write|add|sub} stuff in network tree, I found x86 asm was a litle bit optimistic.
We need to tell gcc that percpu_{write|add|sub|or|xor} are modyfing memory and possibly eflags. We could add another parameter to percpu_to_op() to separate the plain "mov" case (not changing eflags),
but let keep it simple for the moment.
arch/x86/include/asm/percpu.h +8 -4
@@ -82,22 +82,26 @@ do { \case 1 : \ asm(op "b %1,"__percpu_arg(0) \ : "+m" (var) \ -: " ri" ((T__)val)); \ +: " ri" ((T__)val) \ +: " memory", " cc"); \ break; \ case 2: \ asm(op " w %1 ,"__percpu_arg(0) \ : " +m" (var) \ -: " ri" ((T__)val)); \ +: " ri" ((T__)val) \ +: " memory", " cc"); \ break; \ case 4: \ asm(op " l %1 ,"__percpu_arg(0) \ : " +m" (var) \ -: " ri" ((T__)val)); \ +: " ri" ((T__)val) \ +: " memory", " cc"); \ break; \ case 8: \ asm(op " q %1 ,"__percpu_arg(0) \ : " +m" (var) \ -: " re" ((T__)val)); \ +: " re" ((T__)val) \ +: " memory", " cc"); \ break; \ default: __bad_percpu_size(); \ } \
Quoted 8 lines · Eric Dumazet
While playing with new percpu_{read|write|add|sub} stuff in network tree, I found x86 asm was a litle bit optimistic.
We need to tell gcc that percpu_{write|add|sub|or|xor} are modyfing memory and possibly eflags. We could add another parameter to
percpu_to_op()to separate the plain "mov" case (not changing eflags),
but let keep it simple for the moment.
Did you observe an actual failure that this patch fixed?
Quoted 7 lines
Signed-off-by Eric Dumazet <[email protected]>arch/x86/include/asm/percpu.h +0
-0
arch/x86/include/asm/percpu.h do { \ · line 87
Quoted 7 lines
arch/x86/include/asm/percpu.h +2
-1@@ -82,22 +82,26 @@ do { \case 1 : \ asm(op "b %1,"__percpu_arg(0) \ : "+m" (var) \ -: " ri" ((T__)val)); \ +: " ri" ((T__)val) \ +: " memory", " cc"); \
This shouldn't be necessary. The "+m" already tells gcc that var is a memory input and output, and there are no other memory side-effects which it needs to be aware of; clobbering "memory" will force gcc to reload all register-cached memory, which is a pretty hard hit. I think all asms implicitly clobber "cc", so that shouldn't have any effect, but it does no harm.
Now, its true that the asm isn't actually modifying var itself, but %gs:var, which is a different location. But from gcc's perspective that shouldn't matter because var makes a perfectly good proxy for that location, and will make sure it correctly order all accesses to var.
I'd be surprised if this were broken, because we'd be seeing all sorts of strange crashes all over the place. We've seen it before when the old x86-64 pda code didn't have proper constraints on its asm statements.
J
Quoted 4 lines · Jeremy Fitzhardinge
Quoted 3 lines · Eric Dumazet
While playing with new percpu_{read|write|add|sub} stuff in network tree, I found x86 asm was a litle bit optimistic.
Quoted 9 lines
Quoted 6 lines
We need to tell gcc that percpu_{write|add|sub|or|xor} are modyfing memory and possibly eflags. We could add another parameter to
percpu_to_op()
to separate the plain "mov" case (not changing eflags), but let keep it simple for the moment.Did you observe an actual failure that this patch fixed?
Not in current tree, as we dont use yet percpu_xxxx() very much.
If deployed for SNMP mibs with hundred of call sites, can you guarantee it will work as is ?
Quoted 14 lines
Quoted 14 lines
Signed-off-by Eric Dumazet <[email protected]>arch/x86/include/asm/percpu.h +2
-1@@ -82,22 +82,26 @@ do { \case 1 : \ asm(op "b %1,"__percpu_arg(0) \ : "+m" (var) \ -: " ri" ((T__)val)); \ +: " ri" ((T__)val) \ +: " memory", " cc"); \
Quoted 8 lines
Quoted 1 line
This shouldn't be necessary. The "+m" already tells gcc that var is a
memory input and output, and there are no other memory side-effects which it needs to be aware of; clobbering "memory" will force gcc to reload all register-cached memory, which is a pretty hard hit. I think all asms implicitly clobber "cc", so that shouldn't have any effect, but it does no harm.
So, we can probably cleanup many asms in tree :)
static inline void __down_read(struct rw_semaphore *sem) {
asm volatile("# beginning down_read\n\t"
LOCK_PREFIX " incl (%%eax)\n\t"
/* adds 0x00000001, returns the old value */
" jns 1f\n"
" call call_rwsem_down_read_failed\n" "1:\n\t"
"# ending down_read\n\t"
: "+m" (sem->count)
: "a" (sem)
: "memory", "cc");
}
Quoted 9 lines
Now, its true that the asm isn't actually modifying var itself, but %gs:var, which is a different location. But from gcc's perspective that shouldn't matter because var makes a perfectly good proxy for that location, and will make sure it correctly order all accesses to var.
I'd be surprised if this were broken, because we'd be seeing all sorts of strange crashes all over the place. We've seen it before when the old x86-64 pda code didn't have proper constraints on its asm statements.
I was not saying it is broken, but a "litle bit optimistic" :)
Better be safe than sorry, because those errors are very hard to track, since it depends a lot on gcc being aggressive or not. I dont have time to test all gcc versions all over there.
Quoted 12 lines · Eric Dumazet
Quoted 11 lines · Jeremy Fitzhardinge
Quoted 9 lines · Eric Dumazet
While playing with new percpu_{read|write|add|sub} stuff in network tree, I found x86 asm was a litle bit optimistic.
We need to tell gcc that percpu_{write|add|sub|or|xor} are modyfing memory and possibly eflags. We could add another parameter to
percpu_to_op()
to separate the plain "mov" case (not changing eflags), but let keep it simple for the moment.
Quoted 7 lines
Quoted 2 lines
Did you observe an actual failure that this patch fixed?
Not in current tree, as we dont use yet
percpu_xxxx()very much.If deployed for SNMP mibs with hundred of call sites, can you guarantee it will work as is ?
Do we "guarantee" it for you? No.
Is it expected to work just fine? Yes.
Are there any known bugs in this area? No.
Will we fix it if it's demonstrated to be broken? Of course! :-)
[ Btw., it's definitely cool that you will make heavy use for it for
SNMP mib statistics - please share with us your experiences with the facilities - good or bad experiences alike! ]
Quoted 26 lines
Quoted 14 lines
Quoted 14 lines
Signed-off-by Eric Dumazet <[email protected]>arch/x86/include/asm/percpu.h +2
-1@@ -82,22 +82,26 @@ do { \case 1 : \ asm(op "b %1,"__percpu_arg(0) \ : "+m" (var) \ -: " ri" ((T__)val)); \ +: " ri" ((T__)val) \ +: " memory", " cc"); \ Quoted 8 lines
Quoted 1 line
This shouldn't be necessary. The "+m" already tells gcc that var is a
memory input and output, and there are no other memory side-effects which it needs to be aware of; clobbering "memory" will force gcc to reload all register-cached memory, which is a pretty hard hit. I think all asms implicitly clobber "cc", so that shouldn't have any effect, but it does no harm.So, we can probably cleanup many asms in tree :)
Quoted 13 lines
static inline void
__down_read(struct rw_semaphore *sem){
asm volatile("# beginning down_read\n\t"
LOCK_PREFIX" incl (%%eax)\n\t"
/* adds0x00000001, returns the old value */
" jns 1f\n"
" call call_rwsem_down_read_failed\n" "1:\n\t"
"# ending down_read\n\t"
: "+m" (sem->count)
: "a" (sem)
: "memory", "cc");
}
Hm, what's your point with pasting this inline function?
Quoted 9 lines
Quoted 8 lines
Now, its true that the asm isn't actually modifying var itself, but %gs:var, which is a different location. But from gcc's perspective that shouldn't matter because var makes a perfectly good proxy for that location, and will make sure it correctly order all accesses to var.
I'd be surprised if this were broken, because we'd be seeing all sorts of strange crashes all over the place. We've seen it before when the old x86-64 pda code didn't have proper constraints on its asm statements.
Quoted 5 lines
I was not saying it is broken, but a "litle bit optimistic" :)
Better be safe than sorry, because those errors are very hard to track, since it depends a lot on gcc being aggressive or not. I dont have time to test all gcc versions all over there.
Well, Jeremy has already made the valid point that your patch pessimises the constraints and hence likely causes worse code.
We can only apply assembly constraint patches that:
either fix a demonstrated bug,
or improve (speed up) the code emitted,
or very rarely, we will apply patches that dont actually make the
code worse (they are an invariant) but are perceived to be safer
This patch matches neither of these tests and in fact it will probably make the generated code worse.
Ingo
Quoted 6 lines · Ingo Molnar
Quoted 3 lines
: "memory", "cc"); }Hm, what's your point with pasting this inline function?
He's pointing out the redundant (but harmless) "cc" clobber.
J
Quoted 9 lines · Jeremy Fitzhardinge
Quoted 6 lines · Ingo Molnar
Quoted 3 lines
: "memory", "cc"); }Hm, what's your point with pasting this inline function?
He's pointing out the redundant (but harmless) "cc" clobber.
ah, yes. We are completely inconsistent about that. It doesnt matter on x86 so i guess could be removed everywhere.
Ingo
Quoted 17 lines · Ingo Molnar
Quoted 15 lines · Eric Dumazet
Quoted 4 lines · Jeremy Fitzhardinge
Quoted 3 lines · Eric Dumazet
While playing with new percpu_{read|write|add|sub} stuff in network tree, I found x86 asm was a litle bit optimistic.
Quoted 8 lines
Quoted 6 lines
We need to tell gcc that percpu_{write|add|sub|or|xor} are modyfing memory and possibly eflags. We could add another parameter to
percpu_to_op()
to separate the plain "mov" case (not changing eflags), but let keep it simple for the moment.Did you observe an actual failure that this patch fixed?
Not in current tree, as we dont use yet
percpu_xxxx()very much.
Quoted 8 lines
Quoted 2 lines
If deployed for SNMP mibs with hundred of call sites, can you guarantee it will work as is ?
Do we "guarantee" it for you? No.
Is it expected to work just fine? Yes.
Are there any known bugs in this area? No.
Good to know. So I shut up. I am a jerk and should blindly trust linux kernel, sorry.
Quoted 6 lines
Will we fix it if it's demonstrated to be broken? Of course! :-)
[ Btw., it's definitely cool that you will make heavy use for it for
SNMP mib statistics - please share with us your experiences with the facilities - good or bad experiences alike! ]
I tried but I miss kind of an indirect percpu_add() function.
because of Net namespaces, mibs are dynamically allocated, and current percpu_add() works on static percpu only (because of added per_cpu__ prefix)
#define percpu_add(var, val) percpu_to_op("add", per_cpu__##var, val)
I tried adding :
#define dyn_percpu_add(var, val) percpu_to_op("add", var, val)
But I dont know it this is the plan ?
Should we get rid of "per_cpu__" prefix and use a special ELF section/ marker instead ?
I have a patch to add percpu_inc() and percpu_dec(), I am not sure its worth it...
[PATCH] percpu: Adds percpu_inc() and percpu_dec()
Increments and decrements are quite common operations for SNMP mibs.
arch/x86/include/asm/percpu.h +25 -0
@@ -103,6 +103,29 @@ do { \ } \ } while (0) +#define percpu_to_op0 (op, var) \ +do { \ + 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(); \ + } \ +} while (0) +#define percpu_from_op (op, var) \ ({ \ typeof(var) ret__; \
@@ -139,6 +162,8 @@ do { \#define percpu_and (var, val) percpu_to_op("and", per_cpu__##var, val)#define percpu_or (var, val) percpu_to_op("or", per_cpu__##var, val)#define percpu_xor (var, val) percpu_to_op("xor", per_cpu__##var, val) +#define percpu_inc (var) percpu_to_op0("inc", per_cpu__##var) +#define percpu_dec (var) percpu_to_op0("dec", per_cpu__##var)/* This is not atomic against other CPUs -- CPU preemption needs to be off */ #define x86_test_and_clear_bit_percpu (bit, var) \
include/asm-generic/percpu.h +8 -0
@@ -120,6 +120,14 @@ do { \# define percpu_sub (var, val) __percpu_generic_to_op(var, (val), -=)#endif +#ifndef percpu_inc +# define percpu_inc (var) do { percpu_add(var, 1); } while (0) +#endif + +#ifndef percpu_dec +# define percpu_dec (var) do { percpu_sub(var, 1); } while (0) +#endif +#ifndef percpu_and# define percpu_and (var, val) __percpu_generic_to_op(var, (val), &=)#endif
arch/x86/include/asm/percpu.h do { \ · line 166
Quoted 3 lines · Eric Dumazet
arch/x86/include/asm/percpu.h +2
-0+#define percpu_inc (var) percpu_to_op0("inc", per_cpu__##var) +#define percpu_dec (var) percpu_to_op0("dec", per_cpu__##var)
There's probably not a lot of value in this. The Intel and AMD optimisation guides tend to deprecate inc/dec in favour of using add/sub, because the former can cause pipeline stalls due to its partial flags update.
J
Quoted 11 lines · Jeremy Fitzhardinge
arch/x86/include/asm/percpu.h do { \ · line 166
Quoted 3 lines · Eric Dumazet
arch/x86/include/asm/percpu.h +2
-0+#define percpu_inc (var) percpu_to_op0("inc", per_cpu__##var) +#define percpu_dec (var) percpu_to_op0("dec", per_cpu__##var)There's probably not a lot of value in this. The Intel and AMD optimisation guides tend to deprecate inc/dec in favour of using add/sub, because the former can cause pipeline stalls due to its partial flags update.
J
Sure, but this saves one byte per call, this is probably why we still use inc/dec in so many places...
Quoted 5 lines · Jeremy Fitzhardinge
There's probably not a lot of value in this. The Intel and AMD optimisation guides tend to deprecate inc/dec in favour of using add/sub, because the former can cause pipeline stalls due to its partial flags update.
Is this still the case on the latest Intel CPUs?
Cheers,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txtQuoted 7 lines · Herbert Xu
Quoted 2 lines · Jeremy Fitzhardinge
There's probably not a lot of value in this. The Intel and AMD optimisation guides tend to deprecate inc/dec in favour of using add/sub, because the former can cause pipeline stalls due to its partial flags update.
Is this still the case on the latest Intel CPUs?
Yes:
Assembly/Compiler Coding Rule 32. (M impact, H generality) INC and DEC instructions should be replaced with ADD or SUB instructions, because ADD and
SUB overwrite all flags, whereas INC and DEC do not, therefore creating false
dependencies on earlier instructions that set the flags.
J
RFC 1/1 percpu: convert SNMP mibs to new infra
In reply to Eric DumazetQuoted 14 lines · Eric Dumazet
Quoted 4 lines · Ingo Molnar
[ Btw., it's definitely cool that you will make heavy use for it for
SNMP mib statistics - please share with us your experiences with the facilities - good or bad experiences alike! ]I tried but I miss kind of an indirect
percpu_add()function.because of Net namespaces, mibs are dynamically allocated, and current
percpu_add()works on static percpu only (because of addedper_cpu__prefix)#define
percpu_add(var, val)percpu_to_op("add", per_cpu__##var, val)
Quoted 8 lines
I tried adding :
#define
dyn_percpu_add(var, val)percpu_to_op("add", var, val)But I dont know it this is the plan ?
Should we get rid of "per_cpu__" prefix and use a special ELF section/ marker instead ?
Here is a preliminary patch for SNMP mibs that seems to work well on x86_32
[RFC] percpu: convert SNMP mibs to new infra
Some arches can use percpu infrastructure for safe changes to mibs. (percpu_add() is safe against preemption and interrupts), but we want the real thing (a single instruction), not an emulation.
On arches still using an emulation, its better to keep the two views per mib and preemption disable/enable
This shrinks size of mibs by 50%, but also shrinks vmlinux text size (minimum IPV4 config)
$ size vmlinux.old vmlinux.new text data bss dec hex filename 4308458 561092 1728512 6598062 64adae vmlinux.old 4303834 561092 1728512 6593438 649b9e vmlinux.new
--- arch/x86/include/asm/percpu.h | 3 +++ include/net/snmp.h | 27 ++++++++++++++++++++++----- net/ipv4/af_inet.c | 28 +++++++++++++++++++--------- 3 files changed, 44 insertions(+), 14 deletions(-)
arch/x86/include/asm/percpu.h +3 -0
@@ -135,6 +135,9 @@ do { \#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) +#define indir_percpu_add (var, val) percpu_to_op("add", *(var), val) +#define indir_percpu_inc (var) percpu_to_op("add", *(var), 1) +#define indir_percpu_dec (var) percpu_to_op("add", *(var), -1)#define percpu_sub (var, val) percpu_to_op("sub", per_cpu__##var, val)#define percpu_and (var, val) percpu_to_op("and", per_cpu__##var, val)#define percpu_or (var, val) percpu_to_op("or", per_cpu__##var, val)
include/net/snmp.h +22 -5
@@ -123,15 +123,31 @@ struct linux_xfrm_mib { };/* -* FIXME: On x86 and some other CPUs the split into user and softirq parts +* On x86 and some other CPUs the split into user and softirq parts * is not needed because addl $1,memory is atomic against interrupts (but -* atomic_inc would be overkill because of the lock cycles). Wants new -* nonlocked_atomic_inc() primitives -AK +* atomic_inc would be overkill because of the lock cycles). */ +#ifdef CONFIG_X86 +# define SNMP_ARRAY_SZ 1 +#else +# define SNMP_ARRAY_SZ 2 +#endif +#define DEFINE_SNMP_STAT (type, name) \ - __typeof__(type) *name[2] + __typeof__(type) *name[SNMP_ARRAY_SZ]#define DECLARE_SNMP_STAT (type, name) \ - extern __typeof__(type) *name[2] + extern __typeof__(type) *name[SNMP_ARRAY_SZ] + +#if SNMP_ARRAY_SZ ==1 +#define SNMP_INC_STATS (mib, field) indir_percpu_inc(&mib[0]->mibs[field]) +#define SNMP_INC_STATS_BH (mib, field) SNMP_INC_STATS(mib, field) +#define SNMP_INC_STATS_USER (mib, field) SNMP_INC_STATS(mib, field) +#define SNMP_DEC_STATS (mib, field) indir_percpu_dec(&mib[0]->mibs[field]) +#define SNMP_ADD_STATS_BH (mib, field, addend) \ + indir_percpu_add(&mib[0]->mibs[field], addend) +#define SNMP_ADD_STATS_USER (mib, field, addend) \ + indir_percpu_add(&mib[0]->mibs[field], addend) +#else #define SNMP_STAT_BHPTR (name) (name[0])#define SNMP_STAT_USRPTR (name) (name[1])
@@ -160,5 +176,6 @@ struct linux_xfrm_mib {per_cpu_ptr (mib[1 ],get_cpu ())->mibs[field] += addend; \put_cpu (); \ } while (0) +#endif #endif
net/ipv4/af_inet.c +19 -9
@@ -1366,27 +1366,37 @@ unsigned long snmp_fold_field(void *mib[], int offt)for_each_possible_cpu (i) { res += *(((unsigned long *)per_cpu_ptr (mib[0 ], i)) + offt); +#if SNMP_ARRAY_SZ ==2 res += *(((unsigned long *)per_cpu_ptr (mib[1 ], i)) + offt); +#endif }return res; }EXPORT_SYMBOL_GPL (snmp_fold_field); -int snmp_mib_init (void *ptr[2 ],size_t mibsize) +int snmp_mib_init (void *ptr[SNMP_ARRAY_SZ ],size_t mibsize) {BUG_ON (ptr ==NULL ); ptr[0 ] =__alloc_percpu (mibsize, __alignof__(unsigned long long ));if (!ptr[0 ]) - goto err0; +return -ENOMEM ; +#if SNMP_ARRAY_SZ ==2 ptr[1 ] =__alloc_percpu (mibsize, __alignof__(unsigned long long )); -if (!ptr[1 ]) - goto err1; +if (!ptr[1 ]) { +free_percpu (ptr[0 ]); + ptr[0 ] =NULL ; +return -ENOMEM ; + } +#endif + { +int i; +printk (KERN_INFO "snmp_mib_init(%u) %p " , (unsigned int )mibsize, ptr[0 ]); +for_each_possible_cpu (i) { +printk (KERN_INFO "%p " ,per_cpu_ptr (ptr[0 ], i)); + } +printk (KERN_INFO "\n" ); + }return 0 ; -err1: -free_percpu (ptr[0 ]); - ptr[0 ] =NULL ; -err0: -return -ENOMEM ; }EXPORT_SYMBOL_GPL (snmp_mib_init);
Re: [RFC] percpu: convert SNMP mibs to new infra
Hello, Eric, Ingo.
Quoted 5 lines · Eric Dumazet
arch/x86/include/asm/percpu.h +0
-0
arch/x86/include/asm/percpu.h do { \ · line 143
Quoted 9 lines
arch/x86/include/asm/percpu.h +3
-0@@ -135,6 +135,9 @@ do { \#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) +#define indir_percpu_add (var, val) percpu_to_op("add", *(var), val) +#define indir_percpu_inc (var) percpu_to_op("add", *(var), 1) +#define indir_percpu_dec (var) percpu_to_op("add", *(var), -1)#define percpu_sub (var, val) percpu_to_op("sub", per_cpu__##var, val)#define percpu_and (var, val) percpu_to_op("and", per_cpu__##var, val)#define percpu_or (var, val) percpu_to_op("or", per_cpu__##var, val)
The final goal is to unify static and dynamic accesses but we aren't there yet, so, for the time being, we'll need some interim solutions. I would prefer percpu_ptr_add() tho.
Thanks.
Signature · 1 line
tejun
Re: [RFC] percpu: convert SNMP mibs to new infra
Quoted 13 lines · Tejun Heo
Hello, Eric, Ingo.
Quoted 10 lines · Eric Dumazet
arch/x86/include/asm/percpu.h +2
-0@@ -135,6 +135,9 @@ do { \#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) +#define indir_percpu_add (var, val) percpu_to_op("add", *(var), val) +#define indir_percpu_inc (var) percpu_to_op("add", *(var), 1)
Quoted 8 lines
arch/x86/include/asm/percpu.h do { \ · line 143
Quoted 4 lines
arch/x86/include/asm/percpu.h +1
-0+#define indir_percpu_dec (var) percpu_to_op("add", *(var), -1)#define percpu_sub (var, val) percpu_to_op("sub", per_cpu__##var, val)#define percpu_and (var, val) percpu_to_op("and", per_cpu__##var, val)#define percpu_or (var, val) percpu_to_op("or", per_cpu__##var, val)The final goal is to unify static and dynamic accesses but we aren't there yet, so, for the time being, we'll need some interim solutions. I would prefer
percpu_ptr_add()tho.
Yep, that's the standard naming scheme for new APIs: generic to specific, left to right.
Ingo
PATCH 1/1 percpu: convert SNMP mibs to new infra
Quoted 23 lines · Ingo Molnar
Quoted 12 lines · Tejun Heo
Hello, Eric, Ingo.
Quoted 9 lines · Eric Dumazet
arch/x86/include/asm/percpu.h +1
-0@@ -135,6 +135,9 @@ do { \#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) +#define indir_percpu_add (var, val) percpu_to_op("add", *(var), val)Quoted 8 lines
arch/x86/include/asm/percpu.h do { \ · line 143
Quoted 5 lines
arch/x86/include/asm/percpu.h +2
-0+#define indir_percpu_inc (var) percpu_to_op("add", *(var), 1) +#define indir_percpu_dec (var) percpu_to_op("add", *(var), -1)#define percpu_sub (var, val) percpu_to_op("sub", per_cpu__##var, val)#define percpu_and (var, val) percpu_to_op("and", per_cpu__##var, val)#define percpu_or (var, val) percpu_to_op("or", per_cpu__##var, val)The final goal is to unify static and dynamic accesses but we aren't there yet, so, for the time being, we'll need some interim solutions. I would prefer
percpu_ptr_add()tho.
Quoted 3 lines
Yep, that's the standard naming scheme for new APIs: generic to specific, left to right.
Here is a second version of the patch, with percpu_ptr_xxx convention, and more polished form (snmp_mib_free() was forgoten in previous RFC)
Thank you all
[PATCH] percpu: convert SNMP mibs to new infra
Some arches can use percpu infrastructure for safe changes to mibs. (percpu_add() is safe against preemption and interrupts), but we want the real thing (a single instruction), not an emulation.
On arches still using an emulation, its better to keep the two views per mib and preemption disable/enable
This shrinks size of mibs by 50%, but also shrinks vmlinux text size (minimum IPV4 config)
$ size vmlinux.old vmlinux.new text data bss dec hex filename 4308458 561092 1728512 6598062 64adae vmlinux.old 4303834 561092 1728512 6593438 649b9e vmlinux.new
--- arch/x86/include/asm/percpu.h | 3 +++ include/net/snmp.h | 27 ++++++++++++++++++++++----- net/ipv4/af_inet.c | 31 ++++++++++++++++++------------- 3 files changed, 43 insertions(+), 18 deletions(-)
arch/x86/include/asm/percpu.h +3 -0
@@ -135,6 +135,9 @@ do { \#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) +#define percpu_ptr_add (var, val) percpu_to_op("add", *(var), val) +#define percpu_ptr_inc (var) percpu_ptr_add(var, 1) +#define percpu_ptr_dec (var) percpu_ptr_add(var, -1)#define percpu_sub (var, val) percpu_to_op("sub", per_cpu__##var, val)#define percpu_and (var, val) percpu_to_op("and", per_cpu__##var, val)#define percpu_or (var, val) percpu_to_op("or", per_cpu__##var, val)
include/net/snmp.h +22 -5
@@ -123,15 +123,31 @@ struct linux_xfrm_mib { };/* -* FIXME: On x86 and some other CPUs the split into user and softirq parts +* On x86 and some other CPUs the split into user and softirq parts * is not needed because addl $1,memory is atomic against interrupts (but -* atomic_inc would be overkill because of the lock cycles). Wants new -* nonlocked_atomic_inc() primitives -AK +* atomic_inc would be overkill because of the lock cycles). */ +#ifdef CONFIG_X86 +# define SNMP_ARRAY_SZ 1 +#else +# define SNMP_ARRAY_SZ 2 +#endif +#define DEFINE_SNMP_STAT (type, name) \ - __typeof__(type) *name[2] + __typeof__(type) *name[SNMP_ARRAY_SZ]#define DECLARE_SNMP_STAT (type, name) \ - extern __typeof__(type) *name[2] + extern __typeof__(type) *name[SNMP_ARRAY_SZ] + +#if SNMP_ARRAY_SZ ==1 +#define SNMP_INC_STATS (mib, field) percpu_ptr_inc(&mib[0]->mibs[field]) +#define SNMP_INC_STATS_BH (mib, field) SNMP_INC_STATS(mib, field) +#define SNMP_INC_STATS_USER (mib, field) SNMP_INC_STATS(mib, field) +#define SNMP_DEC_STATS (mib, field) percpu_ptr_dec(&mib[0]->mibs[field]) +#define SNMP_ADD_STATS_BH (mib, field, addend) \ + percpu_ptr_add(&mib[0]->mibs[field], addend) +#define SNMP_ADD_STATS_USER (mib, field, addend) \ + percpu_ptr_add(&mib[0]->mibs[field], addend) +#else #define SNMP_STAT_BHPTR (name) (name[0])#define SNMP_STAT_USRPTR (name) (name[1])
@@ -160,5 +176,6 @@ struct linux_xfrm_mib {per_cpu_ptr (mib[1 ],get_cpu ())->mibs[field] += addend; \put_cpu (); \ } while (0) +#endif #endif
net/ipv4/af_inet.c +18 -13
@@ -1366,36 +1366,41 @@ unsigned long snmp_fold_field(void *mib[], int offt)for_each_possible_cpu (i) { res += *(((unsigned long *)per_cpu_ptr (mib[0 ], i)) + offt); +#if SNMP_ARRAY_SZ ==2 res += *(((unsigned long *)per_cpu_ptr (mib[1 ], i)) + offt); +#endif }return res; }EXPORT_SYMBOL_GPL (snmp_fold_field); -int snmp_mib_init (void *ptr[2 ],size_t mibsize) +int snmp_mib_init (void *ptr[SNMP_ARRAY_SZ ],size_t mibsize) {BUG_ON (ptr ==NULL ); ptr[0 ] =__alloc_percpu (mibsize, __alignof__(unsigned long long ));if (!ptr[0 ]) - goto err0; +return -ENOMEM ; +#if SNMP_ARRAY_SZ ==2 ptr[1 ] =__alloc_percpu (mibsize, __alignof__(unsigned long long )); -if (!ptr[1 ]) - goto err1; +if (!ptr[1 ]) { +free_percpu (ptr[0 ]); + ptr[0 ] =NULL ; +return -ENOMEM ; + } +#endif return 0 ; -err1: -free_percpu (ptr[0 ]); - ptr[0 ] =NULL ; -err0: -return -ENOMEM ; }EXPORT_SYMBOL_GPL (snmp_mib_init); -void snmp_mib_free (void *ptr[2 ]) +void snmp_mib_free (void *ptr[SNMP_ARRAY_SZ ]) { +int i; +BUG_ON (ptr ==NULL ); -free_percpu (ptr[0 ]); -free_percpu (ptr[1 ]); - ptr[0 ] = ptr[1 ] =NULL ; +for (i =0 ; i <SNMP_ARRAY_SZ ; i++) { +free_percpu (ptr[i]); + ptr[i] =NULL ; + } }EXPORT_SYMBOL_GPL (snmp_mib_free);
Re: [PATCH] percpu: convert SNMP mibs to new infra
Eric Dumazet wrote:
...
arch/x86/include/asm/percpu.h do { \ · line 143
Quoted 9 lines
#define
percpu_read(var)percpu_from_op("mov", per_cpu__##var)
#definepercpu_write(var, val)percpu_to_op("mov", per_cpu__##var, val)
#definepercpu_add(var, val)percpu_to_op("add", per_cpu__##var, val)arch/x86/include/asm/percpu.h +3
-0+#define percpu_ptr_add (var, val) percpu_to_op("add", *(var), val) +#define percpu_ptr_inc (var) percpu_ptr_add(var, 1) +#define percpu_ptr_dec (var) percpu_ptr_add(var, -1)#define percpu_sub (var, val) percpu_to_op("sub", per_cpu__##var, val)#define percpu_and (var, val) percpu_to_op("and", per_cpu__##var, val)#define percpu_or (var, val) percpu_to_op("or", per_cpu__##var, val)
x86 part looks fine to me.
Quoted 10 lines
include/net/snmp.h +1
-1@@ -123,15 +123,31 @@ struct linux_xfrm_mib { };/* -* FIXME: On x86 and some other CPUs the split into user and softirq parts +* On x86 and some other CPUs the split into user and softirq parts
include/net/snmp.h struct linux_xfrm_mib { · line 134
Quoted 10 lines
include/net/snmp.h +6
-2* is not needed because addl $1,memory is atomic against interrupts (but -* atomic_inc would be overkill because of the lock cycles). Wants new -* nonlocked_atomic_inc() primitives -AK +* atomic_inc would be overkill because of the lock cycles). */ +#ifdef CONFIG_X86 +# define SNMP_ARRAY_SZ 1 +#else +# define SNMP_ARRAY_SZ 2 +#endif
This is quite hacky but, well, for the time being...
Thanks.
Signature · 1 line
tejun
Re: [PATCH] percpu: convert SNMP mibs to new infra
In reply to Eric DumazetQuoted 42 lines · Eric Dumazet
Quoted 23 lines · Ingo Molnar
Quoted 12 lines · Tejun Heo
Hello, Eric, Ingo.
Quoted 9 lines · Eric Dumazet
arch/x86/include/asm/percpu.h +1
-0@@ -135,6 +135,9 @@ do { \#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) +#define indir_percpu_add (var, val) percpu_to_op("add", *(var), val)Quoted 8 lines
arch/x86/include/asm/percpu.h do { \ · line 143
Quoted 5 lines
arch/x86/include/asm/percpu.h +2
-0+#define indir_percpu_inc (var) percpu_to_op("add", *(var), 1) +#define indir_percpu_dec (var) percpu_to_op("add", *(var), -1)#define percpu_sub (var, val) percpu_to_op("sub", per_cpu__##var, val)#define percpu_and (var, val) percpu_to_op("and", per_cpu__##var, val)#define percpu_or (var, val) percpu_to_op("or", per_cpu__##var, val)The final goal is to unify static and dynamic accesses but we aren't there yet, so, for the time being, we'll need some interim solutions. I would prefer
percpu_ptr_add()tho.Quoted 3 lines
Yep, that's the standard naming scheme for new APIs: generic to specific, left to right.
Here is a second version of the patch, with
percpu_ptr_xxxconvention, and more polished form (snmp_mib_free()was forgoten in previous RFC)Thank you all
[PATCH] percpu: convert SNMP mibs to new infra
Some arches can use percpu infrastructure for safe changes to mibs. (
percpu_add()is safe against preemption and interrupts), but we want the real thing (a single instruction), not an emulation.On arches still using an emulation, its better to keep the two views per mib and preemption disable/enable
Quoted 7 lines
This shrinks size of mibs by 50%, but also shrinks vmlinux text size (minimum IPV4 config)
$ size vmlinux.old vmlinux.new text data bss dec hex filename 4308458 561092 1728512 6598062 64adae vmlinux.old 4303834 561092 1728512 6593438 649b9e vmlinux.new
Wow, that's pretty impressive!
Quoted 3 lines
Signed-off-by Eric Dumazet <[email protected]>--- arch/x86/include/asm/percpu.h | 3 +++
As far as x86 goes, feel free to pick it up into any of the networking trees, these bits are easily merged and it's probably best if the patch stays in a single piece - it looks compact enough and if it breaks it's going to break in networking code.
Ingo
Re: [RFC] percpu: convert SNMP mibs to new infra
In reply to Eric DumazetQuoted 3 lines · On Thursday 02 April 2009 05:14:47 Eric Dumazet wrote:
Here is a preliminary patch for SNMP mibs that seems to work well on
x86_32[RFC] percpu: convert SNMP mibs to new infra
OK, I have a whole heap of "convert to dynamic per-cpu" patches waiting in the wings too, once Tejun's conversion is complete.
Also, what is optimal depends on the arch: we had a long discussion on this (it's what local_t was supposed to do, with cpu_local_inc() etc: see
Subject: local_add_return 2008-12-16 thread).
eg. on S/390, atomic_inc is a win over the two-counter version. On Sparc, two-counter wins. On x86, inc wins (obviously).
But efforts to create a single primitive have been problematic: maybe open-coding it like this is the Right Thing.
Cheers,
Rusty.
Re: [RFC] percpu: convert SNMP mibs to new infra
Quoted 12 lines · Rusty Russell
Quoted 3 lines · On Thursday 02 April 2009 05:14:47 Eric Dumazet wrote:
Here is a preliminary patch for SNMP mibs that seems to work well on
x86_32[RFC] percpu: convert SNMP mibs to new infra
OK, I have a whole heap of "convert to dynamic per-cpu" patches waiting in the wings too, once Tejun's conversion is complete.
Also, what is optimal depends on the arch: we had a long discussion on this (it's what
local_twas supposed to do, withcpu_local_inc()etc: see
Subject:local_add_return2008-12-16 thread).
Quoted 6 lines
eg. on S/390,
atomic_incis a win over the two-counter version. On Sparc, two-counter wins. On x86, inc wins (obviously).But efforts to create a single primitive have been problematic: maybe open-coding it like this is the Right Thing.
I tried to find a generic CONFIG_ define that would annonce that an arche has a fast percpu_add() implementation. (faster than __raw_get_cpu_var, for example, when we already are in a preempt disabled section)
Any idea ?
For example, net/ipv4/route.c has :
static DEFINE_PER_CPU(struct rt_cache_stat, rt_cache_stat);
#define RT_CACHE_STAT_INC(field) \
(__raw_get_cpu_var(rt_cache_stat).field++)We could use percpu_add(rt_cache_stat.field, 1) instead, only if percpu_add() is not the generic one.
#define __percpu_generic_to_op(var, val, op) \
do { \
get_cpu_var(var) op val; \
put_cpu_var(var); \
} while (0)
#ifndef percpu_add
# define percpu_add(var, val) __percpu_generic_to_op(var, (val), +=)
#endifRe: [RFC] percpu: convert SNMP mibs to new infra
Quoted 10 lines · On Thursday 02 April 2009 15:49:19 Eric Dumazet wrote:
Quoted 5 lines · Rusty Russell
eg. on S/390,
atomic_incis a win over the two-counter version. On Sparc, two-counter wins. On x86, inc wins (obviously).But efforts to create a single primitive have been problematic: maybe open-coding it like this is the Right Thing.
I tried to find a generic
CONFIG_define that would annonce that an arche has a fastpercpu_add()implementation. (faster than__raw_get_cpu_var, for example, when we already are in a preempt disabled section)
Nope, we don't have one. It was supposed to work like this:
DEFINE_PER_CPU(local_t, counter); cpu_local_inc(counter);
That would do incl in x86, local_t could even be a long[3] (one for hardirq, one for softirq, one for user context). But there were issues:
1) It didn't work on dynamic percpu allocs, which was much of the interesting use (Tejun is fixing this bit right now)
2) The x86 version wasn't optimized anyway,
3) Everyone did atomic_long_inc(), so the ftrace code assumed it would be nmi safe (tho atomic_t isn't nmi-safe on some archs anyway), so the long[3] method would break them,
4) The long[3] version was overkill for networking, which doesn't need hardirq so we'd want another variant of local_t plus all the ops,
5) Some people didn't want long: Christoph had a more generic but more complex version,
6) It's still not used anywhere in the tree (tho local_t is), so there's no reason to stick to the current semantics.
Quoted 8 lines
For example,
net/ipv4/route.chas :static
DEFINE_PER_CPU(struct rt_cache_stat, rt_cache_stat);#define RT_CACHE_STAT_INC(field) \ (__raw_get_cpu_var(rt_cache_stat).field++)We could use
percpu_add(rt_cache_stat.field, 1)instead, only ifpercpu_add()is not the generic one.
Yep, but this one is different from the SNMP stats which needs softirq vs user context safety. This is where I start wondering how many interfaces we're going to have...
Sorry to add more questions than answers :(
Rusty.