| Message ID | fc1ff722c7cbe63a63ae02ade3a714d2049d54a5.1577111367.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: > > READ_ONCE() forces the read of the 64 bit value of > vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec allthough > only the lower part is needed. Seems reasonable and very unlikely to be harmful. That being said, this function really ought to be considered deprecated -- 32-bit time_t is insufficient. Do you get even better code if you move the read into the if statement? Reviewed-by: Andy Lutomirski <luto@kernel.org> --Andy
Le 24/12/2019 à 02:58, Andy Lutomirski a écrit : > On Mon, Dec 23, 2019 at 6:31 AM Christophe Leroy > <christophe.leroy@c-s.fr> wrote: >> >> READ_ONCE() forces the read of the 64 bit value of >> vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec allthough >> only the lower part is needed. > > Seems reasonable and very unlikely to be harmful. That being said, > this function really ought to be considered deprecated -- 32-bit > time_t is insufficient. > > Do you get even better code if you move the read into the if statement? Euh ... How can you return t when time pointer is NULL if you read t only when time pointer is not NULL ? Christophe
> On Dec 24, 2019, at 7:12 PM, christophe leroy <christophe.leroy@c-s.fr> wrote: > > > >> Le 24/12/2019 à 02:58, Andy Lutomirski a écrit : >>> On Mon, Dec 23, 2019 at 6:31 AM Christophe Leroy >>> <christophe.leroy@c-s.fr> wrote: >>> >>> READ_ONCE() forces the read of the 64 bit value of >>> vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec allthough >>> only the lower part is needed. >> Seems reasonable and very unlikely to be harmful. That being said, >> this function really ought to be considered deprecated -- 32-bit >> time_t is insufficient. >> Do you get even better code if you move the read into the if statement? > > Euh ... > > How can you return t when time pointer is NULL if you read t only when time pointer is not NULL ? > > Duh, never mind. But this means your patch may be buggy: you need to make sure the compiler returns the *same* value it stores. Maybe you’re saved by the potential aliasing between the data page and the passed parameter and the value you read, but that’sa bad thing to rely on. Try barrier() after the read.
Christophe Leroy <christophe.leroy@c-s.fr> writes: > > diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c > index 17b4cff6e5f0..5a17a9d2e6cd 100644 > --- a/lib/vdso/gettimeofday.c > +++ b/lib/vdso/gettimeofday.c > @@ -144,7 +144,7 @@ __cvdso_gettimeofday(const struct vdso_data *vd, struct __kernel_old_timeval *tv > static __maybe_unused __kernel_old_time_t > __cvdso_time(const struct vdso_data *vd, __kernel_old_time_t *time) > { > - __kernel_old_time_t t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec); > + __kernel_old_time_t t = vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec; > > if (time) > *time = t; Allows the compiler to load twice, i.e. the returned value might be different from the stored value. So no. Thanks, tglx
On 01/10/2020 09:12 PM, Thomas Gleixner wrote: > Christophe Leroy <christophe.leroy@c-s.fr> writes: >> >> diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c >> index 17b4cff6e5f0..5a17a9d2e6cd 100644 >> --- a/lib/vdso/gettimeofday.c >> +++ b/lib/vdso/gettimeofday.c >> @@ -144,7 +144,7 @@ __cvdso_gettimeofday(const struct vdso_data *vd, struct __kernel_old_timeval *tv >> static __maybe_unused __kernel_old_time_t >> __cvdso_time(const struct vdso_data *vd, __kernel_old_time_t *time) >> { >> - __kernel_old_time_t t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec); >> + __kernel_old_time_t t = vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec; >> >> if (time) >> *time = t; > > Allows the compiler to load twice, i.e. the returned value might be different from the > stored value. So no. > With READ_ONCE() the 64 bits are being read: 00000ac8 <__c_kernel_time>: ac8: 2c 03 00 00 cmpwi r3,0 acc: 81 44 00 20 lwz r10,32(r4) ad0: 81 64 00 24 lwz r11,36(r4) ad4: 41 82 00 08 beq adc <__c_kernel_time+0x14> ad8: 91 63 00 00 stw r11,0(r3) adc: 7d 63 5b 78 mr r3,r11 ae0: 4e 80 00 20 blr Without the READ_ONCE() only 32 bits are read. That's the most optimal. 00000ac8 <__c_kernel_time>: ac8: 7c 69 1b 79 mr. r9,r3 acc: 80 64 00 24 lwz r3,36(r4) ad0: 4d 82 00 20 beqlr ad4: 90 69 00 00 stw r3,0(r9) ad8: 4e 80 00 20 blr Without READ_ONCE() but with a barrier() after the read, we should get the same result but GCC (GCC 8.1) does less good: 00000ac8 <__c_kernel_time>: ac8: 81 24 00 24 lwz r9,36(r4) acc: 2f 83 00 00 cmpwi cr7,r3,0 ad0: 41 9e 00 08 beq cr7,ad8 <__c_kernel_time+0x10> ad4: 91 23 00 00 stw r9,0(r3) ad8: 7d 23 4b 78 mr r3,r9 adc: 4e 80 00 20 blr Assuming both part of the 64 bits data will fall into a single cacheline, the second read is in the noise. So agreed to drop this change. Christophe
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. 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;
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; > >
diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c index 17b4cff6e5f0..5a17a9d2e6cd 100644 --- a/lib/vdso/gettimeofday.c +++ b/lib/vdso/gettimeofday.c @@ -144,7 +144,7 @@ __cvdso_gettimeofday(const struct vdso_data *vd, struct __kernel_old_timeval *tv static __maybe_unused __kernel_old_time_t __cvdso_time(const struct vdso_data *vd, __kernel_old_time_t *time) { - __kernel_old_time_t t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec); + __kernel_old_time_t t = vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec; if (time) *time = t;
READ_ONCE() forces the read of the 64 bit value of vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec allthough only the lower part is needed. This results in a suboptimal code: 00000af4 <__c_kernel_time>: af4: 2c 03 00 00 cmpwi r3,0 af8: 81 44 00 20 lwz r10,32(r4) afc: 81 64 00 24 lwz r11,36(r4) b00: 41 82 00 08 beq b08 <__c_kernel_time+0x14> b04: 91 63 00 00 stw r11,0(r3) b08: 7d 63 5b 78 mr r3,r11 b0c: 4e 80 00 20 blr By removing the READ_ONCE(), only the lower part is read from memory, and the code is cleaner: 00000af4 <__c_kernel_time>: af4: 7c 69 1b 79 mr. r9,r3 af8: 80 64 00 24 lwz r3,36(r4) afc: 4d 82 00 20 beqlr b00: 90 69 00 00 stw r3,0(r9) b04: 4e 80 00 20 blr Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> --- lib/vdso/gettimeofday.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)