2012/2/26 Rafael J. Wysocki <rjw@xxxxxxx>:
> On Wednesday, February 22, 2012, MyungJoo Ham wrote:
>> When a new sampling rate is shorter than the current one, (e.g., 1 sec
>> --> 10 ms) regardless how short the new one is, the current ondemand
>> mechanism wait for the previously set timer to be expired.
>>
>> For example, if the user has just expressed that the sampling rate
>> should be 10 ms from now and the previous was 1000 ms, the new rate may
>> become effective 999 ms later, which could be not acceptable for the
>> user if the user has intended to speed up sampling because the system is
>> expected to react to CPU load fluctuation quickly from __now__.
>>
>> In order to address this issue, we need to cancel the previously set
>> timer (schedule_delayed_work) and reset the timer if resetting timer is
>> expected to trigger the delayed_work ealier.
>>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>> ---
[]
>> +
>> + if (!delayed_work_pending(&dbs_info->work))
>> + goto next;
>> +
>
> What about doing
>
> mutex_unlock(&dbs_info->timer_mutex);
> continue;
>
> here instead of the jump?
>
Like patch 2/2 of this patchset, I'll remove goto in the loop.
>
>> + timer = &dbs_info->work.timer;
>> + appointed_at = timer->expires;
>> +
>
> I would do
>
> next_sampling = jiffies + usecs_to_jiffies(new_rate);
>
> and compare that with timer->expires. Then, the if () below would look better.
> Or perhaps use new_rate_jiffies = usecs_to_jiffies(new_rate) and use that here
> and below?
>
Oh.. yes, this surely looks ugly. I'll update this.
>> + if (time_before(jiffies + usecs_to_jiffies(new_rate),
>> + appointed_at)) {
>> +
>> + mutex_unlock(&dbs_info->timer_mutex);
>
> I'm not sure if this isn't going to be racy. Have you verified that?
This unlock is added to avoid race condition against do_dbs_timer().
Unless the delay is 0 (polling_interval = 0ms?), do_dbs_timer() or
mutex_lock() took several ms, do_dbs_timer() won't be running again
holding the mutex after cancel_delayed_work_sync().
I've tested a few cases only backported on kernel 3.0; however, I'll
do more extensive testing on this part before submitting the next
iteration of the patchset and try to run this code with 3.3-rc5.
>
>> + cancel_delayed_work_sync(&dbs_info->work);
>> + mutex_lock(&dbs_info->timer_mutex);
>> +
>> + schedule_delayed_work_on(dbs_info->cpu, &dbs_info->work,
>> + usecs_to_jiffies(new_rate));
>> +
>> + }
>> +next:
>> + mutex_unlock(&dbs_info->timer_mutex);
>> + }
>> +}
>> +
>> static ssize_t store_sampling_rate(struct kobject *a, struct attribute *b,
>> const char *buf, size_t count)
>> {
>> @@ -265,7 +321,7 @@ static ssize_t store_sampling_rate(struct kobject *a, struct attribute *b,
>> ret = sscanf(buf, "%u", &input);
>> if (ret != 1)
>> return -EINVAL;
>> - dbs_tuners_ins.sampling_rate = max(input, min_sampling_rate);
>> + update_sampling_rate(input);
>> return count;
>> }
>
> Thanks,
> Rafael
Thank you.
Cheers!
MyungJoo.
--
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
