Re: [PATCH V2 3/6] thermal: Add generic cpuhotplug cooling implementation
On 19 March 2012 17:15, Srivatsa S. Bhat
srivatsa.b...@linux.vnet.ibm.com wrote:
On 03/19/2012 11:47 AM, Amit Daniel Kachhap wrote:
This patch adds support for generic cpu thermal cooling low level
implementations using cpuhotplug based on the thermal level requested
from user. Different cpu related cooling devices can be registered by the
user and the binding of these cooling devices to the corresponding
trip points can be easily done as the registration APIs return the
cooling device pointer. The user of these APIs are responsible for
passing the cpumask.
I am really not aware of the cpu thermal cooling stuff, but since this patch
deals with CPU Hotplug (which I am interested in), I have some questions
below..
Signed-off-by: Amit Daniel Kachhap amit.kach...@linaro.org
+
+static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
+ unsigned long *state)
+{
+ int ret = -EINVAL;
+ struct hotplug_cooling_device *hotplug_dev;
+
+ mutex_lock(cooling_cpuhotplug_lock);
+ list_for_each_entry(hotplug_dev, cooling_cpuhotplug_list, node) {
+ if (hotplug_dev hotplug_dev-cool_dev == cdev) {
+ *state = hotplug_dev-hotplug_state;
+ ret = 0;
+ break;
+ }
+ }
+ mutex_unlock(cooling_cpuhotplug_lock);
+ return ret;
+}
+
+/*This cooling may be as ACTIVE type*/
+static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
+ unsigned long state)
+{
+ int cpuid, this_cpu = smp_processor_id();
What prevents this task from being migrated to another CPU?
IOW, what ensures that 'this_cpu' remains valid throughout this function?
I see that you are acquiring mutex locks below.. So this is definitely not
a preempt disabled section.. so I guess my question above is valid..
Or is this code bound to a particular cpu?
No this thread is not bound to a cpu. Your comment is valid and some
synchronization is needed for preemption. Thanks for pointing this
out.
+ struct hotplug_cooling_device *hotplug_dev;
+
+ mutex_lock(cooling_cpuhotplug_lock);
+ list_for_each_entry(hotplug_dev, cooling_cpuhotplug_list, node)
+ if (hotplug_dev hotplug_dev-cool_dev == cdev)
+ break;
+
+ mutex_unlock(cooling_cpuhotplug_lock);
+ if (!hotplug_dev || hotplug_dev-cool_dev != cdev)
+ return -EINVAL;
+
+ if (hotplug_dev-hotplug_state == state)
+ return 0;
+
+ /*
+ * This cooling device may be of type ACTIVE, so state field can
+ * be 0 or 1
+ */
+ if (state == 1) {
+ for_each_cpu(cpuid, hotplug_dev-allowed_cpus) {
+ if (cpu_online(cpuid) (cpuid != this_cpu))
What prevents the cpu numbered cpuid from being taken down right at this
moment?
Don't you need explicit synchronization with CPU Hotplug using something like
get_online_cpus()/put_online_cpus() here?
+ cpu_down(cpuid);
+ }
+ } else if (state == 0) {
+ for_each_cpu(cpuid, hotplug_dev-allowed_cpus) {
+ if (!cpu_online(cpuid) (cpuid != this_cpu))
Same here.
+ cpu_up(cpuid);
+ }
+ } else {
+ return -EINVAL;
+ }
+
+ hotplug_dev-hotplug_state = state;
+
+ return 0;
+}
+/* bind hotplug callbacks to cpu hotplug cooling device */
+static struct thermal_cooling_device_ops cpuhotplug_cooling_ops = {
+ .get_max_state = cpuhotplug_get_max_state,
+ .get_cur_state = cpuhotplug_get_cur_state,
+ .set_cur_state = cpuhotplug_set_cur_state,
+};
+
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V2 3/6] thermal: Add generic cpuhotplug cooling implementation
On 03/19/2012 11:47 AM, Amit Daniel Kachhap wrote:
This patch adds support for generic cpu thermal cooling low level
implementations using cpuhotplug based on the thermal level requested
from user. Different cpu related cooling devices can be registered by the
user and the binding of these cooling devices to the corresponding
trip points can be easily done as the registration APIs return the
cooling device pointer. The user of these APIs are responsible for
passing the cpumask.
I am really not aware of the cpu thermal cooling stuff, but since this patch
deals with CPU Hotplug (which I am interested in), I have some questions
below..
Signed-off-by: Amit Daniel Kachhap amit.kach...@linaro.org
+
+static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
+ unsigned long *state)
+{
+ int ret = -EINVAL;
+ struct hotplug_cooling_device *hotplug_dev;
+
+ mutex_lock(cooling_cpuhotplug_lock);
+ list_for_each_entry(hotplug_dev, cooling_cpuhotplug_list, node) {
+ if (hotplug_dev hotplug_dev-cool_dev == cdev) {
+ *state = hotplug_dev-hotplug_state;
+ ret = 0;
+ break;
+ }
+ }
+ mutex_unlock(cooling_cpuhotplug_lock);
+ return ret;
+}
+
+/*This cooling may be as ACTIVE type*/
+static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
+ unsigned long state)
+{
+ int cpuid, this_cpu = smp_processor_id();
What prevents this task from being migrated to another CPU?
IOW, what ensures that 'this_cpu' remains valid throughout this function?
I see that you are acquiring mutex locks below.. So this is definitely not
a preempt disabled section.. so I guess my question above is valid..
Or is this code bound to a particular cpu?
+ struct hotplug_cooling_device *hotplug_dev;
+
+ mutex_lock(cooling_cpuhotplug_lock);
+ list_for_each_entry(hotplug_dev, cooling_cpuhotplug_list, node)
+ if (hotplug_dev hotplug_dev-cool_dev == cdev)
+ break;
+
+ mutex_unlock(cooling_cpuhotplug_lock);
+ if (!hotplug_dev || hotplug_dev-cool_dev != cdev)
+ return -EINVAL;
+
+ if (hotplug_dev-hotplug_state == state)
+ return 0;
+
+ /*
+ * This cooling device may be of type ACTIVE, so state field can
+ * be 0 or 1
+ */
+ if (state == 1) {
+ for_each_cpu(cpuid, hotplug_dev-allowed_cpus) {
+ if (cpu_online(cpuid) (cpuid != this_cpu))
What prevents the cpu numbered cpuid from being taken down right at this moment?
Don't you need explicit synchronization with CPU Hotplug using something like
get_online_cpus()/put_online_cpus() here?
+ cpu_down(cpuid);
+ }
+ } else if (state == 0) {
+ for_each_cpu(cpuid, hotplug_dev-allowed_cpus) {
+ if (!cpu_online(cpuid) (cpuid != this_cpu))
Same here.
+ cpu_up(cpuid);
+ }
+ } else {
+ return -EINVAL;
+ }
+
+ hotplug_dev-hotplug_state = state;
+
+ return 0;
+}
+/* bind hotplug callbacks to cpu hotplug cooling device */
+static struct thermal_cooling_device_ops cpuhotplug_cooling_ops = {
+ .get_max_state = cpuhotplug_get_max_state,
+ .get_cur_state = cpuhotplug_get_cur_state,
+ .set_cur_state = cpuhotplug_set_cur_state,
+};
+
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
