linux-kernel - Re: [PATCH 1/2] nohz: use seqlock to avoid race on idle time stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 24 Mar 2014 13:15:54 +0530
From:	Preeti Murthy <preeti.lkml@...il.com>
To:	Hidetoshi Seto <seto.hidetoshi@...fujitsu.com>
Cc:	linux-kernel@...r.kernel.org,
	Fernando Luis Vazquez Cao <fernando_b1@....ntt.co.jp>,
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Arjan van de Ven <arjan@...ux.intel.com>,
	Oleg Nesterov <oleg@...hat.com>, preeti@...ux.vnet.ibm.com
Subject: Re: [PATCH 1/2] nohz: use seqlock to avoid race on idle time stats

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.hidetoshi@...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 majordomo@...r.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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/