lkml.org 
[lkml]   [2012]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH V2 3/6] thermal: Add generic cpuhotplug cooling implementation
    From
    On 19 March 2012 17:15, Srivatsa S. Bhat
    <srivatsa.bhat@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.kachhap@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,
    >> +};
    >> +
    >
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-03-20 07:09    [W:0.030 / U:0.300 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site