| Message ID | 1507531597-25947-1-git-send-email-anju@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | powerpc/powernv: Add kernel cmdline parameter to disable imc | expand |
Anju T Sudhakar <anju@linux.vnet.ibm.com> writes: > Add a kernel command line parameter option to disable In-Memory Collection > (IMC) counters and add documentation. This helps in debug. I'd really rather we didn't. Do we *really* need this? We don't have command line parameters to disable any of the other ~20 PMUs, why is this one special? cheers
Michael Ellerman <mpe@ellerman.id.au> writes: > Anju T Sudhakar <anju@linux.vnet.ibm.com> writes: > >> Add a kernel command line parameter option to disable In-Memory Collection >> (IMC) counters and add documentation. This helps in debug. > > I'd really rather we didn't. Do we *really* need this? > > We don't have command line parameters to disable any of the other ~20 > PMUs, why is this one special? You could also do the same thing by editing the device tree before booting your kernel, we do have the facility to do that in petitboot. A recent firmware patch: https://patchwork.ozlabs.org/patch/823249/ would fix the firmware implementation where the counters were already running before the INIT/START calls, which are likely the cause of the problems that this patch is trying to work around. I propose we have the firmware do the right thing and nothing special in kernel. i.e. not to merge this.
Hi mpe, stewart, On Wednesday 11 October 2017 01:55 AM, Stewart Smith wrote: > Michael Ellerman <mpe@ellerman.id.au> writes: >> Anju T Sudhakar <anju@linux.vnet.ibm.com> writes: >> >>> Add a kernel command line parameter option to disable In-Memory Collection >>> (IMC) counters and add documentation. This helps in debug. >> I'd really rather we didn't. Do we *really* need this? >> >> We don't have command line parameters to disable any of the other ~20 >> PMUs, why is this one special? This one is really helpful in debugging, incase if we want to proceed without nest counters OR core counters . But if we have the facility to do the same from petitboot, its fine. > You could also do the same thing by editing the device tree before > booting your kernel, we do have the facility to do that in petitboot. > > A recent firmware patch: https://patchwork.ozlabs.org/patch/823249/ > would fix the firmware implementation where the counters were already > running before the INIT/START calls, which are likely the cause of the > problems that this patch is trying to work around. > > I propose we have the firmware do the right thing and nothing special in > kernel. i.e. not to merge this. > Agreed. Thanks, Anju
Anju T Sudhakar <anju@linux.vnet.ibm.com> writes: > On Wednesday 11 October 2017 01:55 AM, Stewart Smith wrote: >> Michael Ellerman <mpe@ellerman.id.au> writes: >>> Anju T Sudhakar <anju@linux.vnet.ibm.com> writes: >>> >>>> Add a kernel command line parameter option to disable In-Memory Collection >>>> (IMC) counters and add documentation. This helps in debug. >>> I'd really rather we didn't. Do we *really* need this? >>> >>> We don't have command line parameters to disable any of the other ~20 >>> PMUs, why is this one special? > > This one is really helpful in debugging, incase if we want to proceed > without nest counters OR > core counters . But if we have the facility to do the same from > petitboot, its fine. https://patchwork.ozlabs.org/patch/824497/ (skiboot patch) should do the same thing, right? That means we come up with them off (as we should have from the start) and if you don't use the PMUs then the ucode won't be started.
On Thursday 12 October 2017 01:21 PM, Stewart Smith wrote: > Anju T Sudhakar <anju@linux.vnet.ibm.com> writes: >> On Wednesday 11 October 2017 01:55 AM, Stewart Smith wrote: >>> Michael Ellerman <mpe@ellerman.id.au> writes: >>>> Anju T Sudhakar <anju@linux.vnet.ibm.com> writes: >>>> >>>>> Add a kernel command line parameter option to disable In-Memory Collection >>>>> (IMC) counters and add documentation. This helps in debug. >>>> I'd really rather we didn't. Do we *really* need this? >>>> >>>> We don't have command line parameters to disable any of the other ~20 >>>> PMUs, why is this one special? >> This one is really helpful in debugging, incase if we want to proceed >> without nest counters OR >> core counters . But if we have the facility to do the same from >> petitboot, its fine. > https://patchwork.ozlabs.org/patch/824497/ (skiboot patch) should do the > same thing, right? That means we come up with them off (as we should > have from the start) and if you don't use the PMUs then the ucode won't > be started. Stewart, Thats right. With that skiboot patch, IMC will be paused at start.
Anju T Sudhakar <anju@linux.vnet.ibm.com> writes: > Hi mpe, stewart, > > > On Wednesday 11 October 2017 01:55 AM, Stewart Smith wrote: >> Michael Ellerman <mpe@ellerman.id.au> writes: >>> Anju T Sudhakar <anju@linux.vnet.ibm.com> writes: >>> >>>> Add a kernel command line parameter option to disable In-Memory Collection >>>> (IMC) counters and add documentation. This helps in debug. >>> I'd really rather we didn't. Do we *really* need this? >>> >>> We don't have command line parameters to disable any of the other ~20 >>> PMUs, why is this one special? > > This one is really helpful in debugging, incase if we want to proceed > without nest counters OR > core counters . But if we have the facility to do the same from > petitboot, its fine. I understand it's helpful for debugging. But we can't afford to have a command line option to disable every ~1K SLOC of code. If there's a compelling reason why we need to disable IMC *on production systems* - then a command line option is a possibility. Though really if there is a compelling reason for that maybe we have bigger problems :) This is also a very new driver, so we're still finding some bugs, in a few months it should be more stable and then the command line option will just be cruft. cheers
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 0549662..06a8da1 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -820,6 +820,13 @@ disable_ipv6= [IPV6] See Documentation/networking/ipv6.txt. + disable_imc= [PPC] + Format {nest | core | all} + Disable imc counters during boot. + nest----- Disable nest-imc counters. + core----- Disable core and thread imc counters. + all------ Disable nest, core and thread imc counters. + disable_mtrr_cleanup [X86] The kernel tries to adjust MTRR layout from continuous to discrete, to make X server driver able to add WB diff --git a/arch/powerpc/platforms/powernv/opal-imc.c b/arch/powerpc/platforms/powernv/opal-imc.c index 21f6531..e929f33 100644 --- a/arch/powerpc/platforms/powernv/opal-imc.c +++ b/arch/powerpc/platforms/powernv/opal-imc.c @@ -22,6 +22,28 @@ #include <asm/imc-pmu.h> #include <asm/cputhreads.h> +static bool disable_nest_imc; +static bool disable_core_imc; + +/* + * diasble_imc=nest: skip the registration of nest pmus. + * disable_imc=core: skip the registration of core and thread pmus. + * disable_imc=all : disables nest, core and thread. + */ +static int __init disable_imc_counters(char *p) +{ + if (strncmp(p, "nest", 4) == 0) + disable_nest_imc = true; + else if (strncmp(p, "core", 4) == 0) + disable_core_imc = true; + else if (strncmp(p, "all", 3) == 0) { + disable_nest_imc = true; + disable_core_imc = true; + } + return 0; +} +early_param("disable_imc", disable_imc_counters); + /* * imc_get_mem_addr_nest: Function to get nest counter memory region * for each chip @@ -169,6 +191,10 @@ static int opal_imc_counters_probe(struct platform_device *pdev) return -ENODEV; } + /* If kernel is booted with disable_imc parameters, then return */ + if (disable_nest_imc && disable_core_imc) + return -ENODEV; + for_each_compatible_node(imc_dev, NULL, IMC_DTB_UNIT_COMPAT) { if (of_property_read_u32(imc_dev, "type", &type)) { pr_warn("IMC Device without type property\n"); @@ -177,12 +203,21 @@ static int opal_imc_counters_probe(struct platform_device *pdev) switch (type) { case IMC_TYPE_CHIP: + if (disable_nest_imc) + continue; + domain = IMC_DOMAIN_NEST; break; case IMC_TYPE_CORE: + if (disable_core_imc) + continue; + domain =IMC_DOMAIN_CORE; break; case IMC_TYPE_THREAD: + if (disable_core_imc) + continue; + domain = IMC_DOMAIN_THREAD; break; default: