| Message ID | 1496317188-15272-1-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | Accepted |
| Commit | d50a7d8acd780f54c48703ab1069c2e64a24e4a0 |
| Headers | show |
On 01/06/17 12:39, Daniel Lezcano wrote: > Some hardware have clusters with different idle states. The current code does > not support this and fails as it expects all the idle states to be identical. > > Because of this, the Mediatek mtk8173 had to create the same idle state for a > big.Little system and now the Hisilicon 960 is facing the same situation. > > Solve this by simply assuming the multiple driver will be needed for all the > platforms using the ARM generic cpuidle driver which makes sense because of the > different topologies we can support with a single kernel for ARM32 or ARM64. > > Every CPU has its own driver, so every single CPU can specify in the DT the > idle states. > > This simple approach allows to support the future dynamIQ system, current SMP > and HMP. > > It is unoptimal from a memory point of view for a system with a large number of > CPUs but nowadays there is no such system with a cpuidle driver on ARM. > While I agree this may be simple solution, but just not necessary for systems with symmetric idle states especially one with large number of CPUs. I don't like to see 96 CPU Idle driver on say ThunderX. So we *must* have some basic distinction done here. IMO, we can't punish a large SMP systems just because they don't have asymmetric idle states. -- Regards, Sudeep
On 02/06/17 10:25, Daniel Lezcano wrote: > On 02/06/2017 11:20, Sudeep Holla wrote: >> >> >> On 01/06/17 12:39, Daniel Lezcano wrote: >>> Some hardware have clusters with different idle states. The current code does >>> not support this and fails as it expects all the idle states to be identical. >>> >>> Because of this, the Mediatek mtk8173 had to create the same idle state for a >>> big.Little system and now the Hisilicon 960 is facing the same situation. >>> >>> Solve this by simply assuming the multiple driver will be needed for all the >>> platforms using the ARM generic cpuidle driver which makes sense because of the >>> different topologies we can support with a single kernel for ARM32 or ARM64. >>> >>> Every CPU has its own driver, so every single CPU can specify in the DT the >>> idle states. >>> >>> This simple approach allows to support the future dynamIQ system, current SMP >>> and HMP. >>> >>> It is unoptimal from a memory point of view for a system with a large number of >>> CPUs but nowadays there is no such system with a cpuidle driver on ARM. >>> >> >> While I agree this may be simple solution, but just not necessary for >> systems with symmetric idle states especially one with large number of >> CPUs. I don't like to see 96 CPU Idle driver on say ThunderX. So we >> *must* have some basic distinction done here. >> >> IMO, we can't punish a large SMP systems just because they don't have >> asymmetric idle states. > > Can you point me in the upstream kernel a DTS with 96 cpus and using the > cpuidle-arm driver ? > The bindings are upstream right. Not all DTS are upstream, firmware generate them especially for large systems. Check arch/arm64/boot/dts/cavium/thunder{,2}-{88,99}xx.dtsi, it has supports PSCI and firmware can update DTB to add the idle states. They are systems with 96 and 128 CPUs. -- Regards, Sudeep
On 02/06/2017 11:39, Sudeep Holla wrote: > > > On 02/06/17 10:25, Daniel Lezcano wrote: >> On 02/06/2017 11:20, Sudeep Holla wrote: >>> >>> >>> On 01/06/17 12:39, Daniel Lezcano wrote: >>>> Some hardware have clusters with different idle states. The current code does >>>> not support this and fails as it expects all the idle states to be identical. >>>> >>>> Because of this, the Mediatek mtk8173 had to create the same idle state for a >>>> big.Little system and now the Hisilicon 960 is facing the same situation. >>>> >>>> Solve this by simply assuming the multiple driver will be needed for all the >>>> platforms using the ARM generic cpuidle driver which makes sense because of the >>>> different topologies we can support with a single kernel for ARM32 or ARM64. >>>> >>>> Every CPU has its own driver, so every single CPU can specify in the DT the >>>> idle states. >>>> >>>> This simple approach allows to support the future dynamIQ system, current SMP >>>> and HMP. >>>> >>>> It is unoptimal from a memory point of view for a system with a large number of >>>> CPUs but nowadays there is no such system with a cpuidle driver on ARM. >>>> >>> >>> While I agree this may be simple solution, but just not necessary for >>> systems with symmetric idle states especially one with large number of >>> CPUs. I don't like to see 96 CPU Idle driver on say ThunderX. So we >>> *must* have some basic distinction done here. >>> >>> IMO, we can't punish a large SMP systems just because they don't have >>> asymmetric idle states. >> >> Can you point me in the upstream kernel a DTS with 96 cpus and using the >> cpuidle-arm driver ? >> > > The bindings are upstream right. Not all DTS are upstream, firmware > generate them especially for large systems. > > Check arch/arm64/boot/dts/cavium/thunder{,2}-{88,99}xx.dtsi, it has > supports PSCI and firmware can update DTB to add the idle states. > They are systems with 96 and 128 CPUs. Ok, so I have to assume there are out of tree DTB with idle state definitions. In other circumstances I would have just ignored this argument but I can admit the DTB blob thing is in the grey area between what we have to support upstream and out of tree changes. However, even if it is suboptimal, I'm convinced doing a per-cpu driver makes more sense. You said, we punish large SMP systems, I don't think it is true, especially from a performance point of view. Do you have access to such hardware to check if it is correct? We have more memory used but it is not related on how we implement the driver above but on the fixed array size in the cpuidle structures. I can take care of optimizing the memory usage in the cpuidle framework which will benefit all architectures but that will take some time. So can you consider acking this patch, so that unblocks other HMP idle development and I take care of optimizing the structure memory usage? -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
On 02/06/17 11:06, Daniel Lezcano wrote: > On 02/06/2017 11:39, Sudeep Holla wrote: >> >> >> On 02/06/17 10:25, Daniel Lezcano wrote: >>> On 02/06/2017 11:20, Sudeep Holla wrote: >>>> >>>> >>>> On 01/06/17 12:39, Daniel Lezcano wrote: >>>>> Some hardware have clusters with different idle states. The current code does >>>>> not support this and fails as it expects all the idle states to be identical. >>>>> >>>>> Because of this, the Mediatek mtk8173 had to create the same idle state for a >>>>> big.Little system and now the Hisilicon 960 is facing the same situation. >>>>> >>>>> Solve this by simply assuming the multiple driver will be needed for all the >>>>> platforms using the ARM generic cpuidle driver which makes sense because of the >>>>> different topologies we can support with a single kernel for ARM32 or ARM64. >>>>> >>>>> Every CPU has its own driver, so every single CPU can specify in the DT the >>>>> idle states. >>>>> >>>>> This simple approach allows to support the future dynamIQ system, current SMP >>>>> and HMP. >>>>> >>>>> It is unoptimal from a memory point of view for a system with a large number of >>>>> CPUs but nowadays there is no such system with a cpuidle driver on ARM. >>>>> >>>> >>>> While I agree this may be simple solution, but just not necessary for >>>> systems with symmetric idle states especially one with large number of >>>> CPUs. I don't like to see 96 CPU Idle driver on say ThunderX. So we >>>> *must* have some basic distinction done here. >>>> >>>> IMO, we can't punish a large SMP systems just because they don't have >>>> asymmetric idle states. >>> >>> Can you point me in the upstream kernel a DTS with 96 cpus and using the >>> cpuidle-arm driver ? >>> >> >> The bindings are upstream right. Not all DTS are upstream, firmware >> generate them especially for large systems. >> >> Check arch/arm64/boot/dts/cavium/thunder{,2}-{88,99}xx.dtsi, it has >> supports PSCI and firmware can update DTB to add the idle states. >> They are systems with 96 and 128 CPUs. > > Ok, so I have to assume there are out of tree DTB with idle state > definitions. In other circumstances I would have just ignored this > argument but I can admit the DTB blob thing is in the grey area between > what we have to support upstream and out of tree changes. > Not entirely true. It's clear, we support anything whose binding is upstream. I do agree that there are lots of out of tree bindings but I am not referring to them. I am just referring to out of tree DTS with already upstreamed binding. > However, even if it is suboptimal, I'm convinced doing a per-cpu driver > makes more sense. > OK, but I think a simple check to decide not to, on SMP system is not too much a ask ? > You said, we punish large SMP systems, I don't think it is true, > especially from a performance point of view. Do you have access to such > hardware to check if it is correct? > Sorry, I thought "punish" is bit a harsh but couldn't find the work. I can say penalize instead. No I don't have such a system to test. > We have more memory used but it is not related on how we implement the > driver above but on the fixed array size in the cpuidle structures. > Yes, I agree. But I will see what we can do at minimum to avoid it on system's that don't need it. > I can take care of optimizing the memory usage in the cpuidle framework > which will benefit all architectures but that will take some time. > Yest that's always good to have but I was not referring to that. > So can you consider acking this patch, so that unblocks other HMP idle > development and I take care of optimizing the structure memory usage? > I am thinking of checking DT to see if we need multiple driver support or not. But then it comes down to what Lorenzo had already posted. -- Regards, Sudeep
On 02/06/2017 12:14, Sudeep Holla wrote: > > > On 02/06/17 11:06, Daniel Lezcano wrote: >> On 02/06/2017 11:39, Sudeep Holla wrote: >>> >>> >>> On 02/06/17 10:25, Daniel Lezcano wrote: >>>> On 02/06/2017 11:20, Sudeep Holla wrote: >>>>> >>>>> >>>>> On 01/06/17 12:39, Daniel Lezcano wrote: >>>>>> Some hardware have clusters with different idle states. The current code does >>>>>> not support this and fails as it expects all the idle states to be identical. >>>>>> >>>>>> Because of this, the Mediatek mtk8173 had to create the same idle state for a >>>>>> big.Little system and now the Hisilicon 960 is facing the same situation. >>>>>> >>>>>> Solve this by simply assuming the multiple driver will be needed for all the >>>>>> platforms using the ARM generic cpuidle driver which makes sense because of the >>>>>> different topologies we can support with a single kernel for ARM32 or ARM64. >>>>>> >>>>>> Every CPU has its own driver, so every single CPU can specify in the DT the >>>>>> idle states. >>>>>> >>>>>> This simple approach allows to support the future dynamIQ system, current SMP >>>>>> and HMP. >>>>>> >>>>>> It is unoptimal from a memory point of view for a system with a large number of >>>>>> CPUs but nowadays there is no such system with a cpuidle driver on ARM. >>>>>> >>>>> >>>>> While I agree this may be simple solution, but just not necessary for >>>>> systems with symmetric idle states especially one with large number of >>>>> CPUs. I don't like to see 96 CPU Idle driver on say ThunderX. So we >>>>> *must* have some basic distinction done here. >>>>> >>>>> IMO, we can't punish a large SMP systems just because they don't have >>>>> asymmetric idle states. >>>> >>>> Can you point me in the upstream kernel a DTS with 96 cpus and using the >>>> cpuidle-arm driver ? >>>> >>> >>> The bindings are upstream right. Not all DTS are upstream, firmware >>> generate them especially for large systems. >>> >>> Check arch/arm64/boot/dts/cavium/thunder{,2}-{88,99}xx.dtsi, it has >>> supports PSCI and firmware can update DTB to add the idle states. >>> They are systems with 96 and 128 CPUs. >> >> Ok, so I have to assume there are out of tree DTB with idle state >> definitions. In other circumstances I would have just ignored this >> argument but I can admit the DTB blob thing is in the grey area between >> what we have to support upstream and out of tree changes. >> > > Not entirely true. It's clear, we support anything whose binding is > upstream. I do agree that there are lots of out of tree bindings but I > am not referring to them. I am just referring to out of tree DTS with > already upstreamed binding. That is what I meant :) >> However, even if it is suboptimal, I'm convinced doing a per-cpu driver >> makes more sense. >> > > OK, but I think a simple check to decide not to, on SMP system is not > too much a ask ? Yes, it is :) We end up with a hack by parsing and analyzing the DT based on the idea different idle states means different cluster where you stated right before in the topology comment that is not necessary true. So Lorenzo's patch adds heuristic which may be wrong. If we decide to consider all cpus with their own set of idle states, everything is solved with a cpuidle driver per cpu. The only drawback is the memory consumption. But on the other side, as the idle state are per cpu, we don't trash the cache. Moreover, we already had that, an array of idle states per cpu (see commit 46bcfad7), it was in the struct cpuidle_device instead of the struct cpuidle_driver. It has been moved away in the latter with the assumption the SMP have all the same idle states but now we see it is no longer true. Creating a cpuidle_driver per cpu is like using the previous approach. The only thing is we must improve to save memory, that's all (eg. a pointer allocating the idle states rather than hardcoding it). We can live with a driver per cpu and do the memory optimization. If we have an observed regression with large SMP systems, we fix it. >> You said, we punish large SMP systems, I don't think it is true, >> especially from a performance point of view. Do you have access to such >> hardware to check if it is correct? >> > > Sorry, I thought "punish" is bit a harsh but couldn't find the work. I > can say penalize instead. > > No I don't have such a system to test. > >> We have more memory used but it is not related on how we implement the >> driver above but on the fixed array size in the cpuidle structures. >> > > Yes, I agree. But I will see what we can do at minimum to avoid it on > system's that don't need it. > >> I can take care of optimizing the memory usage in the cpuidle framework >> which will benefit all architectures but that will take some time. >> > > Yest that's always good to have but I was not referring to that. > >> So can you consider acking this patch, so that unblocks other HMP idle >> development and I take care of optimizing the structure memory usage? >> > > I am thinking of checking DT to see if we need multiple driver support > or not. But then it comes down to what Lorenzo had already posted. > -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
On 02/06/17 11:40, Daniel Lezcano wrote: > On 02/06/2017 12:14, Sudeep Holla wrote: >> >> >> On 02/06/17 11:06, Daniel Lezcano wrote: >>> On 02/06/2017 11:39, Sudeep Holla wrote: >>>> >>>> >>>> On 02/06/17 10:25, Daniel Lezcano wrote: >>>>> On 02/06/2017 11:20, Sudeep Holla wrote: >>>>>> >>>>>> >>>>>> On 01/06/17 12:39, Daniel Lezcano wrote: >>>>>>> Some hardware have clusters with different idle states. The current code does >>>>>>> not support this and fails as it expects all the idle states to be identical. >>>>>>> >>>>>>> Because of this, the Mediatek mtk8173 had to create the same idle state for a >>>>>>> big.Little system and now the Hisilicon 960 is facing the same situation. >>>>>>> >>>>>>> Solve this by simply assuming the multiple driver will be needed for all the >>>>>>> platforms using the ARM generic cpuidle driver which makes sense because of the >>>>>>> different topologies we can support with a single kernel for ARM32 or ARM64. >>>>>>> >>>>>>> Every CPU has its own driver, so every single CPU can specify in the DT the >>>>>>> idle states. >>>>>>> >>>>>>> This simple approach allows to support the future dynamIQ system, current SMP >>>>>>> and HMP. >>>>>>> >>>>>>> It is unoptimal from a memory point of view for a system with a large number of >>>>>>> CPUs but nowadays there is no such system with a cpuidle driver on ARM. >>>>>>> >>>>>> >>>>>> While I agree this may be simple solution, but just not necessary for >>>>>> systems with symmetric idle states especially one with large number of >>>>>> CPUs. I don't like to see 96 CPU Idle driver on say ThunderX. So we >>>>>> *must* have some basic distinction done here. >>>>>> >>>>>> IMO, we can't punish a large SMP systems just because they don't have >>>>>> asymmetric idle states. >>>>> >>>>> Can you point me in the upstream kernel a DTS with 96 cpus and using the >>>>> cpuidle-arm driver ? >>>>> >>>> >>>> The bindings are upstream right. Not all DTS are upstream, firmware >>>> generate them especially for large systems. >>>> >>>> Check arch/arm64/boot/dts/cavium/thunder{,2}-{88,99}xx.dtsi, it has >>>> supports PSCI and firmware can update DTB to add the idle states. >>>> They are systems with 96 and 128 CPUs. >>> >>> Ok, so I have to assume there are out of tree DTB with idle state >>> definitions. In other circumstances I would have just ignored this >>> argument but I can admit the DTB blob thing is in the grey area between >>> what we have to support upstream and out of tree changes. >>> >> >> Not entirely true. It's clear, we support anything whose binding is >> upstream. I do agree that there are lots of out of tree bindings but I >> am not referring to them. I am just referring to out of tree DTS with >> already upstreamed binding. > > That is what I meant :) > >>> However, even if it is suboptimal, I'm convinced doing a per-cpu driver >>> makes more sense. >>> >> >> OK, but I think a simple check to decide not to, on SMP system is not >> too much a ask ? > > Yes, it is :) > If you are objecting out of the tree DTS whose bindings are upstream, then that's simply wrong. 1. Firmware can generate/update DTB 2. There's or was a plan to move DTS out of the tree. In short, bindings is all what matters. > We end up with a hack by parsing and analyzing the DT based on the idea > different idle states means different cluster where you stated right > before in the topology comment that is not necessary true. So Lorenzo's > patch adds heuristic which may be wrong. > Completely disagree. When it may look hacky, but it's not heuristics. The CPU idle bindings doesn't and need not link with CPU topology. I re-iterate that "different idle states doesn't means different cluster". It just means they are probably in different power domains. Power domains need not share topological boundaries. > If we decide to consider all cpus with their own set of idle states, > everything is solved with a cpuidle driver per cpu. > > The only drawback is the memory consumption. > Yes, I get that. I just feel that we need not do that for systems that don't need it. > But on the other side, as the idle state are per cpu, we don't trash the > cache. > > Moreover, we already had that, an array of idle states per cpu (see > commit 46bcfad7), it was in the struct cpuidle_device instead of the > struct cpuidle_driver. It has been moved away in the latter with the > assumption the SMP have all the same idle states but now we see it is no > longer true. > > Creating a cpuidle_driver per cpu is like using the previous approach. > The only thing is we must improve to save memory, that's all (eg. a > pointer allocating the idle states rather than hardcoding it). > > We can live with a driver per cpu and do the memory optimization. > > If we have an observed regression with large SMP systems, we fix it. > OK, I am fine with that if you agree that we can fix it later. -- Regards, Sudeep
On 02/06/2017 15:45, Sudeep Holla wrote: [ ... ] >> We can live with a driver per cpu and do the memory optimization. >> >> If we have an observed regression with large SMP systems, we fix it. >> > > OK, I am fine with that if you agree that we can fix it later. Yes, I agree. Shall I consider adding your acked-by ? -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm index 21340e0..f521448 100644 --- a/drivers/cpuidle/Kconfig.arm +++ b/drivers/cpuidle/Kconfig.arm @@ -4,6 +4,7 @@ config ARM_CPUIDLE bool "Generic ARM/ARM64 CPU idle Driver" select DT_IDLE_STATES + select CPU_IDLE_MULTIPLE_DRIVERS help Select this to enable generic cpuidle driver for ARM. It provides a generic idle driver whose idle states are configured diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c index f440d38..7080c38 100644 --- a/drivers/cpuidle/cpuidle-arm.c +++ b/drivers/cpuidle/cpuidle-arm.c @@ -18,6 +18,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/slab.h> +#include <linux/topology.h> #include <asm/cpuidle.h> @@ -44,7 +45,7 @@ static int arm_enter_idle_state(struct cpuidle_device *dev, return CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, idx); } -static struct cpuidle_driver arm_idle_driver = { +static struct cpuidle_driver arm_idle_driver __initdata = { .name = "arm_idle", .owner = THIS_MODULE, /* @@ -80,30 +81,42 @@ static const struct of_device_id arm_idle_state_match[] __initconst = { static int __init arm_idle_init(void) { int cpu, ret; - struct cpuidle_driver *drv = &arm_idle_driver; + struct cpuidle_driver *drv; struct cpuidle_device *dev; - /* - * Initialize idle states data, starting at index 1. - * This driver is DT only, if no DT idle states are detected (ret == 0) - * let the driver initialization fail accordingly since there is no - * reason to initialize the idle driver if only wfi is supported. - */ - ret = dt_init_idle_driver(drv, arm_idle_state_match, 1); - if (ret <= 0) - return ret ? : -ENODEV; - - ret = cpuidle_register_driver(drv); - if (ret) { - pr_err("Failed to register cpuidle driver\n"); - return ret; - } - - /* - * Call arch CPU operations in order to initialize - * idle states suspend back-end specific data - */ for_each_possible_cpu(cpu) { + + drv = kmemdup(&arm_idle_driver, sizeof(*drv), GFP_KERNEL); + if (!drv) { + ret = -ENOMEM; + goto out_fail; + } + + drv->cpumask = (struct cpumask *)cpumask_of(cpu); + + /* + * Initialize idle states data, starting at index 1. This + * driver is DT only, if no DT idle states are detected (ret + * == 0) let the driver initialization fail accordingly since + * there is no reason to initialize the idle driver if only + * wfi is supported. + */ + ret = dt_init_idle_driver(drv, arm_idle_state_match, 1); + if (ret <= 0) { + ret = ret ? : -ENODEV; + goto out_fail; + } + + ret = cpuidle_register_driver(drv); + if (ret) { + pr_err("Failed to register cpuidle driver\n"); + goto out_fail; + } + + /* + * Call arch CPU operations in order to initialize + * idle states suspend back-end specific data + */ ret = arm_cpuidle_init(cpu); /* @@ -141,10 +154,11 @@ static int __init arm_idle_init(void) dev = per_cpu(cpuidle_devices, cpu); cpuidle_unregister_device(dev); kfree(dev); + drv = cpuidle_get_driver(); + cpuidle_unregister_driver(drv); + kfree(drv); } - cpuidle_unregister_driver(drv); - return ret; } device_initcall(arm_idle_init);