[1/5] arm64: Fix vDSO clock_getres()
diff mbox series

Message ID 20190401115152.32751-2-vincenzo.frascino@arm.com
State New
Headers show
Series
  • Fix vDSO clock_getres()
Related show

Commit Message

Vincenzo Frascino April 1, 2019, 11:51 a.m. UTC
clock_getres in the vDSO library has to preserve the same behaviour
of posix_get_hrtimer_res().

In particular, posix_get_hrtimer_res() does:
    sec = 0;
    ns = hrtimer_resolution;
and hrtimer_resolution depends on the enablement of the high
resolution timers that can happen either at compile or at run time.

Fix the arm64 vdso implementation of clock_getres keeping a copy of
hrtimer_resolution in vdso data and using that directly.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/vdso_datapage.h |  1 +
 arch/arm64/kernel/asm-offsets.c        |  2 +-
 arch/arm64/kernel/vdso.c               |  2 ++
 arch/arm64/kernel/vdso/gettimeofday.S  | 25 ++++++++++++++-----------
 4 files changed, 18 insertions(+), 12 deletions(-)

Comments

Catalin Marinas April 15, 2019, 5:35 p.m. UTC | #1
On Mon, Apr 01, 2019 at 12:51:48PM +0100, Vincenzo Frascino wrote:
> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
> index 2d419006ad43..47ba72345739 100644
> --- a/arch/arm64/kernel/vdso.c
> +++ b/arch/arm64/kernel/vdso.c
> @@ -245,6 +245,8 @@ void update_vsyscall(struct timekeeper *tk)
>  		vdso_data->cs_shift		= tk->tkr_mono.shift;
>  	}
>  
> +	vdso_data->hrtimer_res			= hrtimer_resolution;
> +
>  	smp_wmb();
>  	++vdso_data->tb_seq_count;
>  }
> diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S
> index c39872a7b03c..7a2cd2f8e13a 100644
> --- a/arch/arm64/kernel/vdso/gettimeofday.S
> +++ b/arch/arm64/kernel/vdso/gettimeofday.S
> @@ -296,32 +296,35 @@ ENDPROC(__kernel_clock_gettime)
>  /* int __kernel_clock_getres(clockid_t clock_id, struct timespec *res); */
>  ENTRY(__kernel_clock_getres)
>  	.cfi_startproc
> +	adr	vdso_data, _vdso_data
>  	cmp	w0, #CLOCK_REALTIME
>  	ccmp	w0, #CLOCK_MONOTONIC, #0x4, ne
>  	ccmp	w0, #CLOCK_MONOTONIC_RAW, #0x4, ne
> -	b.ne	1f
> +	b.ne	2f
>  
> -	ldr	x2, 5f
> -	b	2f
> -1:
> +1:	/* Get hrtimer_res */
> +	seqcnt_acquire
> +	syscall_check fail=5f
> +	ldr	x2, [vdso_data, #CLOCK_REALTIME_RES]
> +	seqcnt_check fail=1b
> +	b	3f
> +2:

We talked briefly but I'm still confused why we need the fallback to the
syscall here if archdata.vdso_direct is false. Is it because if the
timer driver code sets vdso_direct to false, we don't don't support
highres timers? If my understanding is correct, you may want to move the
hrtimer_res setting in update_vsyscall() to the !use_syscall block.
Vincenzo Frascino April 16, 2019, 12:42 p.m. UTC | #2
Hi Catalin,

On 15/04/2019 18:35, Catalin Marinas wrote:
> On Mon, Apr 01, 2019 at 12:51:48PM +0100, Vincenzo Frascino wrote:
>> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
>> index 2d419006ad43..47ba72345739 100644
>> --- a/arch/arm64/kernel/vdso.c
>> +++ b/arch/arm64/kernel/vdso.c
>> @@ -245,6 +245,8 @@ void update_vsyscall(struct timekeeper *tk)
>>  		vdso_data->cs_shift		= tk->tkr_mono.shift;
>>  	}
>>  
>> +	vdso_data->hrtimer_res			= hrtimer_resolution;
>> +
>>  	smp_wmb();
>>  	++vdso_data->tb_seq_count;
>>  }
>> diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S
>> index c39872a7b03c..7a2cd2f8e13a 100644
>> --- a/arch/arm64/kernel/vdso/gettimeofday.S
>> +++ b/arch/arm64/kernel/vdso/gettimeofday.S
>> @@ -296,32 +296,35 @@ ENDPROC(__kernel_clock_gettime)
>>  /* int __kernel_clock_getres(clockid_t clock_id, struct timespec *res); */
>>  ENTRY(__kernel_clock_getres)
>>  	.cfi_startproc
>> +	adr	vdso_data, _vdso_data
>>  	cmp	w0, #CLOCK_REALTIME
>>  	ccmp	w0, #CLOCK_MONOTONIC, #0x4, ne
>>  	ccmp	w0, #CLOCK_MONOTONIC_RAW, #0x4, ne
>> -	b.ne	1f
>> +	b.ne	2f
>>  
>> -	ldr	x2, 5f
>> -	b	2f
>> -1:
>> +1:	/* Get hrtimer_res */
>> +	seqcnt_acquire
>> +	syscall_check fail=5f
>> +	ldr	x2, [vdso_data, #CLOCK_REALTIME_RES]
>> +	seqcnt_check fail=1b
>> +	b	3f
>> +2:
> 
> We talked briefly but I'm still confused why we need the fallback to the
> syscall here if archdata.vdso_direct is false. Is it because if the
> timer driver code sets vdso_direct to false, we don't don't support
> highres timers? If my understanding is correct, you may want to move the
> hrtimer_res setting in update_vsyscall() to the !use_syscall block.
> 

Ok, so let me try to provide more details on what I mentioned yesterday:
- clock_getres syscall follows the rules of what defined in posix-timers.c
- based on the clock_id that, for this purpose, can be separated in coarse and
non-coarse calls either posix_get_coarse_res() or posix_get_hrtimer_res().
- if clock id is set to a coarse clock and posix_get_coarse_res() is invoked,
happens what follows:

static int posix_get_coarse_res(const clockid_t which_clock,
				struct timespec64 *tp)
{
	*tp = ktime_to_timespec64(KTIME_LOW_RES);
	return 0;
}

Note that since CONFIG_1HZ seems not supported (jiffies.h) by the kernel in this
case we do not need rounding in our vDSO implementation.

- if clock id is set to non-coarse and posix_get_hrtimer_res() is invoked,
happens the following:

static int posix_get_hrtimer_res(clockid_t which_clock, struct timespec64 *tp)
{
	tp->tv_sec = 0;
	tp->tv_nsec = hrtimer_resolution;
	return 0;
}

hrtimer_resolution can be high res or low res depending on the call of
hrtimer_switch_to_hres(). For us the only way to preserve the correct value is
to keep it in the vdso data page.

- The assembly code mimics exactly the same behaviour detailed above, with one
difference: the one related to the use_syscall parameter which is specific to arm64.
The use_syscall parameter is set by arm_arch_timer and consumed by
update_vsyscall(). To mirror what update_vsyscall does in update_vsyscall() I
check "syscall_check fail=5f" in clock_getres vdso function.

Said that, even if functionally it is the same thing, I think it is logically
more correct to have hrtimer_res setting inside the !use_syscall block, hence I
am going to change it in the next iteration.

Please let me know your thoughts.
Will Deacon April 16, 2019, 2:23 p.m. UTC | #3
On Tue, Apr 16, 2019 at 01:42:58PM +0100, Vincenzo Frascino wrote:
> On 15/04/2019 18:35, Catalin Marinas wrote:
> > On Mon, Apr 01, 2019 at 12:51:48PM +0100, Vincenzo Frascino wrote:
> >> +1:	/* Get hrtimer_res */
> >> +	seqcnt_acquire
> >> +	syscall_check fail=5f
> >> +	ldr	x2, [vdso_data, #CLOCK_REALTIME_RES]
> >> +	seqcnt_check fail=1b
> >> +	b	3f
> >> +2:
> > 
> > We talked briefly but I'm still confused why we need the fallback to the
> > syscall here if archdata.vdso_direct is false. Is it because if the
> > timer driver code sets vdso_direct to false, we don't don't support
> > highres timers? If my understanding is correct, you may want to move the
> > hrtimer_res setting in update_vsyscall() to the !use_syscall block.
> > 
> 
> Ok, so let me try to provide more details on what I mentioned yesterday:
> - clock_getres syscall follows the rules of what defined in posix-timers.c
> - based on the clock_id that, for this purpose, can be separated in coarse and
> non-coarse calls either posix_get_coarse_res() or posix_get_hrtimer_res().
> - if clock id is set to a coarse clock and posix_get_coarse_res() is invoked,
> happens what follows:
> 
> static int posix_get_coarse_res(const clockid_t which_clock,
> 				struct timespec64 *tp)
> {
> 	*tp = ktime_to_timespec64(KTIME_LOW_RES);
> 	return 0;
> }
> 
> Note that since CONFIG_1HZ seems not supported (jiffies.h) by the kernel in this
> case we do not need rounding in our vDSO implementation.
> 
> - if clock id is set to non-coarse and posix_get_hrtimer_res() is invoked,
> happens the following:
> 
> static int posix_get_hrtimer_res(clockid_t which_clock, struct timespec64 *tp)
> {
> 	tp->tv_sec = 0;
> 	tp->tv_nsec = hrtimer_resolution;
> 	return 0;
> }
> 
> hrtimer_resolution can be high res or low res depending on the call of
> hrtimer_switch_to_hres(). For us the only way to preserve the correct value is
> to keep it in the vdso data page.
> 
> - The assembly code mimics exactly the same behaviour detailed above, with one
> difference: the one related to the use_syscall parameter which is specific to arm64.
> The use_syscall parameter is set by arm_arch_timer and consumed by
> update_vsyscall(). To mirror what update_vsyscall does in update_vsyscall() I
> check "syscall_check fail=5f" in clock_getres vdso function.
> 
> Said that, even if functionally it is the same thing, I think it is logically
> more correct to have hrtimer_res setting inside the !use_syscall block, hence I
> am going to change it in the next iteration.
> 
> Please let me know your thoughts.

I think you can ignore the syscall_check, just like we seem to do for
CLOCK_REALTIME_COARSE and CLOCK_MONOTONIC_COARSE in clock_gettime().

Will

Patch
diff mbox series

diff --git a/arch/arm64/include/asm/vdso_datapage.h b/arch/arm64/include/asm/vdso_datapage.h
index 2b9a63771eda..f89263c8e11a 100644
--- a/arch/arm64/include/asm/vdso_datapage.h
+++ b/arch/arm64/include/asm/vdso_datapage.h
@@ -38,6 +38,7 @@  struct vdso_data {
 	__u32 tz_minuteswest;	/* Whacky timezone stuff */
 	__u32 tz_dsttime;
 	__u32 use_syscall;
+	__u32 hrtimer_res;
 };
 
 #endif /* !__ASSEMBLY__ */
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 7f40dcbdd51d..e10e2a5d9ddc 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -94,7 +94,7 @@  int main(void)
   DEFINE(CLOCK_REALTIME,	CLOCK_REALTIME);
   DEFINE(CLOCK_MONOTONIC,	CLOCK_MONOTONIC);
   DEFINE(CLOCK_MONOTONIC_RAW,	CLOCK_MONOTONIC_RAW);
-  DEFINE(CLOCK_REALTIME_RES,	MONOTONIC_RES_NSEC);
+  DEFINE(CLOCK_REALTIME_RES,	offsetof(struct vdso_data, hrtimer_res));
   DEFINE(CLOCK_REALTIME_COARSE,	CLOCK_REALTIME_COARSE);
   DEFINE(CLOCK_MONOTONIC_COARSE,CLOCK_MONOTONIC_COARSE);
   DEFINE(CLOCK_COARSE_RES,	LOW_RES_NSEC);
diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index 2d419006ad43..47ba72345739 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -245,6 +245,8 @@  void update_vsyscall(struct timekeeper *tk)
 		vdso_data->cs_shift		= tk->tkr_mono.shift;
 	}
 
+	vdso_data->hrtimer_res			= hrtimer_resolution;
+
 	smp_wmb();
 	++vdso_data->tb_seq_count;
 }
diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S
index c39872a7b03c..7a2cd2f8e13a 100644
--- a/arch/arm64/kernel/vdso/gettimeofday.S
+++ b/arch/arm64/kernel/vdso/gettimeofday.S
@@ -296,32 +296,35 @@  ENDPROC(__kernel_clock_gettime)
 /* int __kernel_clock_getres(clockid_t clock_id, struct timespec *res); */
 ENTRY(__kernel_clock_getres)
 	.cfi_startproc
+	adr	vdso_data, _vdso_data
 	cmp	w0, #CLOCK_REALTIME
 	ccmp	w0, #CLOCK_MONOTONIC, #0x4, ne
 	ccmp	w0, #CLOCK_MONOTONIC_RAW, #0x4, ne
-	b.ne	1f
+	b.ne	2f
 
-	ldr	x2, 5f
-	b	2f
-1:
+1:	/* Get hrtimer_res */
+	seqcnt_acquire
+	syscall_check fail=5f
+	ldr	x2, [vdso_data, #CLOCK_REALTIME_RES]
+	seqcnt_check fail=1b
+	b	3f
+2:
 	cmp	w0, #CLOCK_REALTIME_COARSE
 	ccmp	w0, #CLOCK_MONOTONIC_COARSE, #0x4, ne
-	b.ne	4f
+	b.ne	5f
 	ldr	x2, 6f
-2:
-	cbz	x1, 3f
+3:
+	cbz	x1, 4f
 	stp	xzr, x2, [x1]
 
-3:	/* res == NULL. */
+4:	/* res == NULL. */
 	mov	w0, wzr
 	ret
 
-4:	/* Syscall fallback. */
+5:	/* Syscall fallback. */
 	mov	x8, #__NR_clock_getres
 	svc	#0
 	ret
-5:
-	.quad	CLOCK_REALTIME_RES
 6:
 	.quad	CLOCK_COARSE_RES
 	.cfi_endproc