Re: [RFC PATCH v2 07/10] lib: vdso: don't use READ_ONCE() in __c_kernel_time()
From: Christophe Leroy <hidden>
Date: 2020-01-13 06:52:36
Also in:
linux-arm-kernel, linux-mips, lkml
Le 11/01/2020 à 12:07, Thomas Gleixner a écrit :
Christophe Leroy [off-list ref] writes:quoted
With READ_ONCE() the 64 bits are being read: Without the READ_ONCE() only 32 bits are read. That's the most optimal. Without READ_ONCE() but with a barrier() after the read, we should get the same result but GCC (GCC 8.1) does less good: Assuming both part of the 64 bits data will fall into a single cacheline, the second read is in the noise.They definitely are in the same cacheline.quoted
So agreed to drop this change.We could be smart about this and force the compiler to issue a 32bit read for 32bit builds. See below. Not sure whether it's worth it, but OTOH it will take quite a while until the 32bit time interfaces die completely.
I don't think it is worth something so big to just save 1 or 2 cycles in time() function. Lets keep it as it is. Thanks, Christophe
quoted hunk ↗ jump to hunk
Thanks, tglx 8<--------------- a/include/vdso/datapage.h +++ b/include/vdso/datapage.h@@ -21,6 +21,18 @@ #define CS_RAW 1 #define CS_BASES (CS_RAW + 1) +#ifdef __LITTLE_ENDIAN +struct sec_hl { + u32 sec_l; + u32 sec_h; +}; +#else +struct sec_hl { + u32 sec_h; + u32 sec_l; +}; +#endif + /** * struct vdso_timestamp - basetime per clock_id * @sec: seconds@@ -35,7 +47,10 @@ * vdso_data.cs[x].shift. */ struct vdso_timestamp { - u64 sec; + union { + u64 sec; + struct sec_hl sec_hl; + }; u64 nsec; }; --- a/lib/vdso/gettimeofday.c +++ b/lib/vdso/gettimeofday.c@@ -165,8 +165,13 @@ static __maybe_unused int static __maybe_unused __kernel_old_time_t __cvdso_time(__kernel_old_time_t *time) { const struct vdso_data *vd = __arch_get_vdso_data(); - __kernel_old_time_t t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec); + __kernel_old_time_t t; +#if BITS_PER_LONG == 32 + t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec_hl.sec_l); +#else + t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec); +#endif if (time) *time = t;