Skip to contentSkip to conversation

x86/ACPI/boot: use try_cmpxchg in __acpi_{acquire,release}_global_lock

PATCH Applied Uros Bizjak 4 participants 5 messages Jan 16, 2023 – Jan 26, 2023

Subsystems (2)
x86 ACPI
lore
Share & tools
lore

Uros Bizjak

author
#0 lore raw

PATCH 1/1 x86/ACPI/boot: use try_cmpxchg in __acpi_{acquire,release}_global_lock

Use try_cmpxchg instead of cmpxchg (*ptr, old, new) == old in __acpi_{acquire,release}_global_lock. x86 CMPXCHG instruction returns success in ZF flag, so this change saves a compare after cmpxchg (and related move instruction in front of cmpxchg).

Also, try_cmpxchg implicitly assigns old *ptr value to "old" when cmpxchg fails. There is no need to re-read the value in the loop.

Note that the value from *ptr should be read using READ_ONCE to prevent the compiler from merging, refetching or reordering the read.

No functional change intended.

Signed-off-by Uros Bizjak <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Len Brown <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Dave Hansen <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
---
 arch/x86/kernel/acpi/boot.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
arch/x86/kernel/acpi/boot.c +8 -8
index 907cc98b1938..4177577c173b 100644
@@ -1840,23 +1840,23 @@ early_param("acpi_sci", setup_acpi_sci);
 
 int __acpi_acquire_global_lock(unsigned int *lock)
 {
-	unsigned int old, new, val;
+	unsigned int old, new;
+
+	old = READ_ONCE(*lock);
 	do {
-		old = *lock;
 		new = (((old & ~0x3) + 2) + ((old >> 1) & 0x1));
-		val = cmpxchg(lock, old, new);
-	} while (unlikely (val != old));
+	} while (!try_cmpxchg(lock, &old, new));
 	return ((new & 0x3) < 3) ? -1 : 0;
 }
 
 int __acpi_release_global_lock(unsigned int *lock)
 {
-	unsigned int old, new, val;
+	unsigned int old, new;
+
+	old = READ_ONCE(*lock);
 	do {
-		old = *lock;
 		new = old & ~0x3;
-		val = cmpxchg(lock, old, new);
-	} while (unlikely (val != old));
+	} while (!try_cmpxchg(lock, &old, new));
 	return old & 0x1;
 }
Signature · 1 line
2.39.0
maintainer Acked-by on v1 1/1
#1 lore raw
Quoted 14 lines · Uros Bizjak

Use try_cmpxchg instead of cmpxchg (*ptr, old, new) == old in __acpi_{acquire,release}_global_lock. x86 CMPXCHG instruction returns success in ZF flag, so this change saves a compare after cmpxchg (and related move instruction in front of cmpxchg).

Also, try_cmpxchg implicitly assigns old *ptr value to "old" when cmpxchg fails. There is no need to re-read the value in the loop.

Note that the value from *ptr should be read using READ_ONCE to prevent the compiler from merging, refetching or reordering the read.

No functional change intended.

Quoted 9 lines
Signed-off-by Uros Bizjak <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Len Brown <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Dave Hansen <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Acked-by Rafael J. Wysocki <[email protected]>

but please CC linux-acpi on all ACPI-related changes in the future.

Quoted 43 lines
---
 arch/x86/kernel/acpi/boot.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
arch/x86/kernel/acpi/boot.c +8 -9
index 907cc98b1938..4177577c173b 100644
@@ -1840,23 +1840,23 @@ early_param("acpi_sci", setup_acpi_sci);
 
 int __acpi_acquire_global_lock(unsigned int *lock)
 {
-       unsigned int old, new, val;
+       unsigned int old, new;
+
+       old = READ_ONCE(*lock);
        do {
-               old = *lock;
                new = (((old & ~0x3) + 2) + ((old >> 1) & 0x1));
-               val = cmpxchg(lock, old, new);
-       } while (unlikely (val != old));
+       } while (!try_cmpxchg(lock, &old, new));
        return ((new & 0x3) < 3) ? -1 : 0;
 }
 
 int __acpi_release_global_lock(unsigned int *lock)
 {
-       unsigned int old, new, val;
+       unsigned int old, new;
+
+       old = READ_ONCE(*lock);
        do {
-               old = *lock;
                new = old & ~0x3;
-               val = cmpxchg(lock, old, new);
-       } while (unlikely (val != old));
+       } while (!try_cmpxchg(lock, &old, new));
        return old & 0x1;
 }
 
--

2.39.0

#2 lore raw
Quoted 25 lines · Rafael J. Wysocki
Quoted 14 lines · Uros Bizjak

Use try_cmpxchg instead of cmpxchg (*ptr, old, new) == old in __acpi_{acquire,release}_global_lock. x86 CMPXCHG instruction returns success in ZF flag, so this change saves a compare after cmpxchg (and related move instruction in front of cmpxchg).

Also, try_cmpxchg implicitly assigns old *ptr value to "old" when cmpxchg fails. There is no need to re-read the value in the loop.

Note that the value from *ptr should be read using READ_ONCE to prevent the compiler from merging, refetching or reordering the read.

No functional change intended.

Quoted 9 lines
Signed-off-by Uros Bizjak <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Len Brown <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Dave Hansen <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Quoted 3 lines
Acked-by Rafael J. Wysocki <[email protected]>

but please CC linux-acpi on all ACPI-related changes in the future.

Thanks Rafael - I'll take this patch through the locking tree, if that's fine with you.

Thanks,

Ingo

maintainer
#3 lore raw
Quoted 29 lines · Ingo Molnar
Quoted 25 lines · Rafael J. Wysocki
Quoted 23 lines · Uros Bizjak

Use try_cmpxchg instead of cmpxchg (*ptr, old, new) == old in __acpi_{acquire,release}_global_lock. x86 CMPXCHG instruction returns success in ZF flag, so this change saves a compare after cmpxchg (and related move instruction in front of cmpxchg).

Also, try_cmpxchg implicitly assigns old *ptr value to "old" when cmpxchg fails. There is no need to re-read the value in the loop.

Note that the value from *ptr should be read using READ_ONCE to prevent the compiler from merging, refetching or reordering the read.

No functional change intended.

Signed-off-by Uros Bizjak <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Len Brown <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Dave Hansen <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Quoted 6 lines
Quoted 3 lines
Acked-by Rafael J. Wysocki <[email protected]>

but please CC linux-acpi on all ACPI-related changes in the future.

Thanks Rafael - I'll take this patch through the locking tree, if that's fine with you.

It is, thanks!

merge bot Applied
#4 lore raw

[tip: locking/core] x86/ACPI/boot: Use try_cmpxchg() in __acpi_{acquire,release}_global_lock()

In reply to Uros Bizjak
The following commit has been merged into the locking/core branch of tip:

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     890a0794b34f89fcd90e94ec970ad2bc18b70e73
Gitweb:        https://git.kernel.org/tip/890a0794b34f89fcd90e94ec970ad2bc18b70e73
Author:        Uros Bizjak <[email protected]>
AuthorDate:    Mon, 16 Jan 2023 17:25:22 +01:00
Committer:     Ingo Molnar <[email protected]>
CommitterDate: Thu, 26 Jan 2023 11:49:40 +01:00

x86/ACPI/boot: Use try_cmpxchg() in __acpi_{acquire,release}_global_lock()

Use try_cmpxchg instead of cmpxchg (*ptr, old, new) == old in __acpi_{acquire,release}_global_lock(). x86 CMPXCHG instruction returns success in ZF flag, so this change saves a compare after CMPXCHG (and related MOV instruction in front of CMPXCHG).

Also, try_cmpxchg() implicitly assigns old *ptr value to "old" when CMPXCHG fails. There is no need to re-read the value in the loop.

Note that the value from *ptr should be read using READ_ONCE() to prevent the compiler from merging, refetching or reordering the read.

No functional change intended.

Signed-off-by Uros Bizjak <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
Acked-by Rafael J. Wysocki <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
---
 arch/x86/kernel/acpi/boot.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
arch/x86/kernel/acpi/boot.c +8 -8
index 907cc98..4177577 100644
@@ -1840,23 +1840,23 @@ early_param("acpi_sci", setup_acpi_sci);
 
 int __acpi_acquire_global_lock(unsigned int *lock)
 {
-	unsigned int old, new, val;
+	unsigned int old, new;
+
+	old = READ_ONCE(*lock);
 	do {
-		old = *lock;
 		new = (((old & ~0x3) + 2) + ((old >> 1) & 0x1));
-		val = cmpxchg(lock, old, new);
-	} while (unlikely (val != old));
+	} while (!try_cmpxchg(lock, &old, new));
 	return ((new & 0x3) < 3) ? -1 : 0;
 }
 
 int __acpi_release_global_lock(unsigned int *lock)
 {
-	unsigned int old, new, val;
+	unsigned int old, new;
+
+	old = READ_ONCE(*lock);
 	do {
-		old = *lock;
 		new = old & ~0x3;
-		val = cmpxchg(lock, old, new);
-	} while (unlikely (val != old));
+	} while (!try_cmpxchg(lock, &old, new));
 	return old & 0x1;
 }