Skip to contentSkip to conversation

arm+arm64: vdso unification to lib/vdso/

PATCH Superseded Mark Salyzyn 3 participants 5 messages Oct 27, 2017 – Oct 31, 2017 arm64

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v3 cover arm+arm64: vdso unification to lib/vdso/

Take an effort to recode the arm64 vdso code from assembler to C previously submitted by Andrew Pinski <[email protected]>, rework it for use in both arm and arm64, overlapping any optimizations for each architecture. But instead of landing it in arm64, land the result into lib/vdso and unify both implementations to simplify future maintenance. This will act as the basis for implementing arm64 vdso32 in the future.

apinski at cavium.com made the following claims in the original patch:

This allows the compiler to optimize the divide by 1000 and remove the other divides.

On ThunderX, gettimeofday improves by 32%. On ThunderX 2, gettimeofday improves by 18%.

Note I noticed a bug in the old implementation of __kernel_clock_getres; it was checking only the lower 32bits of the pointer; this would work for most cases but could fail in a few.

Cc James Morse <[email protected]>
Cc Russell King <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Dmitry Safonov <[email protected]>
Cc John Stultz <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Laura Abbott <[email protected]>
Cc Kees Cook <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Andy Gross <[email protected]>
Cc Kevin Brodsky <[email protected]>
Cc Andrew Pinski <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc linux-arm-kernel at lists.infradead.org

v2:
- split first CL into 7 pieces, there were cosmetic adjustments.
- make sure profiling is turned off.
- kept quiet_cmd_vdsoas.

v3:
- changed are a result of private email review comments
- rebase
- move arch/arm/vdso/vgettimeofday.c to lib/vdso/vgettimeofday.c
- adjust vgettimeofday.c to be a better global candidate, switch to using ARCH_PROVIDES_TIMER and __arch_counter_get() as more generic.
- do not expose gettimeofday if arch does not support user space timer

reviewer
#1 lore raw

Hi,

Quoted 3 lines · Mark Salyzyn

Note I noticed a bug in the old implementation of __kernel_clock_getres; it was checking only the lower 32bits of the pointer; this would work for most cases but could fail in a few.

Sorry if this is a stupid question, but do you mean from a prior version of this series, or the one in the kernel today?

Thanks,
Mark.

#2 lore raw
Quoted 8 lines · Mark Rutland

Hi,

Quoted 3 lines · Mark Salyzyn

Note I noticed a bug in the old implementation of __kernel_clock_getres; it was checking only the lower 32bits of the pointer; this would work for most cases but could fail in a few.

Sorry if this is a stupid question, but do you mean from a prior version of this series, or the one in the kernel today?

apinski at cavium.com noticed this as part of the existing upstream arm64 assembler when he did the original conversion to C.

Yes, I am aware that a separate patch will need to be made on the assembler code for 'stable' trees. And it needs to be addressed (separately from this patch series IMHO, but if forced, it would be a pre-patch that gets swallowed in patch 10 of the series).

I have not looked into it in depth, because from Android's point of view, this patch series (once approved) is going to be back-ported to our android-common kernels and will be the solution to the problem. I must declare I have to be humble about my understanding of arm64 assembler, so there is _that_ ...

-- MarkS

reviewer
#3 lore raw
Quoted 10 lines · Mark Salyzyn
Quoted 6 lines · Mark Rutland
Quoted 3 lines · Mark Salyzyn

Note I noticed a bug in the old implementation of __kernel_clock_getres; it was checking only the lower 32bits of the pointer; this would work for most cases but could fail in a few.

Sorry if this is a stupid question, but do you mean from a prior version of this series, or the one in the kernel today?

apinski at cavium.com noticed this as part of the existing upstream arm64 assembler when he did the original conversion to C.

Just to check, does the below address the issue, or is there something that I've missed?

Thanks,
Mark.

---->8----

Quoted 1 line

From 3557ca259f0a6dbb35a057256552545ec0cef677 Mon Sep 17 00:00:00 2001

From: Mark Rutland <[email protected]>
Date: Mon, 30 Oct 2017 21:23:19 +0000
Subject: [PATCH] arm64: vdso: fix clock_getres for 4GiB-aligned res

The vdso tries to check for a NULL res pointer in __kernel_clock_getres, but only checks the lower 32 bits as is uses CBZ on the W register the res pointer is held in.

Thus, if the res pointer happened to be aligned to a 4GiB boundary, we'd spuriously skip storing the timespec to it, while returning a zero error code to the caller.

Prevent this by checking the whole pointer, using CBZ on the X register the res pointer is held in.

Fixes 9031fefde6f2ac1d ("arm64: VDSO support")
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/kernel/vdso/gettimeofday.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kernel/vdso/gettimeofday.S +1 -1
index 76320e920965..c39872a7b03c 100644
@@ -309,7 +309,7 @@ ENTRY(__kernel_clock_getres)
 	b.ne	4f
 	ldr	x2, 6f
 2:
-	cbz	w1, 3f
+	cbz	x1, 3f
 	stp	xzr, x2, [x1]
 
 3:	/* res == NULL. */
Signature · 1 line
2.11.0
#4 lore raw
Quoted 6 lines · Mark Rutland
Quoted 5 lines · Mark Salyzyn
Quoted 4 lines · Mark Rutland
Quoted 3 lines · Mark Salyzyn

Note I noticed a bug in the old implementation of __kernel_clock_getres; it was checking only the lower 32bits of the pointer; this would work for most cases but could fail in a few.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Sorry if this is a stupid question, but do you mean from a prior version of this series, or the one in the kernel today?

[email protected] noticed this as part of the existing upstream arm64 assembler when he did the original conversion to C.

Just to check, does the below address the issue, or is there something that I've missed?

Looks fine to me. Applied.

Will