From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752445AbdFNM4C (ORCPT ); Wed, 14 Jun 2017 08:56:02 -0400 Received: from mail-wr0-f173.google.com ([209.85.128.173]:34809 "EHLO mail-wr0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752025AbdFNMz7 (ORCPT ); Wed, 14 Jun 2017 08:55:59 -0400 Date: Wed, 14 Jun 2017 14:55:51 +0200 From: Daniel Lezcano To: Jean Wangtao Cc: Viresh Kumar , Tao Wang , rui.zhang@intel.com, Eduardo Valentin , Amit Kachhap , javi.merino@kernel.org, linux-kernel , linux-pm@vger.kernel.org, "Sunzhaosheng Sun(Zhaosheng)" , Vincent Guittot Subject: Re: [PATCH RFC 1/2] thermal/cpu idle cooling: Introduce cpu idle cooling driver Message-ID: <20170614125551.GI2261@mai> References: <1496653645-20283-1-git-send-email-kevin.wangtao@hisilicon.com> <20170606034135.GA5103@vireshk-i7> <20170609082040.GE2244@mai> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Jun 10, 2017 at 08:00:28PM +0200, Jean Wangtao wrote: > On 9 June 2017 at 10:20, Daniel Lezcano wrote: > > > On Tue, Jun 06, 2017 at 09:11:35AM +0530, viresh kumar wrote: > > > + Daniel > > > > > > On 05-06-17, 17:07, Tao Wang wrote: > > > > cpu idle cooling driver performs synchronized idle injection across > > > > all cpu in same cluster, offers a new method to cooling down cpu, > > > > that is similar to intel_power_clamp driver, but is basically > > > > designed for ARM platform. > > > > Each cluster has its own idle cooling device, each core has its own > > > > idle injection thread, idle injection thread use play_idle to enter > > > > idle. In order to reach deepest idle state, all cores are aligned by > > > > jiffies. the injected idle ratio can be controlled through cooling > > > > device interface. > > > > > > > > Signed-off-by: Tao Wang > > > > [ ... ] > > > > Hi Kevin, > > > > I'm failing to understand all the cpumask logic. > > > > Can you explain the rational? > > > > Thanks. > > > > This driver register cooling device for each cluster, so in module_init, we > init rest_cpu_mask as all cpu, then pick first cpu of the rest_cpu_mask, > set register_cpu_mask as all cpus in the cluster which contains the picked > cpu, and register idle cooling device, clear the register_cpu_mask out of > rest_cpu_mask after the register. repeat the process above until there are > no cpu left. > > In cpu_idle_cooling_register, we will check whether the input cpumask is > empty or have intersection with registered devices(that seems to be > unnecessary now, because the input is under control), the related_cpus is > the copy of input cpumask, the injected_cpus is the same as related_cpus if > there are no error happened during create_idle_thread(there is bug here if > create_idle_thread return error we should call stop_idle_thread to destroy > the idle injection thread already created). > > In create_idle_thread, we create idle injection thread for each cpu in > related_cpus, if operation success, set cpumask in injected_cpus. > In stop_idle_thread, we destory idle injection thread for each cpu in > injected_cpus which marks the threads we have created. > > In get_cpu_idle_injection_dev, we go through all the registered devices to > find one which contain the input cpu. > In idle_injection_cpu_online, when a cpu plug in, if this cpu is the first > cpu of the cluster or current control cpu of the cooling device is offline, > set this cpu as the control cpu. > In idle_injection_cpu_predown, when a cpu plug off, we set the first online > cpu of injected_cpus as the control cpu. > > In set_idle_state, the input cpu mask may cover several cooling device, so > we go through all the registered devices, each device who's related_cpus is > the subset of the input cpumask will execute idle injection with the input > ratio. > > I wish I have explained it clearly. > Well, you explained what it does but I was expecting the why. Is there any document describing in details your code or the logic? For example why the following pseudo-code would it be wrong in place of the cpumask dance? [pseudo code] cpumask_t cluster_id; cpumask_clear(cluster_id); for_each_cpu_possible(cpu) { if (cpumask_test_cpu(topology_physical_package_id(cpu]), &cluster_id)) continue; th_cool_dev = cpu_idle_cooling_register(cpumask_of(cpu)); if (IS_ERR(th_cool_dev) goto rollback; cpumask_set(topology_physical_package_id(cpu], &cluster_id); } > > > > +struct thermal_cooling_device * __init > > > > +cpu_idle_cooling_register(const struct cpumask *clip_cpus) > > > > +{ > > > > + struct cpu_idle_cooling_device *idle_cooling_dev; > > > > + struct thermal_cooling_device *ret; > > > > + unsigned long cpu; > > > > + char dev_name[THERMAL_NAME_LENGTH]; > > > > + > > > > + if (cpumask_empty(clip_cpus)) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + mutex_lock(&cpu_idle_cooling_lock); > > > > + get_online_cpus(); > > > > + list_for_each_entry(idle_cooling_dev, > > > > + &cpu_idle_cooling_dev_list, node) { > > > > + if (cpumask_intersects(idle_cooling_dev->related_cpus, > > > > + clip_cpus)) { > > > > + ret = ERR_PTR(-EINVAL); > > > > + goto exit_unlock; > > > > + } > > > > + } > > > > + > > > > + idle_cooling_dev = kzalloc(sizeof(*idle_cooling_dev), GFP_KERNEL); > > > > + if (!idle_cooling_dev) { > > > > + ret = ERR_PTR(-ENOMEM); > > > > + goto exit_unlock; > > > > + } > > > > + > > > > + if (!zalloc_cpumask_var(&idle_cooling_dev->related_cpus, > > GFP_KERNEL)) { > > > > + ret = ERR_PTR(-ENOMEM); > > > > + goto exit_free_dev; > > > > + } > > > > + > > > > + if (!zalloc_cpumask_var(&idle_cooling_dev->injected_cpus, > > GFP_KERNEL)) { > > > > + ret = ERR_PTR(-ENOMEM); > > > > + goto exit_free_related_cpus; > > > > + } > > > > + > > > > + cpumask_copy(idle_cooling_dev->related_cpus, clip_cpus); > > > > + cpu = cpumask_first(clip_cpus); > > > > + idle_cooling_dev->control_cpu = cpu; > > > > + idle_cooling_dev->id = topology_physical_package_id(cpu); > > > > + idle_cooling_dev->window_size = DEFAULT_WINDOW_SIZE; > > > > + idle_cooling_dev->duration = jiffies_to_msecs(DEFAULT_ > > DURATION_JIFFIES); > > > > + > > > > + if (create_idle_thread(idle_cooling_dev)) { > > > > + ret = ERR_PTR(-ENOMEM); > > > > + goto exit_free_injected_cpus; > > > > + } > > > > + > > > > + snprintf(dev_name, sizeof(dev_name), "thermal-cpuidle-%d", > > > > + idle_cooling_dev->id); > > > > + ret = thermal_cooling_device_register(dev_name, > > > > + idle_cooling_dev, > > > > + &cpu_idle_injection_cooling_ops); > > > > + if (IS_ERR(ret)) > > > > + goto exit_stop_thread; > > > > + > > > > + idle_cooling_dev->cooling_dev = ret; > > > > + > > > > + if (device_create_file(&idle_cooling_dev->cooling_dev->device, > > > > + &dev_attr_duration)) { > > > > + ret = ERR_PTR(-ENOMEM); > > > > + goto exit_unregister_cdev; > > > > + } > > > > + > > > > + if (device_create_file(&idle_cooling_dev->cooling_dev->device, > > > > + &dev_attr_window_size)) { > > > > + ret = ERR_PTR(-ENOMEM); > > > > + goto exit_remove_duration_attr; > > > > + } > > > > + > > > > + list_add(&idle_cooling_dev->node, &cpu_idle_cooling_dev_list); > > > > + > > > > + goto exit_unlock; > > > > + > > > > +exit_remove_duration_attr: > > > > + device_remove_file(&idle_cooling_dev->cooling_dev->device, > > > > + &dev_attr_duration); > > > > +exit_unregister_cdev: > > > > + thermal_cooling_device_unregister(idle_cooling_dev->cooling_dev); > > > > +exit_stop_thread: > > > > + stop_idle_thread(idle_cooling_dev); > > > > +exit_free_injected_cpus: > > > > + free_cpumask_var(idle_cooling_dev->injected_cpus); > > > > +exit_free_related_cpus: > > > > + free_cpumask_var(idle_cooling_dev->related_cpus); > > > > +exit_free_dev: > > > > + kfree(idle_cooling_dev); > > > > +exit_unlock: > > > > + put_online_cpus(); > > > > + mutex_unlock(&cpu_idle_cooling_lock); > > > > + return ret; > > > > +} > > > > + > > > > +void cpu_idle_cooling_unregister(struct thermal_cooling_device *cdev) > > > > +{ > > > > + struct cpu_idle_cooling_device *idle_cooling_dev; > > > > + > > > > + if (IS_ERR_OR_NULL(cdev)) > > > > + return; > > > > + > > > > + idle_cooling_dev = cdev->devdata; > > > > + > > > > + mutex_lock(&cpu_idle_cooling_lock); > > > > + get_online_cpus(); > > > > + list_del(&idle_cooling_dev->node); > > > > + put_online_cpus(); > > > > + mutex_unlock(&cpu_idle_cooling_lock); > > > > + > > > > + device_remove_file(&cdev->device, &dev_attr_window_size); > > > > + device_remove_file(&cdev->device, &dev_attr_duration); > > > > + thermal_cooling_device_unregister(idle_cooling_dev->cooling_dev); > > > > + > > > > + stop_idle_thread(idle_cooling_dev); > > > > + free_cpumask_var(idle_cooling_dev->injected_cpus); > > > > + free_cpumask_var(idle_cooling_dev->related_cpus); > > > > + kfree(idle_cooling_dev); > > > > +} > > > > + > > > > +static void __cpu_idle_cooling_exit(void) > > > > +{ > > > > + struct cpu_idle_cooling_device *idle_cooling_dev; > > > > + > > > > + while (!list_empty(&cpu_idle_cooling_dev_list)) { > > > > + idle_cooling_dev = list_first_entry(&cpu_idle_ > > cooling_dev_list, > > > > + struct cpu_idle_cooling_device, node); > > > > + cpu_idle_cooling_unregister(idle_cooling_dev->cooling_dev) > > ; > > > > + } > > > > + > > > > + if (hp_state > 0) > > > > + cpuhp_remove_state_nocalls(hp_state); > > > > +} > > > > + > > > > +static int __init cpu_idle_cooling_init(void) > > > > +{ > > > > + struct thermal_cooling_device *ret; > > > > + cpumask_t rest_cpu_mask = CPU_MASK_ALL; > > > > + const struct cpumask *register_cpu_mask; > > > > + > > > > + hp_state = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, > > > > + "thermal/cpu_idle_cooling:online", > > > > + idle_injection_cpu_online, > > > > + idle_injection_cpu_predown); > > > > + if (hp_state < 0) > > > > + return hp_state; > > > > + > > > > + do { > > > > + register_cpu_mask = > > > > + topology_core_cpumask(cpumask_ > > first(&rest_cpu_mask)); > > > > + > > > > + if (cpumask_empty(register_cpu_mask)) > > > > + break; > > > > + > > > > + ret = cpu_idle_cooling_register(register_cpu_mask); > > > > + if (IS_ERR(ret)) { > > > > + __cpu_idle_cooling_exit(); > > > > + return -ENOMEM; > > > > + } > > > > + } while (cpumask_andnot(&rest_cpu_mask, > > > > + &rest_cpu_mask, > > > > + register_cpu_mask)); > > > > + > > > > + return 0; > > > > +} > > > > +module_init(cpu_idle_cooling_init); > > > > + > > > > +static void __exit cpu_idle_cooling_exit(void) > > > > +{ > > > > + __cpu_idle_cooling_exit(); > > > > +} > > > > +module_exit(cpu_idle_cooling_exit); > > > > + > > > > +MODULE_LICENSE("GPL v2"); > > > > +MODULE_AUTHOR("Tao Wang "); > > > > +MODULE_DESCRIPTION("CPU Idle Cooling Driver for ARM Platform"); > > > > -- > > > > 1.7.9.5 > > > > > > -- > > > viresh > > > > -- > > > > Linaro.org │ Open source software for ARM SoCs > > > > Follow Linaro: Facebook | > > Twitter | > > Blog > > -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog