| Message ID | 47701b5fb73cf536db074031db8e6e3fa3695168.1577111365.git.christophe.leroy@c-s.fr (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Paul Burton |
| Headers | show |
| Series | powerpc/32: switch VDSO to C implementation. | expand |
On Mon, Dec 23, 2019 at 6:31 AM Christophe Leroy <christophe.leroy@c-s.fr> 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 <christophe.leroy@c-s.fr> 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 <christophe.leroy@c-s.fr> 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 <arnd@arndb.de> wrote: > On Mon, Dec 23, 2019 at 3:31 PM Christophe Leroy <christophe.leroy@c-s.fr> 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 <arnd@arndb.de> wrote: >> On Mon, Dec 23, 2019 at 3:31 PM Christophe Leroy <christophe.leroy@c-s.fr> 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 <luto@kernel.org> writes: > On Mon, Dec 23, 2019 at 6:31 AM Christophe Leroy > <christophe.leroy@c-s.fr> 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/20190830135902.20861-1-vincenzo.frascino@arm.com/ Thanks, tglx
> On Jan 10, 2020, at 10:56 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > > Andy Lutomirski <luto@kernel.org> writes: > >>> On Mon, Dec 23, 2019 at 6:31 AM Christophe Leroy >>> <christophe.leroy@c-s.fr> 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/20190830135902.20861-1-vincenzo.frascino@arm.com/ > Thanks. I had been wondering why the conditionals were still there, since I remember seeing these patches. > Thanks, > > tglx
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;
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 <christophe.leroy@c-s.fr> --- 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(-)