Re: [PATCH 3/5] timer: Change idle/iowait accounting semantics

[Frederic Weisbecker <fweisbec@gmail.com>]

On Sun, Oct 20, 2013 at 09:34:56AM +0200, Andreas Mohr wrote:
> Hi,
> 
> just wanted to report that this capricious open-coded (ok, lone-coded :)
> converter:
> 
> +static inline ktime_t us_to_ktime(u64 us)
> +{
> +	static const ktime_t ktime_zero = { .tv64 = 0 };
> +
> +	return ktime_add_us(ktime_zero, us);
> +}
> 
> triggered a virtual red flag in my processing.
> 
> We obviously seem to be compensating for a domain mismatch here:
> 
> +	iowait = get_cpu_iowait_time_us(cpu, NULL);
> +
> +	if (ktime_compare(idle, us_to_ktime(iowait)) > 0)
> +		idle = ktime_sub_us(idle, iowait);
> +
> 
> And sure enough:
> get_cpu_iowait_time_us() does a final to-us conversion
> only right before result is returned,
> and it's located right within the same build unit (tick-sched.c).
> (and then we go ahead and do a from-us round trip :-P)
> 
> So, in case we want to standardize on ktime_t domain
> for these parts of purely *in-kernel* time handling,
> how about adding a _unit-local_ helper for providing a ktime-only result
> and convert get_cpu_iowait_time_us() into a simple to-us *external user code*
> one-line wrapper for it?
> (and include updates to all other places which would benefit from this change)

Agreed. I admit I worked around this little part because that code is actually
not expected to be merged but was rather posted to raise some questions and may be
answer. But what you're suggesting is definetly the right way.


> And preferably launch such patch as a preparatory patch for this
> subsequent 3/5 patch, within the series?

Yep. Well, I hope we can find another way to fix the initial issue. But if that part of the code
is reused, I'll take care of fixing that.

Thanks!