Re: [RFC PATCH v2 07/10] lib: vdso: don't use READ_ONCE() in __c_kernel_time()

[Christophe Leroy]

Le 11/01/2020 Ã 12:07, Thomas Gleixner a ÃcritÂ:
> Christophe Leroy <christophe.leroy@xxxxxx> 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;
>