| Message ID | 20170127033143.43107-1-brian.brooks@linaro.org |
|---|---|
| State | Accepted |
| Commit | 04b0be7579f77821418c0c7c524950af6aeab6a6 |
| Headers | show |
On Thu, Jan 26, 2017 at 9:31 PM, Brian Brooks <brian.brooks@linaro.org> wrote: > Signed-off-by: Brian Brooks <brian.brooks@linaro.org> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/arch/arm/odp/api/cpu_arch.h | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > index 22b1da2d..7c75a690 100644 > --- a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > +++ b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > @@ -15,6 +15,12 @@ extern "C" { > > static inline void odp_cpu_pause(void) > { > + /* YIELD hints the CPU to switch to another thread if possible > + * and executes as a NOP otherwise. > + * ISB flushes the pipeline, then restarts. This is guaranteed to > + * stall the CPU a number of cycles. > + */ > + __asm volatile("isb" ::: "memory"); > } > > #ifdef __cplusplus > -- > 2.11.0 >
it's abi function, has to be declared with _STATIC. For others arches also. Maxim. On 1 February 2017 at 01:34, Bill Fischofer <bill.fischofer@linaro.org> wrote: > On Thu, Jan 26, 2017 at 9:31 PM, Brian Brooks <brian.brooks@linaro.org> > wrote: > > Signed-off-by: Brian Brooks <brian.brooks@linaro.org> > > Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org> > > > --- > > platform/linux-generic/arch/arm/odp/api/cpu_arch.h | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > > index 22b1da2d..7c75a690 100644 > > --- a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > > +++ b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > > @@ -15,6 +15,12 @@ extern "C" { > > > > static inline void odp_cpu_pause(void) > > { > > + /* YIELD hints the CPU to switch to another thread if possible > > + * and executes as a NOP otherwise. > > + * ISB flushes the pipeline, then restarts. This is guaranteed to > > + * stall the CPU a number of cycles. > > + */ > > + __asm volatile("isb" ::: "memory"); > > } > > > > #ifdef __cplusplus > > -- > > 2.11.0 > > >
On Wed, Feb 1, 2017 at 2:17 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > it's abi function, has to be declared with _STATIC. For others arches also. Actually, in this case since the purpose of the call is to waste time, inlining it would be counterproductive, which is why these routines don't follow those ABI rules. :) > > Maxim. > > On 1 February 2017 at 01:34, Bill Fischofer <bill.fischofer@linaro.org> > wrote: >> >> On Thu, Jan 26, 2017 at 9:31 PM, Brian Brooks <brian.brooks@linaro.org> >> wrote: >> > Signed-off-by: Brian Brooks <brian.brooks@linaro.org> >> >> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org> >> >> > --- >> > platform/linux-generic/arch/arm/odp/api/cpu_arch.h | 6 ++++++ >> > 1 file changed, 6 insertions(+) >> > >> > diff --git a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h >> > b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h >> > index 22b1da2d..7c75a690 100644 >> > --- a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h >> > +++ b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h >> > @@ -15,6 +15,12 @@ extern "C" { >> > >> > static inline void odp_cpu_pause(void) >> > { >> > + /* YIELD hints the CPU to switch to another thread if possible >> > + * and executes as a NOP otherwise. >> > + * ISB flushes the pipeline, then restarts. This is guaranteed >> > to >> > + * stall the CPU a number of cycles. >> > + */ >> > + __asm volatile("isb" ::: "memory"); >> > } >> > >> > #ifdef __cplusplus >> > -- >> > 2.11.0 >> > > >
If this is in the API, then to be ABI compatible it must compile into the lib and not be a static inline I would think, unless we are sure that for a given cpu arch it will never differ which may be true. On 1 February 2017 at 06:12, Bill Fischofer <bill.fischofer@linaro.org> wrote: > On Wed, Feb 1, 2017 at 2:17 AM, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > > it's abi function, has to be declared with _STATIC. For others arches > also. > > Actually, in this case since the purpose of the call is to waste time, > inlining it would be counterproductive, which is why these routines > don't follow those ABI rules. :) > > > > > Maxim. > > > > On 1 February 2017 at 01:34, Bill Fischofer <bill.fischofer@linaro.org> > > wrote: > >> > >> On Thu, Jan 26, 2017 at 9:31 PM, Brian Brooks <brian.brooks@linaro.org> > >> wrote: > >> > Signed-off-by: Brian Brooks <brian.brooks@linaro.org> > >> > >> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org> > >> > >> > --- > >> > platform/linux-generic/arch/arm/odp/api/cpu_arch.h | 6 ++++++ > >> > 1 file changed, 6 insertions(+) > >> > > >> > diff --git a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > >> > b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > >> > index 22b1da2d..7c75a690 100644 > >> > --- a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > >> > +++ b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > >> > @@ -15,6 +15,12 @@ extern "C" { > >> > > >> > static inline void odp_cpu_pause(void) > >> > { > >> > + /* YIELD hints the CPU to switch to another thread if possible > >> > + * and executes as a NOP otherwise. > >> > + * ISB flushes the pipeline, then restarts. This is guaranteed > >> > to > >> > + * stall the CPU a number of cycles. > >> > + */ > >> > + __asm volatile("isb" ::: "memory"); > >> > } > >> > > >> > #ifdef __cplusplus > >> > -- > >> > 2.11.0 > >> > > > > > > -- Mike Holmes Program Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
On 02/01 08:38:08, Mike Holmes wrote: > If this is in the API, then to be ABI compatible it must compile into the > lib and not be a static inline I would think, unless we are sure that for a > given cpu arch it will never differ which may be true. > > > On 1 February 2017 at 06:12, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > > On Wed, Feb 1, 2017 at 2:17 AM, Maxim Uvarov <maxim.uvarov@linaro.org> > > wrote: > > > it's abi function, has to be declared with _STATIC. For others arches > > also. The use of 'static' here specifies that the function name is not exported to the linker--it is not visible outside of the translation unit. 'inline' suggests to the compiler that it should copy the contents of the function into the respective callsites of said function instead of generating function calls and an entry for the function itself. Since no function call will be generated, there is no ABI. Unless, the function is modified to use ODP's '_STATIC' mechanism to control at build time whether the function is 'static inline' or a real function call. Such a change is undesirable for a function that emits a single instruction. In the case of this patch, an 'isb' barrier can be used to slow the core down, e.g. if it is spinning on a memory location to be updated. Slowing the core down in such case helps reduce the contention and thus overhead required to maintain coherency between the cores. Can we accept this patch? > > Actually, in this case since the purpose of the call is to waste time, > > inlining it would be counterproductive, which is why these routines > > don't follow those ABI rules. :) > > > > > > > > Maxim. > > > > > > On 1 February 2017 at 01:34, Bill Fischofer <bill.fischofer@linaro.org> > > > wrote: > > >> > > >> On Thu, Jan 26, 2017 at 9:31 PM, Brian Brooks <brian.brooks@linaro.org> > > >> wrote: > > >> > Signed-off-by: Brian Brooks <brian.brooks@linaro.org> > > >> > > >> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org> > > >> > > >> > --- > > >> > platform/linux-generic/arch/arm/odp/api/cpu_arch.h | 6 ++++++ > > >> > 1 file changed, 6 insertions(+) > > >> > > > >> > diff --git a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > > >> > b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > > >> > index 22b1da2d..7c75a690 100644 > > >> > --- a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > > >> > +++ b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h > > >> > @@ -15,6 +15,12 @@ extern "C" { > > >> > > > >> > static inline void odp_cpu_pause(void) > > >> > { > > >> > + /* YIELD hints the CPU to switch to another thread if possible > > >> > + * and executes as a NOP otherwise. > > >> > + * ISB flushes the pipeline, then restarts. This is guaranteed > > >> > to > > >> > + * stall the CPU a number of cycles. > > >> > + */ > > >> > + __asm volatile("isb" ::: "memory"); > > >> > } > > >> > > > >> > #ifdef __cplusplus > > >> > -- > > >> > 2.11.0 > > >> > > > > > > > > > > > > > -- > Mike Holmes > Program Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows"
I merged this patch because it does not introduce problem and the same static inlines in all others arches. Separate patch will be needed if we decide to un inline all of them. Maxim. On 02/01/17 21:41, Brian Brooks wrote: > On 02/01 08:38:08, Mike Holmes wrote: >> If this is in the API, then to be ABI compatible it must compile into the >> lib and not be a static inline I would think, unless we are sure that for a >> given cpu arch it will never differ which may be true. >> >> >> On 1 February 2017 at 06:12, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> >>> On Wed, Feb 1, 2017 at 2:17 AM, Maxim Uvarov <maxim.uvarov@linaro.org> >>> wrote: >>>> it's abi function, has to be declared with _STATIC. For others arches >>> also. > > The use of 'static' here specifies that the function name is not exported > to the linker--it is not visible outside of the translation unit. > > 'inline' suggests to the compiler that it should copy the contents of the > function into the respective callsites of said function instead of generating > function calls and an entry for the function itself. > > Since no function call will be generated, there is no ABI. Unless, the > function is modified to use ODP's '_STATIC' mechanism to control at build > time whether the function is 'static inline' or a real function call. Such > a change is undesirable for a function that emits a single instruction. > > In the case of this patch, an 'isb' barrier can be used to slow the core > down, e.g. if it is spinning on a memory location to be updated. Slowing > the core down in such case helps reduce the contention and thus overhead > required to maintain coherency between the cores. Can we accept this patch? > >>> Actually, in this case since the purpose of the call is to waste time, >>> inlining it would be counterproductive, which is why these routines >>> don't follow those ABI rules. :) >>> >>>> >>>> Maxim. >>>> >>>> On 1 February 2017 at 01:34, Bill Fischofer <bill.fischofer@linaro.org> >>>> wrote: >>>>> >>>>> On Thu, Jan 26, 2017 at 9:31 PM, Brian Brooks <brian.brooks@linaro.org> >>>>> wrote: >>>>>> Signed-off-by: Brian Brooks <brian.brooks@linaro.org> >>>>> >>>>> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org> >>>>> >>>>>> --- >>>>>> platform/linux-generic/arch/arm/odp/api/cpu_arch.h | 6 ++++++ >>>>>> 1 file changed, 6 insertions(+) >>>>>> >>>>>> diff --git a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h >>>>>> b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h >>>>>> index 22b1da2d..7c75a690 100644 >>>>>> --- a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h >>>>>> +++ b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h >>>>>> @@ -15,6 +15,12 @@ extern "C" { >>>>>> >>>>>> static inline void odp_cpu_pause(void) >>>>>> { >>>>>> + /* YIELD hints the CPU to switch to another thread if possible >>>>>> + * and executes as a NOP otherwise. >>>>>> + * ISB flushes the pipeline, then restarts. This is guaranteed >>>>>> to >>>>>> + * stall the CPU a number of cycles. >>>>>> + */ >>>>>> + __asm volatile("isb" ::: "memory"); >>>>>> } >>>>>> >>>>>> #ifdef __cplusplus >>>>>> -- >>>>>> 2.11.0 >>>>>> >>>> >>>> >>> >> >> >> >> -- >> Mike Holmes >> Program Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs >> "Work should be fun and collaborative, the rest follows"
diff --git a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h index 22b1da2d..7c75a690 100644 --- a/platform/linux-generic/arch/arm/odp/api/cpu_arch.h +++ b/platform/linux-generic/arch/arm/odp/api/cpu_arch.h @@ -15,6 +15,12 @@ extern "C" { static inline void odp_cpu_pause(void) { + /* YIELD hints the CPU to switch to another thread if possible + * and executes as a NOP otherwise. + * ISB flushes the pipeline, then restarts. This is guaranteed to + * stall the CPU a number of cycles. + */ + __asm volatile("isb" ::: "memory"); } #ifdef __cplusplus
Signed-off-by: Brian Brooks <brian.brooks@linaro.org> --- platform/linux-generic/arch/arm/odp/api/cpu_arch.h | 6 ++++++ 1 file changed, 6 insertions(+) -- 2.11.0