Re: [RFC PATCH 1/2] CPUfreq ondemand: update sampling rate withoutwaiting for next sampling

[MyungJoo Ham]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/