| Message ID | c1a8493b-860f-8a24-53c9-80eca6c4c916@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [rs6000] Allow libitm to use HTM on newer hw and kernels | expand |
On Wed, Dec 12, 2018 at 01:47:02PM -0600, Peter Bergner wrote: > Libitm on POWER hardware looks for the "htm" bit in AT_HWCAP2 to determine > whether it can use HTM when executing code within __transaction_atomic > code blocks. However, on newer hardware and kernels, the "htm" bit is no > longer set and instead the "htm-no-suspend" bit is set, so we currently > don't use HTM on new hw and kernels. The following patch adds support > for htm-no-suspend to libitm. I have also added code to use the > __builtin_cpu_supports() builtin if it is available, since that is > much faster than using the getauxval libc call. > > This passed bootstrap and regtesting with no errors and someone within > IBM how had a POWER9 box with a newish kernel how ran into the problem > confirmed it works for his test case. > > Ok for mainline? Should be backport this? Okay for trunk (but see comment below). I think it should be backported yes, to 8 at least, probably 7. Okay for those too. > +/* This is a fairly new feature bit, so handle it not being defined. */ > +#ifndef PPC_FEATURE2_HTM_NO_SUSPEND > +# define PPC_FEATURE2_HTM_NO_SUSPEND 0 > +#endif Doing it this way can be pretty surprising for users not aware you defined it to 0. Since there are no other users yet it's no big deal. Segher
On 12/12/18 2:52 PM, Segher Boessenkool wrote: >> +/* This is a fairly new feature bit, so handle it not being defined. */ >> +#ifndef PPC_FEATURE2_HTM_NO_SUSPEND >> +# define PPC_FEATURE2_HTM_NO_SUSPEND 0 >> +#endif > > Doing it this way can be pretty surprising for users not aware you defined > it to 0. Since there are no other users yet it's no big deal. Would you instead prefer something along the lines of the following? Peter static inline bool htm_available (void) { - return (getauxval (AT_HWCAP2) & PPC_FEATURE2_HAS_HTM) ? true : false; +#ifdef __BUILTIN_CPU_SUPPORTS__ + if (__builtin_cpu_supports ("htm-no-suspend") + || __builtin_cpu_supports ("htm")) + return true; +#else +#ifdef PPC_FEATURE2_HTM_NO_SUSPEND + unsigned long htm_flags = PPC_FEATURE2_HAS_HTM | PPC_FEATURE2_HTM_NO_SUSPEND; +#else + unsigned long htm_flags = PPC_FEATURE2_HAS_HTM; +#endif + if (getauxval (AT_HWCAP2) & htm_flags) + return true; +#endif + return false; }
On Wed, Dec 12, 2018 at 06:55:14PM -0600, Peter Bergner wrote: > On 12/12/18 2:52 PM, Segher Boessenkool wrote: > >> +/* This is a fairly new feature bit, so handle it not being defined. */ > >> +#ifndef PPC_FEATURE2_HTM_NO_SUSPEND > >> +# define PPC_FEATURE2_HTM_NO_SUSPEND 0 > >> +#endif > > > > Doing it this way can be pretty surprising for users not aware you defined > > it to 0. Since there are no other users yet it's no big deal. > > Would you instead prefer something along the lines of the following? Yeah that looks a lot better :-) > static inline bool > htm_available (void) > { > - return (getauxval (AT_HWCAP2) & PPC_FEATURE2_HAS_HTM) ? true : false; > +#ifdef __BUILTIN_CPU_SUPPORTS__ > + if (__builtin_cpu_supports ("htm-no-suspend") > + || __builtin_cpu_supports ("htm")) > + return true; > +#else > +#ifdef PPC_FEATURE2_HTM_NO_SUSPEND > + unsigned long htm_flags = PPC_FEATURE2_HAS_HTM | PPC_FEATURE2_HTM_NO_SUSPEND; > +#else > + unsigned long htm_flags = PPC_FEATURE2_HAS_HTM; > +#endif > + if (getauxval (AT_HWCAP2) & htm_flags) > + return true; > +#endif > + return false; > } Or like unsigned long htm_flags = PPC_FEATURE2_HAS_HTM #ifdef PPC_FEATURE2_HTM_NO_SUSPEND | PPC_FEATURE2_HTM_NO_SUSPEND #endif | 0; Okay for trunk with either style. Thanks! Segher
On 12/13/18 2:41 AM, Segher Boessenkool wrote: > Or like > > unsigned long htm_flags = PPC_FEATURE2_HAS_HTM > #ifdef PPC_FEATURE2_HTM_NO_SUSPEND > | PPC_FEATURE2_HTM_NO_SUSPEND > #endif > | 0; > > Okay for trunk with either style. Thanks! Ok, I'll go with this and commit once I've done another round of testing. I'm assuming your backport approval still stands for this code too. Peter
On 12/13/18 10:42 AM, Peter Bergner wrote: > On 12/13/18 2:41 AM, Segher Boessenkool wrote: >> Or like >> >> unsigned long htm_flags = PPC_FEATURE2_HAS_HTM >> #ifdef PPC_FEATURE2_HTM_NO_SUSPEND >> | PPC_FEATURE2_HTM_NO_SUSPEND >> #endif >> | 0; >> >> Okay for trunk with either style. Thanks! > > Ok, I'll go with this and commit once I've done another round of > testing. I'm assuming your backport approval still stands for this > code too. Testing was clean everywhere. Committed everywhere. Thanks! Peter
Index: libitm/config/powerpc/target.h =================================================================== --- libitm/config/powerpc/target.h (revision 267062) +++ libitm/config/powerpc/target.h (working copy) @@ -26,6 +26,11 @@ #include <sys/auxv.h> #endif +/* This is a fairly new feature bit, so handle it not being defined. */ +#ifndef PPC_FEATURE2_HTM_NO_SUSPEND +# define PPC_FEATURE2_HTM_NO_SUSPEND 0 +#endif + namespace GTM HIDDEN { typedef int v128 __attribute__((vector_size(16), may_alias, aligned(16))); @@ -81,7 +86,16 @@ cpu_relax (void) static inline bool htm_available (void) { - return (getauxval (AT_HWCAP2) & PPC_FEATURE2_HAS_HTM) ? true : false; +#ifdef __BUILTIN_CPU_SUPPORTS__ + if (__builtin_cpu_supports ("htm-no-suspend") + || __builtin_cpu_supports ("htm")) + return true; +#else + if (getauxval (AT_HWCAP2) + & (PPC_FEATURE2_HAS_HTM | PPC_FEATURE2_HTM_NO_SUSPEND)) + return true; +#endif + return false; } static inline uint32_t