In order to simplify next step which moves fallback call at arch
level, ensure all arches have a 32bit fallback instead of handling
the lack of 32bit fallback in the common code based
on VDSO_HAS_32BIT_FALLBACK
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/arm/include/asm/vdso/gettimeofday.h | 26 +++++++++++++++++++++
arch/arm64/include/asm/vdso/compat_gettimeofday.h | 2 --
arch/arm64/include/asm/vdso/gettimeofday.h | 26 +++++++++++++++++++++
arch/mips/include/asm/vdso/gettimeofday.h | 28 +++++++++++++++++++++--
arch/x86/include/asm/vdso/gettimeofday.h | 28 +++++++++++++++++++++--
lib/vdso/gettimeofday.c | 10 --------
6 files changed, 104 insertions(+), 16 deletions(-)
diff --git a/arch/arm/include/asm/vdso/gettimeofday.h b/arch/arm/include/asm/vdso/gettimeofday.h
index 0ad2429c324f..55f8ad6e7777 100644
--- a/arch/arm/include/asm/vdso/gettimeofday.h
+++ b/arch/arm/include/asm/vdso/gettimeofday.h
@@ -70,6 +70,32 @@ static __always_inline int clock_getres_fallback(
return ret;
}
+static __always_inline
+long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
+{
+ struct __kernel_timespec ts;
+ int ret = clock_gettime_fallback(clock, &ts);
+
+ if (likely(!ret)) {
+ _ts->tv_sec = ts.tv_sec;
+ _ts->tv_nsec = ts.tv_nsec;
+ }
+ return ret;
+}
+
+static __always_inline
+long clock_getres32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
+{
+ struct __kernel_timespec ts;
+ int ret = clock_getres_fallback(clock, &ts);
+
+ if (likely(!ret && _ts)) {
+ _ts->tv_sec = ts.tv_sec;
+ _ts->tv_nsec = ts.tv_nsec;
+ }
+ return ret;
+}
+
static __always_inline u64 __arch_get_hw_counter(int clock_mode)
{
#ifdef CONFIG_ARM_ARCH_TIMER
diff --git a/arch/arm64/include/asm/vdso/compat_gettimeofday.h b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
index c50ee1b7d5cd..bab700e37a03 100644
--- a/arch/arm64/include/asm/vdso/compat_gettimeofday.h
+++ b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
@@ -16,8 +16,6 @@
#define VDSO_HAS_CLOCK_GETRES 1
-#define VDSO_HAS_32BIT_FALLBACK 1
-
static __always_inline
int gettimeofday_fallback(struct __kernel_old_timeval *_tv,
struct timezone *_tz)
diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h b/arch/arm64/include/asm/vdso/gettimeofday.h
index b08f476b72b4..c41c86a07423 100644
--- a/arch/arm64/include/asm/vdso/gettimeofday.h
+++ b/arch/arm64/include/asm/vdso/gettimeofday.h
@@ -66,6 +66,32 @@ int clock_getres_fallback(clockid_t _clkid, struct __kernel_timespec *_ts)
return ret;
}
+static __always_inline
+long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
+{
+ struct __kernel_timespec ts;
+ int ret = clock_gettime_fallback(clock, &ts);
+
+ if (likely(!ret)) {
+ _ts->tv_sec = ts.tv_sec;
+ _ts->tv_nsec = ts.tv_nsec;
+ }
+ return ret;
+}
+
+static __always_inline
+long clock_getres32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
+{
+ struct __kernel_timespec ts;
+ int ret = clock_getres_fallback(clock, &ts);
+
+ if (likely(!ret && _ts)) {
+ _ts->tv_sec = ts.tv_sec;
+ _ts->tv_nsec = ts.tv_nsec;
+ }
+ return ret;
+}
+
static __always_inline u64 __arch_get_hw_counter(s32 clock_mode)
{
u64 res;
diff --git a/arch/mips/include/asm/vdso/gettimeofday.h b/arch/mips/include/asm/vdso/gettimeofday.h
index b08825531e9f..60608e930a5c 100644
--- a/arch/mips/include/asm/vdso/gettimeofday.h
+++ b/arch/mips/include/asm/vdso/gettimeofday.h
@@ -109,8 +109,6 @@ static __always_inline int clock_getres_fallback(
#if _MIPS_SIM != _MIPS_SIM_ABI64
-#define VDSO_HAS_32BIT_FALLBACK 1
-
static __always_inline long clock_gettime32_fallback(
clockid_t _clkid,
struct old_timespec32 *_ts)
@@ -150,6 +148,32 @@ static __always_inline int clock_getres32_fallback(
return error ? -ret : ret;
}
+#else
+static __always_inline
+long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
+{
+ struct __kernel_timespec ts;
+ int ret = clock_gettime_fallback(clock, &ts);
+
+ if (likely(!ret)) {
+ _ts->tv_sec = ts.tv_sec;
+ _ts->tv_nsec = ts.tv_nsec;
+ }
+ return ret;
+}
+
+static __always_inline
+long clock_getres32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
+{
+ struct __kernel_timespec ts;
+ int ret = clock_getres_fallback(clock, &ts);
+
+ if (likely(!ret && _ts)) {
+ _ts->tv_sec = ts.tv_sec;
+ _ts->tv_nsec = ts.tv_nsec;
+ }
+ return ret;
+}
#endif
#ifdef CONFIG_CSRC_R4K
diff --git a/arch/x86/include/asm/vdso/gettimeofday.h b/arch/x86/include/asm/vdso/gettimeofday.h
index e9ee139cf29e..e1e16c2fdba0 100644
--- a/arch/x86/include/asm/vdso/gettimeofday.h
+++ b/arch/x86/include/asm/vdso/gettimeofday.h
@@ -94,9 +94,33 @@ long clock_getres_fallback(clockid_t _clkid, struct __kernel_timespec *_ts)
return ret;
}
-#else
+static __always_inline
+long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
+{
+ struct __kernel_timespec ts;
+ int ret = clock_gettime_fallback(clock, &ts);
-#define VDSO_HAS_32BIT_FALLBACK 1
+ if (likely(!ret)) {
+ _ts->tv_sec = ts.tv_sec;
+ _ts->tv_nsec = ts.tv_nsec;
+ }
+ return ret;
+}
+
+static __always_inline
+long clock_getres32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
+{
+ struct __kernel_timespec ts;
+ int ret = clock_getres_fallback(clock, &ts);
+
+ if (likely(!ret && _ts)) {
+ _ts->tv_sec = ts.tv_sec;
+ _ts->tv_nsec = ts.tv_nsec;
+ }
+ return ret;
+}
+
+#else
static __always_inline
long clock_gettime_fallback(clockid_t _clkid, struct __kernel_timespec *_ts)
diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c
index 9ecfd3b547ba..59189ed49352 100644
--- a/lib/vdso/gettimeofday.c
+++ b/lib/vdso/gettimeofday.c
@@ -125,13 +125,8 @@ __cvdso_clock_gettime32(clockid_t clock, struct old_timespec32 *res)
ret = __cvdso_clock_gettime_common(clock, &ts);
-#ifdef VDSO_HAS_32BIT_FALLBACK
if (unlikely(ret))
return clock_gettime32_fallback(clock, res);
-#else
- if (unlikely(ret))
- ret = clock_gettime_fallback(clock, &ts);
-#endif
if (likely(!ret)) {
res->tv_sec = ts.tv_sec;
@@ -238,13 +233,8 @@ __cvdso_clock_getres_time32(clockid_t clock, struct old_timespec32 *res)
ret = __cvdso_clock_getres_common(clock, &ts);
-#ifdef VDSO_HAS_32BIT_FALLBACK
if (unlikely(ret))
return clock_getres32_fallback(clock, res);
-#else
- if (unlikely(ret))
- ret = clock_getres_fallback(clock, &ts);
-#endif
if (likely(!ret && res)) {
res->tv_sec = ts.tv_sec;
--
2.13.3
On Mon, Dec 23, 2019 at 6:31 AM Christophe Leroy
<[email protected]> wrote:
>
> In order to simplify next step which moves fallback call at arch
> level, ensure all arches have a 32bit fallback instead of handling
> the lack of 32bit fallback in the common code based
> on VDSO_HAS_32BIT_FALLBACK
I don't like this. You've implemented what appear to be nonsensical
fallbacks (the 32-bit fallback for a 64-bit vDSO build? There's no
such thing).
How exactly does this simplify patch 2?
--Andy
On Mon, Dec 23, 2019 at 3:31 PM Christophe Leroy
<[email protected]> wrote:
>
> In order to simplify next step which moves fallback call at arch
> level, ensure all arches have a 32bit fallback instead of handling
> the lack of 32bit fallback in the common code based
> on VDSO_HAS_32BIT_FALLBACK
>
> Signed-off-by: Christophe Leroy <[email protected]>
I like the idea of removing VDSO_HAS_32BIT_FALLBACK and ensuring
that all 32-bit architectures implement them, but we really should *not*
have any implementation calling the 64-bit syscalls.
> +static __always_inline
> +long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
> +{
> + struct __kernel_timespec ts;
> + int ret = clock_gettime_fallback(clock, &ts);
> +
> + if (likely(!ret)) {
> + _ts->tv_sec = ts.tv_sec;
> + _ts->tv_nsec = ts.tv_nsec;
> + }
> + return ret;
> +}
> +
> +static __always_inline
> +long clock_getres32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
> +{
> + struct __kernel_timespec ts;
> + int ret = clock_getres_fallback(clock, &ts);
> +
> + if (likely(!ret && _ts)) {
> + _ts->tv_sec = ts.tv_sec;
> + _ts->tv_nsec = ts.tv_nsec;
> + }
> + return ret;
> +}
Please change these to call __NR_clock_gettime and __NR_clock_getres_time
instead of __NR_clock_gettime64/__NR_clock_getres_time64 for multiple reasons.
- When doing migration between containers, the vdso may get copied into
an application running on a kernel that does not support the time64
variants, and then the fallback fails.
- When CONFIG_COMPAT_32BIT_TIME is disabled, the time32 syscalls
return -ENOSYS, and the vdso version should have the exact same behavior
to avoid surprises. In particular an application that checks clock_gettime()
to see if the time32 are in part of the kernel would get an incorrect result
here.
arch/arm64/include/asm/vdso/compat_gettimeofday.h already does this,
I think you can just copy the implementation or find a way to share it.
> diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h b/arch/arm64/include/asm/vdso/gettimeofday.h
> index b08f476b72b4..c41c86a07423 100644
> --- a/arch/arm64/include/asm/vdso/gettimeofday.h
> +++ b/arch/arm64/include/asm/vdso/gettimeofday.h
> @@ -66,6 +66,32 @@ int clock_getres_fallback(clockid_t _clkid, struct __kernel_timespec *_ts)
> return ret;
> }
>
> +static __always_inline
> +long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
> +{
> + struct __kernel_timespec ts;
> + int ret = clock_gettime_fallback(clock, &ts);
> +
> + if (likely(!ret)) {
> + _ts->tv_sec = ts.tv_sec;
> + _ts->tv_nsec = ts.tv_nsec;
> + }
> + return ret;
> +}
As Andy said, this makes no sense at all, nothing should ever call this on a
64-bit architecture.
> diff --git a/arch/mips/include/asm/vdso/gettimeofday.h b/arch/mips/include/asm/vdso/gettimeofday.h
> index b08825531e9f..60608e930a5c 100644
> --- a/arch/mips/include/asm/vdso/gettimeofday.h
> +++ b/arch/mips/include/asm/vdso/gettimeofday.h
> @@ -109,8 +109,6 @@ static __always_inline int clock_getres_fallback(
>
> #if _MIPS_SIM != _MIPS_SIM_ABI64
>
> -#define VDSO_HAS_32BIT_FALLBACK 1
> -
> static __always_inline long clock_gettime32_fallback(
> clockid_t _clkid,
> struct old_timespec32 *_ts)
> @@ -150,6 +148,32 @@ static __always_inline int clock_getres32_fallback(
>
> return error ? -ret : ret;
> }
> +#else
> +static __always_inline
> +long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
> +{
> + struct __kernel_timespec ts;
> + int ret = clock_gettime_fallback(clock, &ts);
> +
> + if (likely(!ret)) {
> + _ts->tv_sec = ts.tv_sec;
> + _ts->tv_nsec = ts.tv_nsec;
> + }
> + return ret;
> +}
> +
Same here.
> --- a/lib/vdso/gettimeofday.c
> +++ b/lib/vdso/gettimeofday.c
> @@ -125,13 +125,8 @@ __cvdso_clock_gettime32(clockid_t clock, struct old_timespec32 *res)
>
> ret = __cvdso_clock_gettime_common(clock, &ts);
>
> -#ifdef VDSO_HAS_32BIT_FALLBACK
> if (unlikely(ret))
> return clock_gettime32_fallback(clock, res);
> -#else
> - if (unlikely(ret))
> - ret = clock_gettime_fallback(clock, &ts);
> -#endif
>
> if (likely(!ret)) {
> res->tv_sec = ts.tv_sec;
Removing the #ifdef and the fallback seems fine. I think this is actually
required for correctness on arm32 as well. Maybe enclose the entire function in
#ifdef VDSO_HAS_CLOCK_GETTIME32
to only define it when it is called?
> @@ -238,13 +233,8 @@ __cvdso_clock_getres_time32(clockid_t clock, struct old_timespec32 *res)
>
> ret = __cvdso_clock_getres_common(clock, &ts);
>
> -#ifdef VDSO_HAS_32BIT_FALLBACK
> if (unlikely(ret))
> return clock_getres32_fallback(clock, res);
> -#else
> - if (unlikely(ret))
> - ret = clock_getres_fallback(clock, &ts);
> -#endif
The same applies to all the getres stuff of course.
Arnd
On Mon, Dec 30, 2019 at 1:27 PM Arnd Bergmann <[email protected]> wrote:
> On Mon, Dec 23, 2019 at 3:31 PM Christophe Leroy <[email protected]> wrote:
> > +static __always_inline
> > +long clock_getres32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
> > +{
> > + struct __kernel_timespec ts;
> > + int ret = clock_getres_fallback(clock, &ts);
> > +
> > + if (likely(!ret && _ts)) {
> > + _ts->tv_sec = ts.tv_sec;
> > + _ts->tv_nsec = ts.tv_nsec;
> > + }
> > + return ret;
> > +}
>
> Please change these to call __NR_clock_gettime and __NR_clock_getres_time
> instead of __NR_clock_gettime64/__NR_clock_getres_time64 for multiple reasons.
>
> - When doing migration between containers, the vdso may get copied into
> an application running on a kernel that does not support the time64
> variants, and then the fallback fails.
>
> - When CONFIG_COMPAT_32BIT_TIME is disabled, the time32 syscalls
> return -ENOSYS, and the vdso version should have the exact same behavior
> to avoid surprises. In particular an application that checks clock_gettime()
> to see if the time32 are in part of the kernel would get an incorrect result
> here.
>
> arch/arm64/include/asm/vdso/compat_gettimeofday.h already does this,
> I think you can just copy the implementation or find a way to share it.
There was a related discussion on this after a vdso regression on mips,
and I suggested to drop the time32 functions completely from the
vdso when CONFIG_COMPAT_32BIT_TIME is disabled, such as
diff --git a/arch/powerpc/kernel/vdso32/vdso32.lds.S
b/arch/powerpc/kernel/vdso32/vdso32.lds.S
index 00c025ba4a92..605f259fa24c 100644
--- a/arch/powerpc/kernel/vdso32/vdso32.lds.S
+++ b/arch/powerpc/kernel/vdso32/vdso32.lds.S
@@ -145,10 +145,12 @@ VERSION
__kernel_get_syscall_map;
#ifndef CONFIG_PPC_BOOK3S_601
+#ifdef CONFIG_COMPAT_32BIT_TIME
__kernel_gettimeofday;
__kernel_clock_gettime;
__kernel_clock_getres;
__kernel_time;
+#endif
__kernel_get_tbfreq;
#endif
__kernel_sync_dicache;
Any opinions on this? If everyone agrees with that approach, I can
send a cross-architecture patch to do this everywhere. It's probably
best though if Christophe adds that to his series as it touches a lot
of the same files and I would prefer to avoid conflicting changes.
Arnd
Le 02/01/2020 à 12:29, Arnd Bergmann a écrit :
> On Mon, Dec 30, 2019 at 1:27 PM Arnd Bergmann <[email protected]> wrote:
>> On Mon, Dec 23, 2019 at 3:31 PM Christophe Leroy <[email protected]> wrote:
>>> +static __always_inline
>>> +long clock_getres32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
>>> +{
>>> + struct __kernel_timespec ts;
>>> + int ret = clock_getres_fallback(clock, &ts);
>>> +
>>> + if (likely(!ret && _ts)) {
>>> + _ts->tv_sec = ts.tv_sec;
>>> + _ts->tv_nsec = ts.tv_nsec;
>>> + }
>>> + return ret;
>>> +}
>>
>> Please change these to call __NR_clock_gettime and __NR_clock_getres_time
>> instead of __NR_clock_gettime64/__NR_clock_getres_time64 for multiple reasons.
>>
>> - When doing migration between containers, the vdso may get copied into
>> an application running on a kernel that does not support the time64
>> variants, and then the fallback fails.
>>
>> - When CONFIG_COMPAT_32BIT_TIME is disabled, the time32 syscalls
>> return -ENOSYS, and the vdso version should have the exact same behavior
>> to avoid surprises. In particular an application that checks clock_gettime()
>> to see if the time32 are in part of the kernel would get an incorrect result
>> here.
>>
>> arch/arm64/include/asm/vdso/compat_gettimeofday.h already does this,
>> I think you can just copy the implementation or find a way to share it.
>
> There was a related discussion on this after a vdso regression on mips,
> and I suggested to drop the time32 functions completely from the
> vdso when CONFIG_COMPAT_32BIT_TIME is disabled, such as
>
> diff --git a/arch/powerpc/kernel/vdso32/vdso32.lds.S
> b/arch/powerpc/kernel/vdso32/vdso32.lds.S
> index 00c025ba4a92..605f259fa24c 100644
> --- a/arch/powerpc/kernel/vdso32/vdso32.lds.S
> +++ b/arch/powerpc/kernel/vdso32/vdso32.lds.S
> @@ -145,10 +145,12 @@ VERSION
>
> __kernel_get_syscall_map;
> #ifndef CONFIG_PPC_BOOK3S_601
> +#ifdef CONFIG_COMPAT_32BIT_TIME
> __kernel_gettimeofday;
> __kernel_clock_gettime;
> __kernel_clock_getres;
> __kernel_time;
> +#endif
> __kernel_get_tbfreq;
> #endif
> __kernel_sync_dicache;
>
> Any opinions on this? If everyone agrees with that approach, I can
> send a cross-architecture patch to do this everywhere. It's probably
> best though if Christophe adds that to his series as it touches a lot
> of the same files and I would prefer to avoid conflicting changes.
>
I guess it would be wise.
I don't think my series to switch powerpc to C VDSO will get ready
anytime soon, because (in addition to the performance impact) I'm having
hard time with the 32 bits VDSO for PPC64. Many of the powerpc header
files used by lib/vdso/gettimeofday.c are not ready for generating 32
bits code for PPC64. Main problem is that at many places, #ifdef
CONFIG_PPC64 is used instead of #ifdef __powerpc64__. There are also
some CONFIG options like CONFIG_GENERIC_ATOMIC64 that are selected only
when CONFIG_PPC32 is set, but which are required for building the 32
bits VDSO. For that I don't even know how to deal with it.
So, feel free to send your patch, and if my series comes early enough to
conflict, I'll manage it.
Christophe
Andy Lutomirski <[email protected]> writes:
> On Mon, Dec 23, 2019 at 6:31 AM Christophe Leroy
> <[email protected]> wrote:
>>
>> In order to simplify next step which moves fallback call at arch
>> level, ensure all arches have a 32bit fallback instead of handling
>> the lack of 32bit fallback in the common code based
>> on VDSO_HAS_32BIT_FALLBACK
>
> I don't like this. You've implemented what appear to be nonsensical
> fallbacks (the 32-bit fallback for a 64-bit vDSO build? There's no
> such thing).
>
> How exactly does this simplify patch 2?
There is a patchset from Vincenzo which fell through the cracks which
addresses the VDS_HAS_32BIT_FALLBACK issue properly. I'm about to pick
it up. See:
https://lore.kernel.org/lkml/[email protected]/
Thanks,
tglx
> On Jan 10, 2020, at 10:56 AM, Thomas Gleixner <[email protected]> wrote:
>
> Andy Lutomirski <[email protected]> writes:
>
>>> On Mon, Dec 23, 2019 at 6:31 AM Christophe Leroy
>>> <[email protected]> wrote:
>>>
>>> In order to simplify next step which moves fallback call at arch
>>> level, ensure all arches have a 32bit fallback instead of handling
>>> the lack of 32bit fallback in the common code based
>>> on VDSO_HAS_32BIT_FALLBACK
>>
>> I don't like this. You've implemented what appear to be nonsensical
>> fallbacks (the 32-bit fallback for a 64-bit vDSO build? There's no
>> such thing).
>>
>> How exactly does this simplify patch 2?
>
> There is a patchset from Vincenzo which fell through the cracks which
> addresses the VDS_HAS_32BIT_FALLBACK issue properly. I'm about to pick
> it up. See:
>
> https://lore.kernel.org/lkml/[email protected]/
>
Thanks. I had been wondering why the conditionals were still there, since I remember seeing these patches.
> Thanks,
>
> tglx