Re: [RFC] nohz: no update idle entry time on get_cpu_idle/iowait_time_us()

[Yunhong Jiang <yunhong.jiang@linux.intel.com>]

On Sun, Oct 11, 2015 at 08:38:17PM +0200, Thomas Gleixner wrote:
> On Mon, 28 Sep 2015, Yunhong Jiang wrote:
> > Currently the get_cpu_idle/iowait_time_us() updates the idle_entrytime.
> > When it's invoked from another CPU and the target CPU has been on idle
> > already, it will update the idle_entrytime to now, which is incorrect.
> > 
> > However, the get_cpu_idle/iowait_time_us() is not guranteed to be called
> > on the target CPU. For example, the get_cpu_idle_time_us() seems is
> > invoked remotely on drivers/cpufreq/cpufreq_governor.c through
> > get_cpu_idle_time().
> > 
> > There is a check that the update happens only if a valid last_update_time
> > parameter passed. IMHO, this is more a hack because there is no guarantee
> > that it's invoked on the target CPU when last_update_time is valid.
> 
> Looking at the call sites, this last_update_time parameter is
> silly. Why is the calling code not taking the timestamp? There is
> hardly a requirement that this needs to be the same timestamp as the
> one which is used to calculate idle time. That cpufreq calculations
> are speculative anyway.
> 
> So we better get rid of that parameter completely.

Sure, will change the patch accordingly.

> 
> > In fact, we don't need update the idle stats from
> > get_cpu_idle/iowait_time_us(). Now the policy is, we record the
> >  entrytime when tick_nohz_start_idle() and update the stats
> >  when tick_nohz_stop_idle(). We calculate the stats on other situations.
> > 
> > Please notice:
> > 1) There is a bug currently that the tick_nohz_stop_idle() calls the
> > update_ts_time_stats() and update the idle_entrytime, which is sure
> > to be wrong. Removing the idle_entrytime update resolves the bug also.
> 
> Care to explain the actual bug and what wreckage it causes? If it's a
> real bug then removing "ts->idle_entrytime = now" needs to be a
> separate patch. AFAICT, it's a cosmetic issue.

Thanks for review and sorry for slow response.
Yes, it's an exaggeration to call it a bug. I just thought that updating the
idle_entrytime when exiting the idle state should be something wrong. But 
since the idle_entrytime is not used meaningful, it has no impact.

>  
> > 2) There is a small widows in tick_nohz_stop_idle() between
> >  update_ts_time_stats() and clear ts->idle_active, that
> > get_cpu_idle/iowait_time_us(), when invoked remotely, may double
> >  count last idle period. This window exists w/o this change and this
> > change does not fix it.
> 
> Calling any of those functions from a remote cpu is broken to begin
> with, especially on 32bit machines. And that does not change with your
> patch at all.

Yes, as stated, this change does not fix it.

Arjan also said that get_cpu_idle/iowait_time_us() should not be called from 
remote CPUs, but as stated, drivers/cpufreq/cpufreq_governor.c seems trying 
to invoke them remotely. I have no idea of the impact, Rafael or Viresh, can 
you give me some hints?

> 
> What we really need here is protecting the idle stats fields with a
> raw_spinlock.

If no calling from remote CPU, then we don't need the spinlock protection, 
right?

Thanks
--jyh
> 
> Thanks,
> 
> 	tglx