cmpxchg_double: remove local variables to get better performance

RFC PATCH In review Alex Shi 2 participants 7 messages Mar 2, 2012 – Mar 3, 2012 x86

Share & tools

Alex Shi

author

2012-03-02 08:32 UTC #0 lore raw

RFC PATCH 1/1 cmpxchg_double: remove local variables to get better performance

There are some local variables in cmpxchg_double macro, seems these are used to for force casting on input variables to transfer them into '*p1' type. May there are some reason I don't know. But I just saw 2 problems here:

1, user may mis-use the macro, like give a 'long' type o1, but just use a 'int*' or 'char*' p1.
If we remove the force cast here, gcc will check the mis-using in compiling. and user can get the error report in compiling for such issues.

2, local variable increased the data section, and bring extra memory bus accesses, that hurt performance in this critical macro.
I did a little experiment on my nhm i7 desktop, to run the macro with a fixed times, here is the data:
using local vars no local variable

with lock prefix,         267700578ns             232079696ns
without lock prefix,      34715666ns              34687566ns

So, we may need rethink about the local variable usage here.

Signed-off-by Alex Shi <[email protected]>

---

arch/x86/include/asm/cmpxchg.h +3 -5

index b3b7332..8bf9127 100644
@@ -210,17 +210,15 @@ extern void __add_wrong_size(void)
 #define __cmpxchg_double(pfx, p1, p2, o1, o2, n1, n2)			\
 ({									\
 	bool __ret;							\
-	__typeof__(*(p1)) __old1 = (o1), __new1 = (n1);			\
-	__typeof__(*(p2)) __old2 = (o2), __new2 = (n2);			\
 	BUILD_BUG_ON(sizeof(*(p1)) != sizeof(long));			\
 	BUILD_BUG_ON(sizeof(*(p2)) != sizeof(long));			\
 	VM_BUG_ON((unsigned long)(p1) % (2 * sizeof(long)));		\
 	VM_BUG_ON((unsigned long)((p1) + 1) != (unsigned long)(p2));	\
 	asm volatile(pfx "cmpxchg%c4b %2; sete %0"			\
-		     : "=a" (__ret), "+d" (__old2),			\
+		     : "=a" (__ret), "+d" (o2),				\
 		       "+m" (*(p1)), "+m" (*(p2))			\
-		     : "i" (2 * sizeof(long)), "a" (__old1),		\
-		       "b" (__new1), "c" (__new2));			\
+		     : "i" (2 * sizeof(long)), "a" (o1),		\
+		       "b" (n1), "c" (n2));				\
 	__ret;								\
 })

Jan Beulich

2012-03-02 08:53 UTC #1 lore raw

Quoted 8 lines

Quoted 1 line
Quoted 1 line · Alex Shi
There are some local variables in cmpxchg_double macro, seems these are used to for force casting on input variables to transfer them into '*p1' type. May there are some reason I don't know. But I just saw 2 problems here:
1, user may mis-use the macro, like give a 'long' type o1, but just use a 'int*' or 'char*' p1.

No - see the BUILD_BUG_ON()s right after the lines you suggest to remove.

Further, it seems to be intentional to allow _compatible_ types for o1 and o2 - you could pass in a literal number without L suffix here, which I don't think you can anymore with the intermediate variable removed.

Quoted 5 lines

If we remove the force cast here, gcc will check the mis-using in compiling. and user can get the error report in compiling for such issues.
2, local variable increased the data section, and bring extra memory bus

These aren't static, so the data section can't possibly increase.

Quoted 1 line

accesses, that hurt performance in this critical macro.

With optimization enabled, the compiler should eliminate all unnecessary intermediate variables.

Quoted 9 lines

I did a little experiment on my nhm i7 desktop, to run the macro with a fixed times, here is the data:
using local vars no local variable
with lock prefix,         267700578ns             232079696ns
without lock prefix,      34715666ns              34687566ns
So, we may need rethink about the local variable usage here.
Signed-off-by Alex Shi <[email protected]>

Sorry, but if this counts, this is a nack from me.

Jan

Quoted 25 lines

arch/x86/include/asm/cmpxchg.h +3 -5

index b3b7332..8bf9127 100644
@@ -210,17 +210,15 @@ extern void __add_wrong_size(void)
 #define __cmpxchg_double(pfx, p1, p2, o1, o2, n1, n2)			\
 ({									\
 	bool __ret;							\
-	__typeof__(*(p1)) __old1 = (o1), __new1 = (n1);			\
-	__typeof__(*(p2)) __old2 = (o2), __new2 = (n2);			\
 	BUILD_BUG_ON(sizeof(*(p1)) != sizeof(long));			\
 	BUILD_BUG_ON(sizeof(*(p2)) != sizeof(long));			\
 	VM_BUG_ON((unsigned long)(p1) % (2 * sizeof(long)));		\
 	VM_BUG_ON((unsigned long)((p1) + 1) != (unsigned long)(p2));	\
 	asm volatile(pfx "cmpxchg%c4b %2; sete %0"			\
-		     : "=a" (__ret), "+d" (__old2),			\
+		     : "=a" (__ret), "+d" (o2),				\
 		       "+m" (*(p1)), "+m" (*(p2))			\
-		     : "i" (2 * sizeof(long)), "a" (__old1),		\
-		       "b" (__new1), "c" (__new2));			\
+		     : "i" (2 * sizeof(long)), "a" (o1),		\
+		       "b" (n1), "c" (n2));				\
 	__ret;								\
 })

Alex Shi

author

2012-03-02 09:01 UTC #2 lore raw

Quoted 9 lines · Jan Beulich

Quoted 8 lines
Quoted 1 line
Quoted 1 line · Alex Shi
There are some local variables in cmpxchg_double macro, seems these are used to for force casting on input variables to transfer them into '*p1' type. May there are some reason I don't know. But I just saw 2 problems here:
1, user may mis-use the macro, like give a 'long' type o1, but just use a 'int*' or 'char*' p1.

Quoted 7 lines

No - see the BUILD_BUG_ON()s right after the lines you suggest to remove.
Further, it seems to be intentional to allow _compatible_ types for o1 and o2 - you could pass in a literal number without L suffix here, which I don't think you can anymore with the intermediate variable removed.

Yes, we can use cast for intermediate data. And actually, current kernel has live mis-used case on cmpxchg(), that I plan to point out too.

-- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -203,12 +203,12 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)

void kvm_flush_remote_tlbs(struct kvm *kvm)
{
- int dirty_count = kvm->tlbs_dirty;
+ long dirty_count = kvm->tlbs_dirty;

        smp_mb();
        if (make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH))
                ++kvm->stat.remote_tlb_flush;

- cmpxchg(&kvm->tlbs_dirty, dirty_count, 0);
+ cmpxchg(&kvm->tlbs_dirty, dirty_count, 0L);
}

Quoted 8 lines

Quoted 6 lines
If we remove the force cast here, gcc will check the mis-using in compiling. and user can get the error report in compiling for such issues.
2, local variable increased the data section, and bring extra memory bus
These aren't static, so the data section can't possibly increase.

sorry, it is text section increasing.

Quoted 5 lines

Quoted 2 lines
accesses, that hurt performance in this critical macro.
With optimization enabled, the compiler should eliminate all unnecessary intermediate variables.

oh, I don't know now. I will recheck this point.

Quoted 43 lines

Quoted 10 lines

I did a little experiment on my nhm i7 desktop, to run the macro with a fixed times, here is the data:
using local vars no local variable
with lock prefix,         267700578ns             232079696ns
without lock prefix,      34715666ns              34687566ns
So, we may need rethink about the local variable usage here.
Signed-off-by Alex Shi <[email protected]>

Sorry, but if this counts, this is a nack from me.

Jan

Quoted 25 lines

arch/x86/include/asm/cmpxchg.h +3 -5

index b3b7332..8bf9127 100644
@@ -210,17 +210,15 @@ extern void __add_wrong_size(void)
 #define __cmpxchg_double(pfx, p1, p2, o1, o2, n1, n2)			\
 ({									\
 	bool __ret;							\
-	__typeof__(*(p1)) __old1 = (o1), __new1 = (n1);			\
-	__typeof__(*(p2)) __old2 = (o2), __new2 = (n2);			\
 	BUILD_BUG_ON(sizeof(*(p1)) != sizeof(long));			\
 	BUILD_BUG_ON(sizeof(*(p2)) != sizeof(long));			\
 	VM_BUG_ON((unsigned long)(p1) % (2 * sizeof(long)));		\
 	VM_BUG_ON((unsigned long)((p1) + 1) != (unsigned long)(p2));	\
 	asm volatile(pfx "cmpxchg%c4b %2; sete %0"			\
-		     : "=a" (__ret), "+d" (__old2),			\
+		     : "=a" (__ret), "+d" (o2),				\
 		       "+m" (*(p1)), "+m" (*(p2))			\
-		     : "i" (2 * sizeof(long)), "a" (__old1),		\
-		       "b" (__new1), "c" (__new2));			\
+		     : "i" (2 * sizeof(long)), "a" (o1),		\
+		       "b" (n1), "c" (n2));				\
 	__ret;								\
 })

Jan Beulich

2012-03-02 09:11 UTC #3 lore raw

Quoted 9 lines

Quoted 1 line
Quoted 1 line · Alex Shi
Yes, we can use cast for intermediate data. And actually, current kernel has live mis-used case on cmpxchg(), that I plan to point out too.
-- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
(quoted diff) +0 -0
@@ -203,12 +203,12 @@ static bool make_all_cpus_request(struct kvm *kvm, 
 unsigned int req)
 

Review static bool make_all_cpus_request(struct kvm *kvm, · line 212

Quoted 10 lines

(quoted diff) +2 -2

 void kvm_flush_remote_tlbs(struct kvm *kvm)
 {
-       int dirty_count = kvm->tlbs_dirty;
+       long dirty_count = kvm->tlbs_dirty;
 
        smp_mb();
        if (make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH))
                ++kvm->stat.remote_tlb_flush;
-       cmpxchg(&kvm->tlbs_dirty, dirty_count, 0);
+       cmpxchg(&kvm->tlbs_dirty, dirty_count, 0L);

Indeed - the cmpxchg would fail if the value doesn't fit. But this is not to say that in certain cases it isn't valid to pass an int for the second and/or third argument. (And quite likely the issue here is theoretical only anyway.)

In particular, requiring an L suffix here on literals should be avoided.

Jan

Quoted 1 line

(quoted diff) +0 -0
 }

Alex Shi

author

2012-03-02 15:12 UTC #4 lore raw

Quoted 14 lines · Jan Beulich

Quoted 14 lines
Quoted 1 line
Quoted 1 line · Alex Shi
Yes, we can use cast for intermediate data. And actually, current kernel has live mis-used case on cmpxchg(), that I plan to point out too.
-- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
(quoted diff) +1 -1
@@ -203,12 +203,12 @@ static bool make_all_cpus_request(struct kvm *kvm, 
 unsigned int req)
 
 void kvm_flush_remote_tlbs(struct kvm *kvm)
 {
-       int dirty_count = kvm->tlbs_dirty;
+       long dirty_count = kvm->tlbs_dirty;
 

Quoted 10 lines

Review static bool make_all_cpus_request(struct kvm *kvm, · line 212
Quoted 5 lines
(quoted diff) +1 -1
        smp_mb();
        if (make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH))
                ++kvm->stat.remote_tlb_flush;
-       cmpxchg(&kvm->tlbs_dirty, dirty_count, 0);
+       cmpxchg(&kvm->tlbs_dirty, dirty_count, 0L);
Indeed - the cmpxchg would fail if the value doesn't fit. But this is not to say that in certain cases it isn't valid to pass an int for the second and/or third argument. (And quite likely the issue here is theoretical only anyway.)

It may cause potential issue, if it is tlbs_dirty mis-used here, not dirty_count. If so, it may cause data damage.

Quoted 2 lines

In particular, requiring an L suffix here on literals should be avoided.

Even the each macro may save 0x40 bytes text, and bring more 10% execution speed?

Quoted 6 lines

Jan
Quoted 1 line
(quoted diff) +0 -0
 }

Jan Beulich

2012-03-02 15:29 UTC #5 lore raw

Quoted 7 lines

Quoted 1 line
Quoted 1 line · Alex Shi
Quoted 1 line · Jan Beulich
In particular, requiring an L suffix here on literals should be avoided.
Even the each macro may save 0x40 bytes text, and bring more 10% execution speed?

Again - if you see meaningful text size differences with optimization properly enabled, this may need investigation at the compiler end.

Jan

Alex Shi

author

2012-03-03 06:03 UTC #6 lore raw

Quoted 10 lines · Jan Beulich

Quoted 7 lines
Quoted 1 line
Quoted 1 line · Alex Shi
Quoted 1 line · Jan Beulich
In particular, requiring an L suffix here on literals should be avoided.
Even the each macro may save 0x40 bytes text, and bring more 10% execution speed?
Again - if you see meaningful text size differences with optimization properly enabled, this may need investigation at the compiler end.

Yes, you are right.
I take back this patch.

Quoted 3 lines

Jan