* [PATCH 2/6] powerpc: Add enable_ppr kernel parameter to enable PPR save/restore
@ 2012-09-09 11:37 Haren Myneni
2012-09-10 0:12 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 6+ messages in thread
From: Haren Myneni @ 2012-09-09 11:37 UTC (permalink / raw)
To: benh, paulus, anton, mikey; +Cc: linuxppc-dev
enable_ppr kernel parameter is used to enable PPR save and restore.
Supported on Power7 and later processors.
By default, CPU_FTR_HAS_PPR is set for POWER7. If this parameter is not
passed, disable CPU_FTR_HAS_PPR.
Signed-off-by: Haren Myneni <haren@us.ibm.com>
---
Documentation/kernel-parameters.txt | 4 ++++
arch/powerpc/include/asm/cputable.h | 6 ++++--
arch/powerpc/kernel/setup_64.c | 14 ++++++++++++++
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index ad7e2e5..2881e5f 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -809,6 +809,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
to discrete, to make X server driver able to add WB
entry later. This parameter enables that.
+ enable_ppr [PPC/PSERIES]
+ Saves user defined PPR when process enters to kernel
+ and restores PPR at exit. But it impacts performance.
+
enable_timer_pin_1 [X86]
Enable PIN 1 of APIC timer
Can be useful to work around chipset bugs
diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
index b3c083d..880c469 100644
--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h
@@ -203,6 +203,7 @@ extern const char *powerpc_base_platform;
#define CPU_FTR_POPCNTD LONG_ASM_CONST(0x0800000000000000)
#define CPU_FTR_ICSWX LONG_ASM_CONST(0x1000000000000000)
#define CPU_FTR_VMX_COPY LONG_ASM_CONST(0x2000000000000000)
+#define CPU_FTR_HAS_PPR LONG_ASM_CONST(0x4000000000000000)
#ifndef __ASSEMBLY__
@@ -432,7 +433,8 @@ extern const char *powerpc_base_platform;
CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
CPU_FTR_DSCR | CPU_FTR_SAO | CPU_FTR_ASYM_SMT | \
CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
- CPU_FTR_ICSWX | CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY)
+ CPU_FTR_ICSWX | CPU_FTR_CFAR | CPU_FTR_HVMODE | \
+ CPU_FTR_VMX_COPY | CPU_FTR_HAS_PPR)
#define CPU_FTRS_CELL (CPU_FTR_USE_TB | CPU_FTR_LWSYNC | \
CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \
CPU_FTR_ALTIVEC_COMP | CPU_FTR_MMCRA | CPU_FTR_SMT | \
@@ -454,7 +456,7 @@ extern const char *powerpc_base_platform;
(CPU_FTRS_POWER3 | CPU_FTRS_RS64 | CPU_FTRS_POWER4 | \
CPU_FTRS_PPC970 | CPU_FTRS_POWER5 | CPU_FTRS_POWER6 | \
CPU_FTRS_POWER7 | CPU_FTRS_CELL | CPU_FTRS_PA6T | \
- CPU_FTR_VSX)
+ CPU_FTR_VSX | CPU_FTR_HAS_PPR)
#endif
#else
enum {
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 389bd4f..e4c1945 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -98,6 +98,8 @@ int dcache_bsize;
int icache_bsize;
int ucache_bsize;
+static u32 enable_ppr = 0;
+
#ifdef CONFIG_SMP
static char *smt_enabled_cmdline;
@@ -357,6 +359,9 @@ void __init setup_system(void)
{
DBG(" -> setup_system()\n");
+ if (cpu_has_feature(CPU_FTR_HAS_PPR) && !enable_ppr)
+ cur_cpu_spec->cpu_features &= ~CPU_FTR_HAS_PPR;
+
/* Apply the CPUs-specific and firmware specific fixups to kernel
* text (nop out sections not relevant to this CPU or this firmware)
*/
@@ -683,6 +688,15 @@ void __init setup_per_cpu_areas(void)
}
#endif
+/* early_ppr kernel parameter to save/restore PPR register */
+static int __init early_ppr_enabled(char *str)
+{
+ enable_ppr = 1;
+
+ return 0;
+}
+
+early_param("enable_ppr", early_ppr_enabled);
#ifdef CONFIG_PPC_INDIRECT_IO
struct ppc_pci_io ppc_pci_io;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/6] powerpc: Add enable_ppr kernel parameter to enable PPR save/restore
2012-09-09 11:37 [PATCH 2/6] powerpc: Add enable_ppr kernel parameter to enable PPR save/restore Haren Myneni
@ 2012-09-10 0:12 ` Benjamin Herrenschmidt
2012-09-10 0:22 ` Michael Neuling
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2012-09-10 0:12 UTC (permalink / raw)
To: Haren Myneni; +Cc: linuxppc-dev, mikey, paulus, anton
On Sun, 2012-09-09 at 04:37 -0700, Haren Myneni wrote:
> enable_ppr kernel parameter is used to enable PPR save and restore.
> Supported on Power7 and later processors.
>
> By default, CPU_FTR_HAS_PPR is set for POWER7. If this parameter is not
> passed, disable CPU_FTR_HAS_PPR.
What is the point ? Obscure / magic kernel command line options to turn
on a feature are pointless. Nobody knows about them, nobody enables
them.
What you are doing is guarantee that nobody's ever going to enable your
code, so your whole patch series is thus irrelevant :-)
If there's a good reason to *avoid* your option, then maybe consider
adding an option to *disable* the PPR save/restore, though of course
that would bring the argument that if it needs to be disabled maybe we
shouldn't do it in the first place, or you might want to think of a
reasonable way to intuit what the option should be.
Ben.
> Signed-off-by: Haren Myneni <haren@us.ibm.com>
>
> ---
> Documentation/kernel-parameters.txt | 4 ++++
> arch/powerpc/include/asm/cputable.h | 6 ++++--
> arch/powerpc/kernel/setup_64.c | 14 ++++++++++++++
> 3 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index ad7e2e5..2881e5f 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -809,6 +809,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> to discrete, to make X server driver able to add WB
> entry later. This parameter enables that.
>
> + enable_ppr [PPC/PSERIES]
> + Saves user defined PPR when process enters to kernel
> + and restores PPR at exit. But it impacts performance.
> +
> enable_timer_pin_1 [X86]
> Enable PIN 1 of APIC timer
> Can be useful to work around chipset bugs
> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
> index b3c083d..880c469 100644
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -203,6 +203,7 @@ extern const char *powerpc_base_platform;
> #define CPU_FTR_POPCNTD LONG_ASM_CONST(0x0800000000000000)
> #define CPU_FTR_ICSWX LONG_ASM_CONST(0x1000000000000000)
> #define CPU_FTR_VMX_COPY LONG_ASM_CONST(0x2000000000000000)
> +#define CPU_FTR_HAS_PPR LONG_ASM_CONST(0x4000000000000000)
>
> #ifndef __ASSEMBLY__
>
> @@ -432,7 +433,8 @@ extern const char *powerpc_base_platform;
> CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
> CPU_FTR_DSCR | CPU_FTR_SAO | CPU_FTR_ASYM_SMT | \
> CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
> - CPU_FTR_ICSWX | CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY)
> + CPU_FTR_ICSWX | CPU_FTR_CFAR | CPU_FTR_HVMODE | \
> + CPU_FTR_VMX_COPY | CPU_FTR_HAS_PPR)
> #define CPU_FTRS_CELL (CPU_FTR_USE_TB | CPU_FTR_LWSYNC | \
> CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \
> CPU_FTR_ALTIVEC_COMP | CPU_FTR_MMCRA | CPU_FTR_SMT | \
> @@ -454,7 +456,7 @@ extern const char *powerpc_base_platform;
> (CPU_FTRS_POWER3 | CPU_FTRS_RS64 | CPU_FTRS_POWER4 | \
> CPU_FTRS_PPC970 | CPU_FTRS_POWER5 | CPU_FTRS_POWER6 | \
> CPU_FTRS_POWER7 | CPU_FTRS_CELL | CPU_FTRS_PA6T | \
> - CPU_FTR_VSX)
> + CPU_FTR_VSX | CPU_FTR_HAS_PPR)
> #endif
> #else
> enum {
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index 389bd4f..e4c1945 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -98,6 +98,8 @@ int dcache_bsize;
> int icache_bsize;
> int ucache_bsize;
>
> +static u32 enable_ppr = 0;
> +
> #ifdef CONFIG_SMP
>
> static char *smt_enabled_cmdline;
> @@ -357,6 +359,9 @@ void __init setup_system(void)
> {
> DBG(" -> setup_system()\n");
>
> + if (cpu_has_feature(CPU_FTR_HAS_PPR) && !enable_ppr)
> + cur_cpu_spec->cpu_features &= ~CPU_FTR_HAS_PPR;
> +
> /* Apply the CPUs-specific and firmware specific fixups to kernel
> * text (nop out sections not relevant to this CPU or this firmware)
> */
> @@ -683,6 +688,15 @@ void __init setup_per_cpu_areas(void)
> }
> #endif
>
> +/* early_ppr kernel parameter to save/restore PPR register */
> +static int __init early_ppr_enabled(char *str)
> +{
> + enable_ppr = 1;
> +
> + return 0;
> +}
> +
> +early_param("enable_ppr", early_ppr_enabled);
>
> #ifdef CONFIG_PPC_INDIRECT_IO
> struct ppc_pci_io ppc_pci_io;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/6] powerpc: Add enable_ppr kernel parameter to enable PPR save/restore
2012-09-10 0:12 ` Benjamin Herrenschmidt
@ 2012-09-10 0:22 ` Michael Neuling
2012-09-11 5:42 ` Haren Myneni
0 siblings, 1 reply; 6+ messages in thread
From: Michael Neuling @ 2012-09-10 0:22 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, paulus, anton, Haren Myneni
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Sun, 2012-09-09 at 04:37 -0700, Haren Myneni wrote:
> > enable_ppr kernel parameter is used to enable PPR save and restore.
> > Supported on Power7 and later processors.
> >
> > By default, CPU_FTR_HAS_PPR is set for POWER7. If this parameter is not
> > passed, disable CPU_FTR_HAS_PPR.
>
> What is the point ? Obscure / magic kernel command line options to turn
> on a feature are pointless. Nobody knows about them, nobody enables
> them.
>
> What you are doing is guarantee that nobody's ever going to enable your
> code, so your whole patch series is thus irrelevant :-)
>
> If there's a good reason to *avoid* your option, then maybe consider
> adding an option to *disable* the PPR save/restore, though of course
> that would bring the argument that if it needs to be disabled maybe we
> shouldn't do it in the first place, or you might want to think of a
> reasonable way to intuit what the option should be.
IIRC, Haren was saying there's a 6% hit on null syscall for this. So we
suggested having a cmdline option to disable it for distros.
Haren, is my recollection correct? If so, can you add this info the
change log and change the sex of the option.
Mikey
>
> Ben.
>
> > Signed-off-by: Haren Myneni <haren@us.ibm.com>
> >
> > ---
> > Documentation/kernel-parameters.txt | 4 ++++
> > arch/powerpc/include/asm/cputable.h | 6 ++++--
> > arch/powerpc/kernel/setup_64.c | 14 ++++++++++++++
> > 3 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > index ad7e2e5..2881e5f 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -809,6 +809,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> > to discrete, to make X server driver able to add WB
> > entry later. This parameter enables that.
> >
> > + enable_ppr [PPC/PSERIES]
> > + Saves user defined PPR when process enters to kernel
> > + and restores PPR at exit. But it impacts performance.
> > +
> > enable_timer_pin_1 [X86]
> > Enable PIN 1 of APIC timer
> > Can be useful to work around chipset bugs
> > diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
> > index b3c083d..880c469 100644
> > --- a/arch/powerpc/include/asm/cputable.h
> > +++ b/arch/powerpc/include/asm/cputable.h
> > @@ -203,6 +203,7 @@ extern const char *powerpc_base_platform;
> > #define CPU_FTR_POPCNTD LONG_ASM_CONST(0x0800000000000000)
> > #define CPU_FTR_ICSWX LONG_ASM_CONST(0x1000000000000000)
> > #define CPU_FTR_VMX_COPY LONG_ASM_CONST(0x2000000000000000)
> > +#define CPU_FTR_HAS_PPR LONG_ASM_CONST(0x4000000000000000)
> >
> > #ifndef __ASSEMBLY__
> >
> > @@ -432,7 +433,8 @@ extern const char *powerpc_base_platform;
> > CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
> > CPU_FTR_DSCR | CPU_FTR_SAO | CPU_FTR_ASYM_SMT | \
> > CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
> > - CPU_FTR_ICSWX | CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY)
> > + CPU_FTR_ICSWX | CPU_FTR_CFAR | CPU_FTR_HVMODE | \
> > + CPU_FTR_VMX_COPY | CPU_FTR_HAS_PPR)
> > #define CPU_FTRS_CELL (CPU_FTR_USE_TB | CPU_FTR_LWSYNC | \
> > CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \
> > CPU_FTR_ALTIVEC_COMP | CPU_FTR_MMCRA | CPU_FTR_SMT | \
> > @@ -454,7 +456,7 @@ extern const char *powerpc_base_platform;
> > (CPU_FTRS_POWER3 | CPU_FTRS_RS64 | CPU_FTRS_POWER4 | \
> > CPU_FTRS_PPC970 | CPU_FTRS_POWER5 | CPU_FTRS_POWER6 | \
> > CPU_FTRS_POWER7 | CPU_FTRS_CELL | CPU_FTRS_PA6T | \
> > - CPU_FTR_VSX)
> > + CPU_FTR_VSX | CPU_FTR_HAS_PPR)
> > #endif
> > #else
> > enum {
> > diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> > index 389bd4f..e4c1945 100644
> > --- a/arch/powerpc/kernel/setup_64.c
> > +++ b/arch/powerpc/kernel/setup_64.c
> > @@ -98,6 +98,8 @@ int dcache_bsize;
> > int icache_bsize;
> > int ucache_bsize;
> >
> > +static u32 enable_ppr = 0;
> > +
> > #ifdef CONFIG_SMP
> >
> > static char *smt_enabled_cmdline;
> > @@ -357,6 +359,9 @@ void __init setup_system(void)
> > {
> > DBG(" -> setup_system()\n");
> >
> > + if (cpu_has_feature(CPU_FTR_HAS_PPR) && !enable_ppr)
> > + cur_cpu_spec->cpu_features &= ~CPU_FTR_HAS_PPR;
> > +
> > /* Apply the CPUs-specific and firmware specific fixups to kernel
> > * text (nop out sections not relevant to this CPU or this firmware)
> > */
> > @@ -683,6 +688,15 @@ void __init setup_per_cpu_areas(void)
> > }
> > #endif
> >
> > +/* early_ppr kernel parameter to save/restore PPR register */
> > +static int __init early_ppr_enabled(char *str)
> > +{
> > + enable_ppr = 1;
> > +
> > + return 0;
> > +}
> > +
> > +early_param("enable_ppr", early_ppr_enabled);
> >
> > #ifdef CONFIG_PPC_INDIRECT_IO
> > struct ppc_pci_io ppc_pci_io;
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/6] powerpc: Add enable_ppr kernel parameter to enable PPR save/restore
2012-09-10 0:22 ` Michael Neuling
@ 2012-09-11 5:42 ` Haren Myneni
2012-09-11 5:55 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 6+ messages in thread
From: Haren Myneni @ 2012-09-11 5:42 UTC (permalink / raw)
To: Michael Neuling; +Cc: linuxppc-dev, anton, paulus
On 09/09/2012 05:22 PM, Michael Neuling wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
>> On Sun, 2012-09-09 at 04:37 -0700, Haren Myneni wrote:
>>> enable_ppr kernel parameter is used to enable PPR save and restore.
>>> Supported on Power7 and later processors.
>>>
>>> By default, CPU_FTR_HAS_PPR is set for POWER7. If this parameter is not
>>> passed, disable CPU_FTR_HAS_PPR.
>>
>> What is the point ? Obscure / magic kernel command line options to turn
>> on a feature are pointless. Nobody knows about them, nobody enables
>> them.
>>
>> What you are doing is guarantee that nobody's ever going to enable your
>> code, so your whole patch series is thus irrelevant :-)
>>
>> If there's a good reason to *avoid* your option, then maybe consider
>> adding an option to *disable* the PPR save/restore, though of course
>> that would bring the argument that if it needs to be disabled maybe we
>> shouldn't do it in the first place, or you might want to think of a
>> reasonable way to intuit what the option should be.
>
> IIRC, Haren was saying there's a 6% hit on null syscall for this. So we
> suggested having a cmdline option to disable it for distros.
>
> Haren, is my recollection correct? If so, can you add this info the
> change log and change the sex of the option.
Thanks Michael. Yes, we noticed 6% overhead with null syscall test.
Hence added cmdline option as suggested. I will add this comment in the
changelog.
Regarding the option name, I thought about various ones such as
retain_process_ppr, retain_smt_priority, save_ppr and etc. Finally added
'enable_ppr' since it enables CPU_FTR (CPU_FTR_HAS_PPR) which allows to
save/restore PPR value. Sure, I will change this option.
Thanks
Haren
>
> Mikey
>
>>
>> Ben.
>>
>>> Signed-off-by: Haren Myneni <haren@us.ibm.com>
>>>
>>> ---
>>> Documentation/kernel-parameters.txt | 4 ++++
>>> arch/powerpc/include/asm/cputable.h | 6 ++++--
>>> arch/powerpc/kernel/setup_64.c | 14 ++++++++++++++
>>> 3 files changed, 22 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>>> index ad7e2e5..2881e5f 100644
>>> --- a/Documentation/kernel-parameters.txt
>>> +++ b/Documentation/kernel-parameters.txt
>>> @@ -809,6 +809,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>> to discrete, to make X server driver able to add WB
>>> entry later. This parameter enables that.
>>>
>>> + enable_ppr [PPC/PSERIES]
>>> + Saves user defined PPR when process enters to kernel
>>> + and restores PPR at exit. But it impacts performance.
>>> +
>>> enable_timer_pin_1 [X86]
>>> Enable PIN 1 of APIC timer
>>> Can be useful to work around chipset bugs
>>> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
>>> index b3c083d..880c469 100644
>>> --- a/arch/powerpc/include/asm/cputable.h
>>> +++ b/arch/powerpc/include/asm/cputable.h
>>> @@ -203,6 +203,7 @@ extern const char *powerpc_base_platform;
>>> #define CPU_FTR_POPCNTD LONG_ASM_CONST(0x0800000000000000)
>>> #define CPU_FTR_ICSWX LONG_ASM_CONST(0x1000000000000000)
>>> #define CPU_FTR_VMX_COPY LONG_ASM_CONST(0x2000000000000000)
>>> +#define CPU_FTR_HAS_PPR LONG_ASM_CONST(0x4000000000000000)
>>>
>>> #ifndef __ASSEMBLY__
>>>
>>> @@ -432,7 +433,8 @@ extern const char *powerpc_base_platform;
>>> CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
>>> CPU_FTR_DSCR | CPU_FTR_SAO | CPU_FTR_ASYM_SMT | \
>>> CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
>>> - CPU_FTR_ICSWX | CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY)
>>> + CPU_FTR_ICSWX | CPU_FTR_CFAR | CPU_FTR_HVMODE | \
>>> + CPU_FTR_VMX_COPY | CPU_FTR_HAS_PPR)
>>> #define CPU_FTRS_CELL (CPU_FTR_USE_TB | CPU_FTR_LWSYNC | \
>>> CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \
>>> CPU_FTR_ALTIVEC_COMP | CPU_FTR_MMCRA | CPU_FTR_SMT | \
>>> @@ -454,7 +456,7 @@ extern const char *powerpc_base_platform;
>>> (CPU_FTRS_POWER3 | CPU_FTRS_RS64 | CPU_FTRS_POWER4 | \
>>> CPU_FTRS_PPC970 | CPU_FTRS_POWER5 | CPU_FTRS_POWER6 | \
>>> CPU_FTRS_POWER7 | CPU_FTRS_CELL | CPU_FTRS_PA6T | \
>>> - CPU_FTR_VSX)
>>> + CPU_FTR_VSX | CPU_FTR_HAS_PPR)
>>> #endif
>>> #else
>>> enum {
>>> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
>>> index 389bd4f..e4c1945 100644
>>> --- a/arch/powerpc/kernel/setup_64.c
>>> +++ b/arch/powerpc/kernel/setup_64.c
>>> @@ -98,6 +98,8 @@ int dcache_bsize;
>>> int icache_bsize;
>>> int ucache_bsize;
>>>
>>> +static u32 enable_ppr = 0;
>>> +
>>> #ifdef CONFIG_SMP
>>>
>>> static char *smt_enabled_cmdline;
>>> @@ -357,6 +359,9 @@ void __init setup_system(void)
>>> {
>>> DBG(" -> setup_system()\n");
>>>
>>> + if (cpu_has_feature(CPU_FTR_HAS_PPR) && !enable_ppr)
>>> + cur_cpu_spec->cpu_features &= ~CPU_FTR_HAS_PPR;
>>> +
>>> /* Apply the CPUs-specific and firmware specific fixups to kernel
>>> * text (nop out sections not relevant to this CPU or this firmware)
>>> */
>>> @@ -683,6 +688,15 @@ void __init setup_per_cpu_areas(void)
>>> }
>>> #endif
>>>
>>> +/* early_ppr kernel parameter to save/restore PPR register */
>>> +static int __init early_ppr_enabled(char *str)
>>> +{
>>> + enable_ppr = 1;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +early_param("enable_ppr", early_ppr_enabled);
>>>
>>> #ifdef CONFIG_PPC_INDIRECT_IO
>>> struct ppc_pci_io ppc_pci_io;
>>
>>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/6] powerpc: Add enable_ppr kernel parameter to enable PPR save/restore
2012-09-11 5:42 ` Haren Myneni
@ 2012-09-11 5:55 ` Benjamin Herrenschmidt
2012-09-28 22:11 ` Ryan Arnold
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2012-09-11 5:55 UTC (permalink / raw)
To: Haren Myneni; +Cc: paulus, Michael Neuling, linuxppc-dev, anton
On Mon, 2012-09-10 at 22:42 -0700, Haren Myneni wrote:
>
> Thanks Michael. Yes, we noticed 6% overhead with null syscall test.
> Hence added cmdline option as suggested. I will add this comment in
> the
> changelog.
>
> Regarding the option name, I thought about various ones such as
> retain_process_ppr, retain_smt_priority, save_ppr and etc. Finally
> added
> 'enable_ppr' since it enables CPU_FTR (CPU_FTR_HAS_PPR) which allows
> to
> save/restore PPR value. Sure, I will change this option.
No, that isn't a problem with the name. It's a problem with the polarity
of the option.
If you need a command line argument to enable the option, then nobody
will enable it, it's pointless.
Cheers,
Ben.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/6] powerpc: Add enable_ppr kernel parameter to enable PPR save/restore
2012-09-11 5:55 ` Benjamin Herrenschmidt
@ 2012-09-28 22:11 ` Ryan Arnold
0 siblings, 0 replies; 6+ messages in thread
From: Ryan Arnold @ 2012-09-28 22:11 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Michael Neuling, Adhemerval Zanella, sjmunroe, paulus, anton,
linuxppc-dev, Haren Myneni
On Tue, 2012-09-11 at 15:55 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2012-09-10 at 22:42 -0700, Haren Myneni wrote:
> >
> > Thanks Michael. Yes, we noticed 6% overhead with null syscall test.
> > Hence added cmdline option as suggested. I will add this comment in
> > the
> > changelog.
> >
> > Regarding the option name, I thought about various ones such as
> > retain_process_ppr, retain_smt_priority, save_ppr and etc. Finally
> > added
> > 'enable_ppr' since it enables CPU_FTR (CPU_FTR_HAS_PPR) which allows
> > to
> > save/restore PPR value. Sure, I will change this option.
>
> No, that isn't a problem with the name. It's a problem with the polarity
> of the option.
>
> If you need a command line argument to enable the option, then nobody
> will enable it, it's pointless.
In GLIBC (ppc.h) we'll be providing a user space API to change the
thread priority in user state. We're also interested in using this in
some of the locking constructs if performance tests indicate it's
beneficial.
I have concerns with being able to enable/disable this option at boot
time. Usually, in GLIBC we'll just do a kernel version check and enable
certain facilities if we're building against a particular kernel that
supports them.
In this case, with a configurable option, GLIBC is going to need the
kernel to export a hwcap bit that tells us whether we need to do the
save/restore ourselves. Having to check the hwcap, and do the
save/restore in user space will, of course, increase the overhead on our
side.
If no hwcap bit is provided and this is disabled at kernel boot time, no
check is done and the user process assumes it's running under a certain
priority when it is, in-fact, not. I don't care for this option. We'll
be hitting code paths that are ineffective and unnecessary.
Ryan S. Arnold
Linux Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-28 22:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-09 11:37 [PATCH 2/6] powerpc: Add enable_ppr kernel parameter to enable PPR save/restore Haren Myneni
2012-09-10 0:12 ` Benjamin Herrenschmidt
2012-09-10 0:22 ` Michael Neuling
2012-09-11 5:42 ` Haren Myneni
2012-09-11 5:55 ` Benjamin Herrenschmidt
2012-09-28 22:11 ` Ryan Arnold
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).