Re: [PATCH RFC] tick/nohz: fix data races in get_cpu_idle_time_us()

[Alexey Dobriyan <adobriyan@gmail.com>]

On Tue, Jan 31, 2023 at 03:44:00PM +0100, Thomas Gleixner wrote:
> On Sat, Jan 28 2023 at 10:00, Yu Liao wrote:
> > selftest/proc/proc-uptime-001 complains:
> >   Euler:/mnt # while true; do ./proc-uptime-001; done
> >   proc-uptime-001: proc-uptime-001.c:41: main: Assertion `i1 >= i0' failed.
> >   proc-uptime-001: proc-uptime-001.c:41: main: Assertion `i1 >= i0' failed.
> >
> > /proc/uptime should be monotonically increasing. This occurs because
> > the data races between get_cpu_idle_time_us and
> > tick_nohz_stop_idle/tick_nohz_start_idle, for example:
> >
> > CPU0                        CPU1
> > get_cpu_idle_time_us
> >
> >                             tick_nohz_idle_exit
> >                               now = ktime_get();
> >                               tick_nohz_stop_idle
> >                                 update_ts_time_stats
> >                                   delta = ktime_sub(now, ts->idle_entrytime);
> >                                   ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta)
> >                                   ts->idle_entrytime = now
> >
> > now = ktime_get();
> > if (ts->idle_active && !nr_iowait_cpu(cpu)) {
> >     ktime_t delta = ktime_sub(now, ts->idle_entrytime);
> >     idle = ktime_add(ts->idle_sleeptime, delta);
> >     //idle is slightly greater than the actual value
> > } else {
> >     idle = ts->idle_sleeptime;
> > }
> >                             ts->idle_active = 0
> >
> > After this, idle = idle_sleeptime(actual idle value) + now(CPU0) - now(CPU1).
> > If get_cpu_idle_time_us() is called immediately after ts->idle_active = 0,
> > only ts->idle_sleeptime is returned, which is smaller than the previously
> > read one, resulting in a non-monotonically increasing idle time. In
> > addition, there are other data race scenarios not listed here.
> 
> Seriously this procfs accuracy is the least of the problems and if this
> would be the only issue then we could trivially fix it by declaring that
> the procfs output might go backwards.

Declarations on l-k are meaningless.

> If there would be a real reason to ensure monotonicity there then we could
> easily do that in the readout code.

People expect it to be monotonic. I wrote this test fully expecting
that /proc/uptime is monotonic. It didn't ever occured to me that
idletime can go backwards (nor uptime, but uptime is not buggy).

> But the real issue is that both get_cpu_idle_time_us() and
> get_cpu_iowait_time_us() can invoke update_ts_time_stats() which is way
> worse than the above procfs idle time going backwards.
> 
> If update_ts_time_stats() is invoked concurrently for the same CPU then
> ts->idle_sleeptime and ts->iowait_sleeptime are turning into random
> numbers.
> 
> This has been broken 12 years ago in commit 595aac488b54 ("sched:
> Introduce a function to update the idle statistics").