From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e33.co.us.ibm.com (e33.co.us.ibm.com [32.97.110.151]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "e33.co.us.ibm.com", Issuer "GeoTrust SSL CA" (not verified)) by ozlabs.org (Postfix) with ESMTPS id 93C112C0090 for ; Tue, 11 Sep 2012 15:42:20 +1000 (EST) Received: from /spool/local by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 10 Sep 2012 23:42:17 -0600 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 6237719D803F for ; Mon, 10 Sep 2012 23:42:15 -0600 (MDT) Received: from d03av06.boulder.ibm.com (d03av06.boulder.ibm.com [9.17.195.245]) by d03relay04.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q8B5gFZo233386 for ; Mon, 10 Sep 2012 23:42:15 -0600 Received: from d03av06.boulder.ibm.com (loopback [127.0.0.1]) by d03av06.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q8B5hhup002005 for ; Mon, 10 Sep 2012 23:43:44 -0600 Message-ID: <504ECF35.9070305@linux.vnet.ibm.com> Date: Mon, 10 Sep 2012 22:42:13 -0700 From: Haren Myneni MIME-Version: 1.0 To: Michael Neuling Subject: Re: [PATCH 2/6] powerpc: Add enable_ppr kernel parameter to enable PPR save/restore References: <1347190671.3418.12.camel@hbabu-laptop> <1347235922.2385.138.camel@pasglop> <13010.1347236558@neuling.org> In-Reply-To: <13010.1347236558@neuling.org> Content-Type: text/plain; charset=ISO-8859-1 Cc: linuxppc-dev@lists.ozlabs.org, anton@samba.org, paulus@samba.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 09/09/2012 05:22 PM, Michael Neuling wrote: > Benjamin Herrenschmidt 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 >>> >>> --- >>> 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 >