On Fri, Feb 22, 2019 at 12:24:09PM +0000, Vincenzo Frascino wrote: > In the last few years we assisted to an explosion of vdso > implementations that mostly share similar code. > > Try to unify the gettimeofday vdso implementation introducing > lib/vdso. The code contained in this library can ideally be > reused by all the architectures avoiding, where possible, code > duplication. > > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@xxxxxxx> > --- > include/vdso/datapage.h | 1 + > include/vdso/helpers.h | 52 ++++++++++++ > include/vdso/types.h | 39 +++++++++ > lib/Kconfig | 5 ++ > lib/vdso/Kconfig | 37 +++++++++ > lib/vdso/Makefile | 22 +++++ > lib/vdso/gettimeofday.c | 175 ++++++++++++++++++++++++++++++++++++++++ > 7 files changed, 331 insertions(+) > create mode 100644 include/vdso/helpers.h > create mode 100644 include/vdso/types.h > create mode 100644 lib/vdso/Kconfig > create mode 100644 lib/vdso/Makefile > create mode 100644 lib/vdso/gettimeofday.c > > diff --git a/include/vdso/datapage.h b/include/vdso/datapage.h > index da346ad02b03..ff332fcba73c 100644 > --- a/include/vdso/datapage.h > +++ b/include/vdso/datapage.h > @@ -9,6 +9,7 @@ > #include <linux/bits.h> > #include <linux/types.h> > #include <linux/time.h> > +#include <vdso/types.h> > > #define VDSO_BASES (CLOCK_TAI + 1) > #define VDSO_HRES (BIT(CLOCK_REALTIME) | \ > diff --git a/include/vdso/helpers.h b/include/vdso/helpers.h > new file mode 100644 > index 000000000000..511dea979f6b > --- /dev/null > +++ b/include/vdso/helpers.h > @@ -0,0 +1,52 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __VDSO_HELPERS_H > +#define __VDSO_HELPERS_H > + > +#ifdef __KERNEL__ Nit: __KERNEL__ guards can go. > + > +#ifndef __ASSEMBLY__ > + > +#include <vdso/datapage.h> > + > +static __always_inline notrace u32 vdso_read_begin(const struct vdso_data *vd) Rather than explicitly annotating all functions with notrace, can't we disable instrumentation for all C files used for the vDSO using compiler flags? That would be more robust, and make the code less noisy to read. > +{ > + u32 seq; > + > +repeat: > + seq = READ_ONCE(vd->seq); > + if (seq & 1) { > + cpu_relax(); > + goto repeat; > + } > + > + smp_rmb(); > + return seq; > +} You could simplify the repeat loop as: while ((seq = READ_ONCE(vd->seq)) & 1) cpu_relax(); > + > +static __always_inline notrace u32 vdso_read_retry(const struct vdso_data *vd, > + u32 start) > +{ > + u32 seq; > + > + smp_rmb(); > + seq = READ_ONCE(vd->seq); > + return seq != start; > +} > + > +static __always_inline notrace void vdso_write_begin(struct vdso_data *vd) > +{ > + ++vd->seq; > + smp_wmb(); > +} > + > +static __always_inline notrace void vdso_write_end(struct vdso_data *vd) > +{ > + smp_wmb(); > + ++vd->seq; > +} I realise this is what existing vDSO update code does, but I do think that these should be using WRITE_ONCE() to perform the update, to ensure that the write is not torn. e.g. these should be: static __always_inline notrace void vdso_write_begin(struct vdso_data *vd) { WRITE_ONCE(vd->seq, vd->seq + 1); smp_wmb(); } static __always_inline notrace void vdso_write_end(struct vdso_data *vd) { smp_wmb(); WRITE_ONCE(vd->seq, vd->seq + 1); } Otherwise, the compiler can validly tear updates to vd->seq, and there's the possibility that a reader sees an inconsistent value for vd->seq, and consequently uses inconsistent data read from the vdso data page. [...] > +#include <linux/types.h> > +#include <linux/time.h> Nit: please order headers alphabetically> > + > +/* > + * The definitions below are required to overcome the limitations > + * of time_t on 32 bit architectures, which overflows in 2038. > + * The new code should use the replacements based on time64_t and > + * timespec64. > + * > + * The abstraction below will be updated once the migration to > + * time64_t is complete. > + */ > +#ifdef CONFIG_GENERIC_VDSO_32 > +#define __vdso_timespec old_timespec32 > +#define __vdso_timeval old_timeval32 > +#else > +#ifdef ENABLE_COMPAT_VDSO > +#define __vdso_timespec old_timespec32 > +#define __vdso_timeval old_timeval32 > +#else > +#define __vdso_timespec __kernel_timespec > +#define __vdso_timeval __kernel_old_timeval > +#endif /* CONFIG_COMPAT_VDSO */ > +#endif /* CONFIG_GENERIC_VDSO_32 */ I'm not sure what the comment is trying to say. For a 64-bit kernel, does this affec the native vDSO, or just the compat vDSO? [...] > +config HAVE_GENERIC_VDSO > + bool > + default n IIRC, 'n' is the implicit default. Thanks, Mark.