Messages in this thread | | | Subject | Re: [RFC PATCH v2 07/10] lib: vdso: don't use READ_ONCE() in __c_kernel_time() | From | Christophe Leroy <> | Date | Mon, 13 Jan 2020 07:52:31 +0100 |
| |
Le 11/01/2020 à 12:07, Thomas Gleixner a écrit : > Christophe Leroy <christophe.leroy@c-s.fr> writes: >> >> 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. > >> 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
> > 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; > >
| |