| Message ID | 1424820067-13112-1-git-send-email-nathan_lynch@mentor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 24, 2015 at 11:21:07PM +0000, Nathan Lynch wrote: > The vdso implementation of clock_getres currently returns 0 (success) > whenever a null timespec is provided by the caller, regardless of the > clock id supplied. > > This behavior is incorrect. It should fall back to syscall when an > unrecognized clock id is passed, even when the timespec argument is > null. This ensures that clock_getres always returns an error for > invalid clock ids. > > Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com> > --- > arch/arm64/kernel/vdso/gettimeofday.S | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Acked-by: Will Deacon <will.deacon@arm.com> Thanks, Nathan. Will > diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S > index fe652ffd34c2..efa79e8d4196 100644 > --- a/arch/arm64/kernel/vdso/gettimeofday.S > +++ b/arch/arm64/kernel/vdso/gettimeofday.S > @@ -174,8 +174,6 @@ ENDPROC(__kernel_clock_gettime) > /* int __kernel_clock_getres(clockid_t clock_id, struct timespec *res); */ > ENTRY(__kernel_clock_getres) > .cfi_startproc > - cbz w1, 3f > - > cmp w0, #CLOCK_REALTIME > ccmp w0, #CLOCK_MONOTONIC, #0x4, ne > b.ne 1f > @@ -188,6 +186,7 @@ ENTRY(__kernel_clock_getres) > b.ne 4f > ldr x2, 6f > 2: > + cbz w1, 3f > stp xzr, x2, [x1] > > 3: /* res == NULL. */ > -- > 1.9.3 > >
On 02/25/2015 10:02 AM, Will Deacon wrote: > On Tue, Feb 24, 2015 at 11:21:07PM +0000, Nathan Lynch wrote: >> The vdso implementation of clock_getres currently returns 0 (success) >> whenever a null timespec is provided by the caller, regardless of the >> clock id supplied. >> >> This behavior is incorrect. It should fall back to syscall when an >> unrecognized clock id is passed, even when the timespec argument is >> null. This ensures that clock_getres always returns an error for >> invalid clock ids. >> >> Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com> >> --- >> arch/arm64/kernel/vdso/gettimeofday.S | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) > > Acked-by: Will Deacon <will.deacon@arm.com> > > Thanks, Nathan. Thank you. I'm curious -- do you know of a use case for the VDSO implementation of clock_getres? I.e. what kind of real-world workload sees a benefit from it? I'm not suggesting removing it from the arm64 vdso, but I'm considering dropping clock_getres from the 32-bit ARM vdso patch set since I haven't been able to answer this.
On Wed, Feb 25, 2015 at 04:14:19PM +0000, Nathan Lynch wrote: > On 02/25/2015 10:02 AM, Will Deacon wrote: > > On Tue, Feb 24, 2015 at 11:21:07PM +0000, Nathan Lynch wrote: > >> The vdso implementation of clock_getres currently returns 0 (success) > >> whenever a null timespec is provided by the caller, regardless of the > >> clock id supplied. > >> > >> This behavior is incorrect. It should fall back to syscall when an > >> unrecognized clock id is passed, even when the timespec argument is > >> null. This ensures that clock_getres always returns an error for > >> invalid clock ids. > >> > >> Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com> > >> --- > >> arch/arm64/kernel/vdso/gettimeofday.S | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > > > > Acked-by: Will Deacon <will.deacon@arm.com> > > > > Thanks, Nathan. > > Thank you. I'm curious -- do you know of a use case for the VDSO > implementation of clock_getres? I.e. what kind of real-world workload > sees a benefit from it? No, I just implemented it for completeness (and it's also way simpler than the other functions!). Calling it with a NULL timespec is probably even less common, so an alternative to your patch would be changing the label of the existing cbz to hand-off to the kernel when the thing is NULL. > I'm not suggesting removing it from the arm64 vdso, but I'm considering > dropping clock_getres from the 32-bit ARM vdso patch set since I haven't > been able to answer this. Well, if there aren't any fastpath users then I'm not bothered about removing it from arm64 either. I'm not sure how we establish that, though. Will
On 02/25/2015 10:32 AM, Will Deacon wrote: > On Wed, Feb 25, 2015 at 04:14:19PM +0000, Nathan Lynch wrote: >> On 02/25/2015 10:02 AM, Will Deacon wrote: >>> On Tue, Feb 24, 2015 at 11:21:07PM +0000, Nathan Lynch wrote: >>>> The vdso implementation of clock_getres currently returns 0 (success) >>>> whenever a null timespec is provided by the caller, regardless of the >>>> clock id supplied. >>>> >>>> This behavior is incorrect. It should fall back to syscall when an >>>> unrecognized clock id is passed, even when the timespec argument is >>>> null. This ensures that clock_getres always returns an error for >>>> invalid clock ids. >>>> >>>> Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com> >>>> --- >>>> arch/arm64/kernel/vdso/gettimeofday.S | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> Acked-by: Will Deacon <will.deacon@arm.com> >>> >>> Thanks, Nathan. >> >> Thank you. I'm curious -- do you know of a use case for the VDSO >> implementation of clock_getres? I.e. what kind of real-world workload >> sees a benefit from it? > > No, I just implemented it for completeness (and it's also way simpler than > the other functions!). Yeah it was the first function I implemented for the arm32 vdso :-) > Calling it with a NULL timespec is probably even > less common, so an alternative to your patch would be changing the label > of the existing cbz to hand-off to the kernel when the thing is NULL. Yes, but I'll leave it alone unless you'd prefer that to this patch. >> I'm not suggesting removing it from the arm64 vdso, but I'm considering >> dropping clock_getres from the 32-bit ARM vdso patch set since I haven't >> been able to answer this. > > Well, if there aren't any fastpath users then I'm not bothered about > removing it from arm64 either. I'm not sure how we establish that, though. FWIW x86 has never implemented it, which makes me think there's little demand for it. I stumbled upon another reason to omit clock_getres from the arm32 vdso. If you have CONFIG_HIGHRES_TIMERS=y and the kernel doesn't switch to high-res mode for whatever reason (this happens for me in qemu), the vdso clock_getres returns a different result than the syscall does: # vdsotest clock-getres-monotonic verify clock resolutions differ: [0, 10000000] (kernel) [0, 1] (vDSO) Not sure whether this is a concern for arm64 since I think the only way to provoke it there is to boot with clocksource=jiffies.
diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S index fe652ffd34c2..efa79e8d4196 100644 --- a/arch/arm64/kernel/vdso/gettimeofday.S +++ b/arch/arm64/kernel/vdso/gettimeofday.S @@ -174,8 +174,6 @@ ENDPROC(__kernel_clock_gettime) /* int __kernel_clock_getres(clockid_t clock_id, struct timespec *res); */ ENTRY(__kernel_clock_getres) .cfi_startproc - cbz w1, 3f - cmp w0, #CLOCK_REALTIME ccmp w0, #CLOCK_MONOTONIC, #0x4, ne b.ne 1f @@ -188,6 +186,7 @@ ENTRY(__kernel_clock_getres) b.ne 4f ldr x2, 6f 2: + cbz w1, 3f stp xzr, x2, [x1] 3: /* res == NULL. */
The vdso implementation of clock_getres currently returns 0 (success) whenever a null timespec is provided by the caller, regardless of the clock id supplied. This behavior is incorrect. It should fall back to syscall when an unrecognized clock id is passed, even when the timespec argument is null. This ensures that clock_getres always returns an error for invalid clock ids. Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com> --- arch/arm64/kernel/vdso/gettimeofday.S | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)