Re: [PATCH 3/3] proc: consider NO_HZ when printing idle and iowait times

Re: [PATCH 3/3] proc: consider NO_HZ when printing idle and iowait times

[Michal Hocko]

On Thu 04-08-11 17:20:32, Michal Hocko wrote:
> show_stat handler of the /proc/stat file relies on kstat_cpu(cpu)
> statistics when priting information about idle and iowait times.
> This is OK if we are not using tickless kernel (CONFIG_NO_HZ) because
> counters are updated periodically.
> With NO_HZ things got more tricky because we are not doing idle/iowait
> accounting while we are tickless so the value might get outdated.
> Users of /proc/stat will notice that by unchanged idle/iowait values
> which is then interpreted as 0% idle/iowait time. From the user space
> POV this is an unexpected behavior and a change of the interface.
> 
> Let's fix this by using get_cpu_{idle,iowait}_time_us which accounts the
> total idle/iowait time since boot and it doesn't rely on sampling or any
> other periodic activity. Fall back to the previous behavior if NO_HZ is
> disabled or not configured.

I forgot to mention that this might be racy because we are updating
those per-cpu values without having preemption disabled or any other
locking which would be necessary as governors iterate over all CPUs.
Governors do not have to care about that because they are singletons. 
Introducing locks doesn't look like an option but I was thinking
about adding __get_cpu_{idle,iowait}_time_us which wouldn't call
update_ts_timestat and calculate the result instead.
I can add a patch which does that but I wanted to hear about general
approach first.

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH 0/3 RFC] Fix /proc/stat idle/iowait statistics for tickless kernel

[Michal Hocko]

Nobody interested or it just slipped through?

On Fri 05-08-11 13:42:21, Michal Hocko wrote:
> [Added cpufreq people into CC - the email thread started at
> https://lkml.org/lkml/2011/7/25/227]
> 
> This patch set aims at addressing /proc/stat issue which has been
> introduced with tickless kernel.
> In short, show_stat (proc handler) relies on kstat_cpu(i).cpustat
> statistics which are updated periodically so those numbers are more or
> less accurate.
> This is, however, not true with tickless kernel for idle and iowait
> counters because those are not updated while the cpu is in the tickless
> state. As the time when CPU might be tickless is not bounded, we can see
> really outdated values.
> The biggest problem is that tools which read /proc/stat interpret
> unchanged idle/iowait numbers as 0% idle/iowait which might confuse
> those who rely on them.
> 
> First patch in this series changes update_ts_time_stat semantic. The
> current implementation updates idle counter regardless we are in iowait
> loop at the moment. I see it as an optimization because cpufreq drivers,
> which are only users of those counters, care about busy vs. non-busy
> states so idle+iowait makes perfect sense. This, however, makes idle
> counter useless for others. 
> I think that calling using get_cpu_idle_time_us + get_cpu_iowait_time_us
> should have the same meaning (at least this is what we do for jiffies
> variants).
> This is necessary for the third patch.
> 
> The second patch is trivial and just a cleanup. It is not necessary for
> the series but I fixed those macros while I was at it.
> 
> The third patch uses get_cpu_{idle,iowait}_time_us to get precise
> counters. We still fall back to kstat_cpu if tickless kernel is
> disabled.
> 
> Thanks for any comments.
> 
> Michal Hocko (3):
>   tick: fix update_ts_time_stat idle accounting
>   cputime: clean up cputime_to_usecs and usecs_to_cputime macros
>   proc: consider NO_HZ when printing idle and iowait times
> 
>  drivers/cpufreq/cpufreq_conservative.c |    4 ++-
>  drivers/cpufreq/cpufreq_ondemand.c     |    4 ++-
>  fs/proc/stat.c                         |   41 ++++++++++++++++++++++++++-----
>  include/asm-generic/cputime.h          |    4 +-
>  kernel/time/tick-sched.c               |    8 +++---
>  5 files changed, 46 insertions(+), 15 deletions(-)
> 
> -- 
> 1.7.5.4

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH 2/3] cputime: clean up cputime_to_usecs and usecs_to_cputime macros

[Arnd Bergmann]

On Thursday 04 August 2011, Michal Hocko wrote:
> get rid of semicolon so that those expressions can be used also
> somewhere else than just in an assignment.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH] nohz: do not update idle/iowait counters from get_cpu_{idle,iowait}_time_us if not asked

[Michal Hocko]

