Messages in this thread |  | | Subject | Re: [PATCH V2 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver | From | Daniel Lezcano <> | Date | Fri, 23 Feb 2018 12:28:51 +0100 |
| |
On 23/02/2018 08:34, Viresh Kumar wrote: > On 21-02-18, 16:29, Daniel Lezcano wrote: >> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c >> index 5c219dc..9340216 100644 >> --- a/drivers/thermal/cpu_cooling.c >> +++ b/drivers/thermal/cpu_cooling.c >> @@ -10,18 +10,32 @@ >> * Viresh Kumar <viresh.kumar@linaro.org> >> * >> */ >> +#undef DEBUG > > Why is this required ?
It is usually added, so if you set the -DDEBUG flag when compiling, you don't get all the pr_debug traces for all files, but the just the ones where you commented the #undef above. pr_debug is a no-op otherwise.
>> +#define pr_fmt(fmt) "CPU cooling: " fmt > > I think you can use the dev_***() routines instead, as you can > directly the CPU device from anywhere.
Can we postpone this change for later ? All the file is using pr_* (cpufreq_cooling included). There is only one place where dev_err is used but it is removed by the patch 3/7.
[ ... ]
>> +#ifdef CONFIG_CPU_IDLE_THERMAL >> +/* >> + * The idle duration injection. As we don't have yet a way to specify >> + * from the DT configuration, let's default to a tick duration. >> + */ >> +#define DEFAULT_IDLE_TIME_US TICK_USEC > > I think this macro is a bit unnecessary here. Its used only at > initialization and so the same comment can be present there and you > can use TICK_USEC there. > > Else, Keep it as it is and remove the "idle_cycle" field from the > below structure, as it holds a constant forever.
Yes, makes sense.
>> +/** >> + * struct cpuidle_cooling_device - data for the idle cooling device >> + * @cdev: a pointer to a struct thermal_cooling_device >> + * @cpumask: a cpumask containing the CPU managed by the cooling device > > Missed @node here.
Ok.
>> + * @timer: a hrtimer giving the tempo for the idle injection cycles >> + * @kref: a kernel refcount on this structure >> + * @count: an atomic to keep track of the last task exiting the idle cycle >> + * @idle_cycle: an integer defining the duration of the idle injection >> + * @state: an normalized integer giving the state of the cooling device >> + */ >> +struct cpuidle_cooling_device { >> + struct thermal_cooling_device *cdev; >> + struct cpumask *cpumask; >> + struct list_head node; >> + struct hrtimer timer; >> + struct kref kref; >> + atomic_t count; >> + unsigned int idle_cycle; >> + unsigned int state; >> +}; >> + >> +/** >> + * @tsk: an array of pointer to the idle injection tasks >> + * @waitq: the waiq for the idle injection tasks >> + */ >> +struct cpuidle_cooling_tsk { >> + struct task_struct *tsk; >> + wait_queue_head_t waitq; >> +}; >> + >> +DEFINE_PER_CPU(struct cpuidle_cooling_tsk, cpuidle_cooling_tsk); > > static ?
Ok.
[ ... ]
>> +static s64 cpuidle_cooling_runtime(struct cpuidle_cooling_device *idle_cdev) >> +{ >> + s64 next_wakeup; >> + int state = idle_cdev->state; >> + >> + /* >> + * The function must never be called when there is no >> + * mitigation because: >> + * - that does not make sense >> + * - we end up with a division by zero >> + */ >> + BUG_ON(!state); > > As there is no locking in place, we can surely hit this case. What if > the state changed to 0 right before this routine was called ? > > I would suggest we should just return 0 in that case and get away with > the BUG_ON().
Ok.
>> + >> + next_wakeup = (s64)((idle_cdev->idle_cycle * 100) / state) - >> + idle_cdev->idle_cycle; >> + >> + return next_wakeup * NSEC_PER_USEC; >> +} >> + >> +/** >> + * cpuidle_cooling_injection_thread - Idle injection mainloop thread function >> + * @arg: a void pointer containing the idle cooling device address >> + * >> + * This main function does basically two operations: >> + * >> + * - Goes idle for a specific amount of time >> + * >> + * - Sets a timer to wake up all the idle injection threads after a >> + * running period >> + * >> + * That happens only when the mitigation is enabled, otherwise the >> + * task is scheduled out. >> + * >> + * In order to keep the tasks synchronized together, it is the last >> + * task exiting the idle period which is in charge of setting the >> + * timer. >> + * >> + * This function never returns. >> + */ >> +static int cpuidle_cooling_injection_thread(void *arg) >> +{ >> + struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO/2 }; >> + struct cpuidle_cooling_device *idle_cdev = arg; >> + struct cpuidle_cooling_tsk *cct = per_cpu_ptr(&cpuidle_cooling_tsk, >> + smp_processor_id()); > > this_cpu_ptr ?
yeah, even better.
>> + DEFINE_WAIT(wait); >> + >> + set_freezable(); >> + >> + sched_setscheduler(current, SCHED_FIFO, ¶m); >> + >> + while (1) { >> + s64 next_wakeup; >> + >> + prepare_to_wait(&cct->waitq, &wait, TASK_INTERRUPTIBLE); >> + >> + schedule(); >> + >> + atomic_inc(&idle_cdev->count); >> + >> + play_idle(idle_cdev->idle_cycle / USEC_PER_MSEC); >> + >> + /* >> + * The last CPU waking up is in charge of setting the >> + * timer. If the CPU is hotplugged, the timer will >> + * move to another CPU (which may not belong to the >> + * same cluster) but that is not a problem as the >> + * timer will be set again by another CPU belonging to >> + * the cluster, so this mechanism is self adaptive and >> + * does not require any hotplugging dance. >> + */ > > Well this depends on how CPU hotplug really happens. What happens to > the per-cpu-tasks which are in the middle of something when hotplug > happens? Does hotplug wait for those per-cpu-tasks to finish ? > >> + if (!atomic_dec_and_test(&idle_cdev->count)) >> + continue; >> + >> + if (!idle_cdev->state) >> + continue; >> + >> + next_wakeup = cpuidle_cooling_runtime(idle_cdev); >> + >> + hrtimer_start(&idle_cdev->timer, ns_to_ktime(next_wakeup), >> + HRTIMER_MODE_REL_PINNED); >> + } >> + >> + finish_wait(&cct->waitq, &wait); >> + >> + return 0; >> +} >> + >> +/** >> + * cpuidle_cooling_release - Kref based release helper >> + * @kref: a pointer to the kref structure >> + * >> + * This function is automatically called by the kref_put function when >> + * the idle cooling device refcount reaches zero. At this point, we >> + * have the guarantee the structure is no longer in use and we can >> + * safely release all the ressources. >> + */ >> +static void __init cpuidle_cooling_release(struct kref *kref) >> +{ >> + struct cpuidle_cooling_device *idle_cdev = >> + container_of(kref, struct cpuidle_cooling_device, kref); >> + >> + thermal_cooling_device_unregister(idle_cdev->cdev); >> + kfree(idle_cdev); >> +} > > Maybe just move it closer to the only function that calls it?
Ok.
[ ... ]
>> +/** >> + * cpuidle_cooling_get_cur_state - Get the current cooling state >> + * @cdev: the thermal cooling device >> + * @state: a pointer to the state >> + * >> + * The function just copy the state value from the private thermal >> + * cooling device structure, the mapping is 1 <-> 1. >> + * >> + * The function can not fail, it returns always zero. >> + */ >> +static int cpuidle_cooling_get_cur_state(struct thermal_cooling_device *cdev, >> + unsigned long *state) >> +{ >> + struct cpuidle_cooling_device *idle_cdev = cdev->devdata; > > This line isn't aligned properly. Spaces are present instead of a tab.
Ok.
[ ... ]
> Same at other places as well.
Ok, I will double check it.
[ ... ]
>> +static void cpuidle_cooling_unregister(void) >> +{ >> + struct cpuidle_cooling_device *tmp, *idle_cdev = NULL; >> + struct cpuidle_cooling_tsk *cct; >> + int cpu; >> + >> + list_for_each_entry_safe(idle_cdev, tmp, &cpuidle_cdev_list, node) { >> + for_each_cpu(cpu, idle_cdev->cpumask) { >> + cct = per_cpu_ptr(&cpuidle_cooling_tsk, cpu); >> + if (cct->tsk) >> + kthread_stop(cct->tsk); > > What about hrtimer ? Shouldn't that be stopped as well ?
IMO, practically it is not possible to have a timer running at this point, but rigorously it must be stopped in the release routine, so I will add it.
>> + kref_put(&idle_cdev->kref, cpuidle_cooling_release); >> + } >> + } >> +} >> + >> +/** >> + * cpuidle_cooling_register - Idle cooling device initialization function >> + * >> + * This function is in charge of creating a cooling device per cluster >> + * and register it to thermal framework. For this we rely on the >> + * topology as there is nothing yet describing better the idle state >> + * power domains. >> + * >> + * For each first CPU of the cluster's cpumask, we allocate the idle >> + * cooling device, initialize the general fields and then we initialze >> + * the rest in a per cpu basis. >> + * >> + * Returns zero on success, < 0 otherwise. > > This wouldn't get shown in the doc properly, it should be written as: > > * Return: zero on success, < 0 otherwise.
Ok.
>> + */ >> +int cpuidle_cooling_register(void) >> +{ >> + struct cpuidle_cooling_device *idle_cdev = NULL; >> + struct thermal_cooling_device *cdev; >> + struct cpuidle_cooling_tsk *cct; >> + struct task_struct *tsk; >> + struct device_node *np; >> + cpumask_t *cpumask; >> + char dev_name[THERMAL_NAME_LENGTH]; >> + int ret = -ENOMEM, cpu; >> + int index = 0; >> + >> + for_each_possible_cpu(cpu) { >> + cpumask = topology_core_cpumask(cpu); >> + >> + cct = per_cpu_ptr(&cpuidle_cooling_tsk, cpu); >> + >> + /* >> + * This condition makes the first cpu belonging to the >> + * cluster to create a cooling device and allocates >> + * the structure. Others CPUs belonging to the same >> + * cluster will just increment the refcount on the >> + * cooling device structure and initialize it. >> + */ >> + if (cpu == cpumask_first(cpumask)) { > > Your function still have few assumptions of cpu numbering and it will > break in few cases. What if the CPUs on a big Little system (4x4) are > present in this order: B L L L L B B B ?? > > This configuration can happen if CPUs in DT are marked as: 0-3 LITTLE, > 4-7 big and a big CPU is used by the boot loader to bring up Linux.
Ok, how can I sort it out ?
>> + np = of_cpu_device_node_get(cpu); >> + >> + idle_cdev = kzalloc(sizeof(*idle_cdev), GFP_KERNEL); >> + if (!idle_cdev) >> + goto out_fail; >> + >> + idle_cdev->idle_cycle = DEFAULT_IDLE_TIME_US; >> + >> + atomic_set(&idle_cdev->count, 0); > > This should already be 0, isn't it ?
Yes.
>> + >> + kref_init(&idle_cdev->kref); >> + >> + /* >> + * Initialize the timer to wakeup all the idle >> + * injection tasks >> + */ >> + hrtimer_init(&idle_cdev->timer, >> + CLOCK_MONOTONIC, HRTIMER_MODE_REL); >> + >> + /* >> + * The wakeup function callback which is in >> + * charge of waking up all CPUs belonging to >> + * the same cluster >> + */ >> + idle_cdev->timer.function = cpuidle_cooling_wakeup_fn; >> + >> + /* >> + * The thermal cooling device name >> + */ >> + snprintf(dev_name, sizeof(dev_name), "thermal-idle-%d", index++); >> + cdev = thermal_of_cooling_device_register(np, dev_name, >> + idle_cdev, >> + &cpuidle_cooling_ops); > > You registered the cooling device, while the idle_cdev is still > getting initialized. Ideally, we should register with the thermal core > (or any other framework) when we are fully ready. For example, any of > the callbacks present in cpuidle_cooling_ops() can get called by the > core after this point and you should be ready to handle them. It will > result in kernel crash in your case as idle_cdev isn't fully > initialized yet. For example, with the set-state callback you will end > up calling wake_up_task(NULL). > >> + if (IS_ERR(cdev)) { >> + ret = PTR_ERR(cdev); >> + goto out_fail; >> + } >> + >> + idle_cdev->cdev = cdev; >> + >> + idle_cdev->cpumask = cpumask; >> + >> + list_add(&idle_cdev->node, &cpuidle_cdev_list); > > I would have removed the above two blank lines as these can all go > together.
Ok.
>> + >> + pr_info("Created idle cooling device for cpus '%*pbl'\n", >> + cpumask_pr_args(cpumask)); > > I am not sure if it makes sense to print the message right here. We > can still fail while creating the cooling devices. Maybe a single > print at the very end of the function is more than enough?
Ok.
>> + } >> + >> + kref_get(&idle_cdev->kref); > > Did you check if the kref thing is really working here or not? I think > your structure will never get freed on errors because kref_init() > initializes the count by 1 and then you do an additional kref_get() > here for the first CPU of the cluster.
Yes, you are right. I will fix that.
>> + >> + init_waitqueue_head(&cct->waitq); >> + >> + tsk = kthread_create_on_cpu(cpuidle_cooling_injection_thread, >> + idle_cdev, cpu, "kidle_inject/%u"); >> + if (IS_ERR(tsk)) { >> + ret = PTR_ERR(tsk); >> + goto out_fail; >> + } >> + >> + cct->tsk = tsk; >> + >> + wake_up_process(tsk); >> + } >> + >> + return 0; >> + >> +out_fail: >> + cpuidle_cooling_unregister(); >> + pr_err("Failed to create idle cooling device (%d)\n", ret); > > So you are already printing the error message here, just make the > return type void as the caller of this can't do anything anyway.
Ok.
>> + >> + return ret; >> +} >> +#endif /* CONFIG_CPU_IDLE_THERMAL */
Thanks for the review.
-- Daniel
-- <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
|  |