Hi Hidetoshi,
The patch looks good to me except the comments around the monotonicity
of the return value of the idle stats observer. I am unable to relate them
to the dependency on nr_iowait_cpu.
I see that when the reader queries for the idle stats and calls
get_cpu_idle_time_us(), the nr_iowait_cpu might be 0. When he later
queries get_cpu_iowait_time_us(), it may be >0 . Hence we will be
accounting for the idle time in both idle time and iowait time. This
is definitely a problem. But I do not understand what this has got to
do with the monotonicity of the time
returned. This is just for my understanding.
Thanks!
Regards
Preeti U Murthy
On 3/24/14, Hidetoshi Seto <seto.hideto...@jp.fujitsu.com> wrote:
<snip>
> + * Known bug: Return value is not monotonic in case if @last_update_time
> + * is NULL and therefore update is not performed. Because it includes
> + * cputime which is not determined idle or iowait so not accounted yet.
> + *
> * This function returns -1 if NOHZ is not enabled.
> */
> u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
> @@ -467,16 +489,28 @@ u64 get_cpu_idle_time_us(int cpu, u64
> *last_update_time)
>
> now = ktime_get();
> if (last_update_time) {
> - update_ts_time_stats(cpu, ts, now, last_update_time);
> - idle = ts->idle_sleeptime;
> + update_ts_time_stats(cpu, ts, now, &idle, NULL, 0);
> + *last_update_time = ktime_to_us(now);
> } else {
> - if (ts->idle_active && !nr_iowait_cpu(cpu)) {
> - ktime_t delta = ktime_sub(now, ts->idle_entrytime);
> + unsigned int seq;
>
> - idle = ktime_add(ts->idle_sleeptime, delta);
> - } else {
> + do {
> + seq = read_seqbegin(&ts->idle_sleeptime_seq);
> idle = ts->idle_sleeptime;
> - }
> + /*
> + * FIXME: At this point, delta is determined neither
> + * idle nor iowait. This function temporarily treat
> + * this delta depending on the value of nr_iowait
> + * when this function reaches here. It will result
> + * in non-monotonic return value. So user of this
> + * function must not expect monotonicity here.
> + */
> + if (ts->idle_active && !nr_iowait_cpu(cpu)
> + && (ktime_compare(now, ts->idle_entrytime) > 0)) {
> + ktime_t delta = ktime_sub(now,
> ts->idle_entrytime);
> + idle = ktime_add(ts->idle_sleeptime, delta);
> + }
> + } while (read_seqretry(&ts->idle_sleeptime_seq, seq));
> }
>
> return ktime_to_us(idle);
> @@ -496,6 +530,10 @@ EXPORT_SYMBOL_GPL(get_cpu_idle_time_us);
> * This time is measured via accounting rather than sampling,
> * and is as accurate as ktime_get() is.
> *
> + * Known bug: Return value is not monotonic in case if @last_update_time
> + * is NULL and therefore update is not performed. Because it includes
> + * cputime which is not determined idle or iowait so not accounted yet.
> + *
> * This function returns -1 if NOHZ is not enabled.
> */
> u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
> @@ -508,16 +546,28 @@ u64 get_cpu_iowait_time_us(int cpu, u64
> *last_update_time)
>
> now = ktime_get();
> if (last_update_time) {
> - update_ts_time_stats(cpu, ts, now, last_update_time);
> - iowait = ts->iowait_sleeptime;
> + update_ts_time_stats(cpu, ts, now, NULL, &iowait, 0);
> + *last_update_time = ktime_to_us(now);
> } else {
> - if (ts->idle_active && nr_iowait_cpu(cpu) > 0) {
> - ktime_t delta = ktime_sub(now, ts->idle_entrytime);
> + unsigned int seq;
>
> - iowait = ktime_add(ts->iowait_sleeptime, delta);
> - } else {
> + do {
> + seq = read_seqbegin(&ts->idle_sleeptime_seq);
> iowait = ts->iowait_sleeptime;
> - }
> + /*
> + * FIXME: At this point, delta is determined neither
> + * idle nor iowait. This function temporarily treat
> + * this delta depending on the value of nr_iowait
> + * when this function reaches here. It will result
> + * in non-monotonic return value. So user of this
> + * function must not expect monotonicity here.
> + */
> + if (ts->idle_active && nr_iowait_cpu(cpu) > 0
> + && (ktime_compare(now, ts->idle_entrytime) > 0)) {
> + ktime_t delta = ktime_sub(now,
> ts->idle_entrytime);
> + iowait = ktime_add(ts->iowait_sleeptime, delta);
> + }
> + } while (read_seqretry(&ts->idle_sleeptime_seq, seq));
> }
>
> return ktime_to_us(iowait);
> --
> 1.7.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