On Fri 05-08-11 16:23:49, Michal Hocko wrote:
> On Thu 04-08-11 17:20:32, Michal Hocko wrote:
> > show_stat handler of the /proc/stat file relies on kstat_cpu(cpu)
> > statistics when priting information about idle and iowait times.
> > This is OK if we are not using tickless kernel (CONFIG_NO_HZ) because
> > counters are updated periodically.
> > With NO_HZ things got more tricky because we are not doing idle/iowait
> > accounting while we are tickless so the value might get outdated.
> > Users of /proc/stat will notice that by unchanged idle/iowait values
> > which is then interpreted as 0% idle/iowait time. From the user space
> > POV this is an unexpected behavior and a change of the interface.
> > 
> > Let's fix this by using get_cpu_{idle,iowait}_time_us which accounts the
> > total idle/iowait time since boot and it doesn't rely on sampling or any
> > other periodic activity. Fall back to the previous behavior if NO_HZ is
> > disabled or not configured.
> 
> I forgot to mention that this might be racy because we are updating
> those per-cpu values without having preemption disabled or any other
> locking which would be necessary as governors iterate over all CPUs.
> Governors do not have to care about that because they are singletons. 
> Introducing locks doesn't look like an option but I was thinking
> about adding __get_cpu_{idle,iowait}_time_us which wouldn't call
> update_ts_timestat and calculate the result instead.
> I can add a patch which does that but I wanted to hear about general
> approach first.

I guess we do not need a separate __get_cpu_{idle,iowait}_time_us
variant and rather reuse last_update_time parameter to determine whether
to update counters or not.

AFAICS we can still race with IRQ in update path (governors):
irq_enter
  tick_check_idle
    tick_check_nohz
      tick_nohz_stop_idle
but this is a separate issue IMO.
--- 
From 46b3c7735b23180299c6a85089571574f28527a2 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Mon, 22 Aug 2011 11:35:27 +0200
Subject: [PATCH] nohz: do not update idle/iowait counters from
 get_cpu_{idle,iowait}_time_us if not asked

get_cpu_{idle,iowait}_time_us update idle/iowait counters
unconditionally if the given CPU is in the idle loop. This doesn't work
well outside of CPU governors which are singletons so nobody (except for
IRQ) can race with them.
We will need to use both functions from /proc/stat handler to properly
handle nohz idle/iowait times.
Let's update those counters only if the given last_update_time parameter
is non-NULL which means that the caller is interested in updating.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 kernel/time/tick-sched.c |   37 +++++++++++++++++++++++++++++++------
 1 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 7ab44bc..5bcff38 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -198,7 +198,8 @@ static ktime_t tick_nohz_start_idle(int cpu, struct tick_sched *ts)
 /**
  * get_cpu_idle_time_us - get the total idle time of a cpu
  * @cpu: CPU number to query
- * @last_update_time: variable to store update time in
+ * @last_update_time: variable to store update time in. Do not update
+ * counters if NULL.
  *
  * Return the cummulative idle time (since boot) for a given
  * CPU, in microseconds.
@@ -211,20 +212,33 @@ static ktime_t tick_nohz_start_idle(int cpu, struct tick_sched *ts)
 u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 {
 	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	ktime_t now, idle;
 
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
+	now = ktime_get();
+	if (last_update_time) {
+		update_ts_time_stats(cpu, ts, now, last_update_time);
+		idle = ts->idle_sleeptime;
+	} else {
+		if (ts->idle_active && !nr_iowait_cpu(cpu)) {
+			ktime_t delta = ktime_sub(now, ts->idle_entrytime);
+			idle = ktime_add(ts->idle_sleeptime, delta);
+		} else
+			idle = ts->idle_sleeptime;
+	}
+
+	return ktime_to_us(idle);
 
-	return ktime_to_us(ts->idle_sleeptime);
 }
 EXPORT_SYMBOL_GPL(get_cpu_idle_time_us);
 
 /**
  * get_cpu_iowait_time_us - get the total iowait time of a cpu
  * @cpu: CPU number to query
- * @last_update_time: variable to store update time in
+ * @last_update_time: variable to store update time in. Do not update
+ * counters if NULL.
  *
  * Return the cummulative iowait time (since boot) for a given
  * CPU, in microseconds.
@@ -237,13 +251,24 @@ EXPORT_SYMBOL_GPL(get_cpu_idle_time_us);
 u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 {
 	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	ktime_t now, iowait;
 
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
+	now = ktime_get();
+	if (last_update_time) {
+		update_ts_time_stats(cpu, ts, now, last_update_time);
+		iowait = ts->iowait_sleeptime;
+	} else {
+		if (ts->idle_active && nr_iowait_cpu(cpu) > 0) {
+			ktime_t delta = ktime_sub(now, ts->idle_entrytime);
+			iowait = ktime_add(ts->iowait_sleeptime, delta);
+		} else
+			iowait = ts->iowait_sleeptime;
+	}
 
-	return ktime_to_us(ts->iowait_sleeptime);
+	return ktime_to_us(iowait);
 }
 EXPORT_SYMBOL_GPL(get_cpu_iowait_time_us);
 
-- 
1.7.5.4


-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH 0/3 RFC] Fix /proc/stat idle/iowait statistics for tickless kernel

[Andrew Morton]

On Wed, 17 Aug 2011 13:59:25 +0200
Michal Hocko <mhocko@suse.cz> wrote:

> Nobody interested or it just slipped through?

I was asleep on a beach at the time, am catching up now.

The patchset is a bit confusing, with an afterthough followup patch. 
Please redo it all and send a v2 series?

Then I'll queue it up and will periodically harrass Thomas with it, so
there's no point in hiding ;)