nohz: fix idle accounting in NO_HZ kernels
Subsystems (2)
v4 cover nohz: fix idle accounting in NO_HZ kernels
Hi all,
This patch set (rebased on v3.15-rc1) is my 4th try to fix an issue that idle/iowait of /proc/stat can go backward. Originally reported by Tetsuo and Fernando at last year, Mar 2013.
This v4 updates v3's approach, and adds description/comments to make patch's intent to be clear (I hope so).
Of course still reviews are welcome.
Thanks,
H.Seto
Hidetoshi Seto (2):
nohz: make updating sleep stats local
nohz: delayed iowait accounting for nohz idle time stats
include/linux/tick.h | 6 +- kernel/time/tick-sched.c | 179 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 136 insertions(+), 49 deletions(-)
PATCH v4 1/2 nohz: make updating sleep stats local
This patch is 1/2 of v4 patch set to fix an issue that idle/iowait of /proc/stat can go backward. Originally reported by Tetsuo and Fernando at last year, Mar 2013.
[BACKGROUNDS]: idle accounting on NO_HZ
If NO_HZ is enabled, cpu stops tick interrupt for itself before go sleep to be idle. It means that time stats of the sleeping cpu will not be updated in tick interrupt. Instead when cpu wakes up, it updates time stats by calculating idle duration time from timestamp at entering idle and current time as exiting idle.
OTOH, it can happen that there are some kind of observer who want to know how long the sleeping cpu have been idle. Imagine that userland runs top command or application who read /proc/stats. Therefore kernel provides get_cpu_{idle,iowait}_time_us() function for user to obtain current idle time stats of such sleeping cpu. This function reads time stats and timestamp at entering idle, and then return current idle time by adding duration calculated from timestamp and current time.
There are 2 problems:
(this patch 1/2 targets one, and following 2/2 targets another)
[PROBLEM 1]: there is no exclusive control.
It is easy to understand that there are 2 different cpu - an observing cpu where running a program observing idle cpu's stat and an observed cpu where performing idle. It means race can happen if observed cpu wakes up while it is observed. Observer can accidentally add calculated duration time (say delta) to time stats which is just updated by woken cpu. Soon correct idle time is returned in next turn, so it will result in backward time. Therefore readers must be excluded by writer.
Then time stats are updated by woken cpu so there are only one writer, right? No, unfortunately no. I'm not sure why are they, but in some reason the function get_cpu_{idle,iowait}_time_us() has ability to update sleeping cpu's time stats from observing cpu. From grep of today's kernel tree, this feature is used by cpuspeed module. Calling this function with this feature in periodically manner works like emulating tick for sleeping cpu.
In summary there are races between multiple writer and mutiple reader but no exclusive control on this idle time stats dataset.
[WHAT THIS PATCH PROPOSED]:
To fix problem 1, this patch 1/2 does following changes:
- Stop updating from get_cpu_{idle,iowait}_time_us():
It is hard to get reasonable performance with having exclusive locking for multiple writers. To limit the update areas to local, remove update functionality from these functions. It makes time stats to be updated by woken cpu only, so there are only one writer now!
- Adds seqcount as exclusive locking for NO_HZ idle:
It shoud be the minimum implemetation to avoid possible races between multiple readers vs. single writer. This lock protects:
idle_active, idle_entrytime, idle_sleeptime, iowait_sleeptime.
Now there is no other way to reach update_ts_time_stats(), fold this static routine into tick_nohz_stop_idle().
(Still there is problem 2. Continue to following patch 2/2.)
--- include/linux/tick.h | 5 ++- kernel/time/tick-sched.c | 75 ++++++++++++++++++++++------------------------ 2 files changed, 39 insertions(+), 41 deletions(-)
include/linux/tick.h +3 -2
@@ -62,6 +62,7 @@ struct tick_sched {unsigned long idle_calls;unsigned long idle_sleeps;int idle_active; +seqcount_t idle_sleeptime_seq;ktime_t idle_entrytime;ktime_t idle_waketime;ktime_t idle_exittime;
@@ -137,8 +138,8 @@ extern void tick_nohz_idle_enter(void);extern void tick_nohz_idle_exit (void );extern void tick_nohz_irq_exit (void );extern ktime_t tick_nohz_get_sleep_length (void ); -extern u64 get_cpu_idle_time_us (int cpu,u64 *last_update_time); -extern u64 get_cpu_iowait_time_us (int cpu,u64 *last_update_time); +extern u64 get_cpu_idle_time_us (int cpu,u64 *wall); +extern u64 get_cpu_iowait_time_us (int cpu,u64 *wall);# else /* !CONFIG_NO_HZ_COMMON */ static inline int tick_nohz_tick_stopped (void )
kernel/time/tick-sched.c +36 -39
149 lines not on this page · raw message
Signature · 1 line
1.7.1
PATCH v4 2/2 nohz: delayed iowait accounting for nohz idle time stats
In reply to Hidetoshi SetoThis patch is 2/2 of v4 patch set to fix an issue that idle/iowait of /proc/stat can go backward. Originally reported by Tetsuo and Fernando at last year, Mar 2013.
(Continued from previous patch 1/2.)
[PROBLEM 2]: broken iowait accounting.
As historical nature, cpu's idle time was accounted as either idle or iowait depending on the presence of tasks blocked by I/O. No one complain about it for a long time. However:
Quoted 4 lines
Still trying to wrap my head around it, but conceptually
get_cpu_iowait_time_us()doesn't make any kind of sense. iowait isn't per cpu since effectively tasks that aren't running aren't assigned a cpu (as Oleg already pointed out).
-- Peter Zijlstra
Now some kernel folks realized that accounting iowait as per-cpu does not make sense in SMP world. When we were in traditional UP era, cpu is considered to be waiting I/O if it is idle while nr_iowait > 0. But in these days with SMP systems, tasks easily migrate from a cpu where they issued an I/O to another cpu where they are queued after I/O completion.
Back to NO_HZ mechanism. Totally terrible thing here is that observer need to handle duration "delta" without knowing that nr_iowait of sleeping cpu can be changed easily by woken tasks even if cpu is sleeping. So it can happen that:
given:
idle time stats: idle=1000, iowait=100
stamp at idle entry: entry=50
nr tasks waiting io: nr_iowait=1
observer temporary assigns delta as iowait at 1st place, (but does not do update (=account delta to time stats)):
1st reader's query @ now = 60:
idle=1000
iowait=110 (=100+(60-50))
then blocked task is queued to runqueue of other active cpu, woken up from io_schedule{,_timeout}() and do atomic_dec() from the remote:
nr_iowait=0
and at last in 2nd turn observer assign delta as idle:
2nd reader's query @ now = 70:
idle=1020 (=1000+(70-50))
iowait=100
You will see that iowait is decreased from 110 to 100.
In summary, the original problem that iowait of /proc/stats can go backward is a kind of hidden bug, while at the same time iowait accounting has fundamental problem and needs to be precisely reworked.
[TARGET OF THIS PATCH]:
Complete rework for iowait accounting implies that some user interfaces might be replaced completely. It will introduce new counter or so, and kill per-cpu iowait counter which is known to being nonsense. It will take long time to be achieved, considering time to make the problem to a public knowledge, and also time for interface transition. Anyway as long as linux try to be reliable OS, such drastic change must be placed on mainline kernel in development sooner or later.
OTOH, drastic change like above is not acceptable for conservative environment, such as stable kernels, some distributor's kernel and so on, due to respect for compatibility. Still these kernel expects per-cpu iowait counter works as well as it might have been. Therefore for such environment, applying a simple patch to fix behavior of counter will be appreciated rather than replacing an over-used interface or changing an existing usage/semantics.
This patch targets latter kernels mainly. It does not do too much, but intend to fix the idle stats counters to be monotonic. I believe that mainline kernel should pick up this patch too, because it surely fix a hidden bug and does not intercept upcoming drastic change.
Again, in summary, this patch does not do drastic change to cope with problem 2. But it just fix behavior of idle/iowait counter of /proc/stats.
[WHAT THIS PATCH PROPOSED]:
The main reason of the bug is that observers have no idea to determine the delta to be idle or iowait at the first place.
So I introduced delayed iowait accounting to allow observers to assign delta based on status of observed cpu at idle entry.
Refer comment in patch for the detail.
[References]:
First report from Fernando:
[RFC] iowait/idle time accounting hiccups in NOHZ kernels
https://lkml.org/lkml/2013/3/18/962
Steps to reproduce guided by Tetsuo:
https://lkml.org/lkml/2013/4/1/201
1st patchset from Frederic:
[PATCH 0/4] nohz: Fix racy sleeptime stats
https://lkml.org/lkml/2013/8/8/638
[PATCH RESEND 0/4] nohz: Fix racy sleeptime stats
https://lkml.org/lkml/2013/8/16/274
2nd patchset from Frederic:
[RFC PATCH 0/5] nohz: Fix racy sleeptime stats v2
https://lkml.org/lkml/2013/10/19/86
My previous patch set:
[PATCH 0/2] nohz: fix idle accounting in NO_HZ kernels
https://lkml.org/lkml/2014/3/23/256
[PATCH 0/2 v2] nohz: fix idle accounting in NO_HZ kernels
https://lkml.org/lkml/2014/3/30/315
[PATCH v3 0/2] nohz: fix idle accounting in NO_HZ kernels
https://lkml.org/lkml/2014/4/10/133
v4: rework delayed iowait accounting: check nr_iowait at entry
(not to account iowait at out of io boundary) update description/comments to explain more details
v3: use seqcount instead of seqlock
(achieved by inserting cleanup as former patch) plus introduce delayed iowait accounting
v2: update comments and description about problem 2.
include fix for minor typo
--- include/linux/tick.h | 1 + kernel/time/tick-sched.c | 122 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 106 insertions(+), 17 deletions(-)
include/linux/tick.h +1 -0
@@ -68,6 +68,7 @@ struct tick_sched {ktime_t idle_exittime;ktime_t idle_sleeptime;ktime_t iowait_sleeptime; +ktime_t iowait_pending;ktime_t sleep_length;unsigned long last_jiffies;unsigned long next_jiffies;
kernel/time/tick-sched.c +105 -17
159 lines not on this page · raw message
Signature · 1 line
1.7.1
Re: [PATCH 2/2] nohz: delayed iowait accounting for nohz idle time stats
Quoted 87 lines · Hidetoshi Seto
This patch is 2/2 of v4 patch set to fix an issue that idle/iowait of
/proc/statcan go backward. Originally reported by Tetsuo and Fernando at last year, Mar 2013.(Continued from previous patch 1/2.)
[PROBLEM 2]: broken iowait accounting.
As historical nature, cpu's idle time was accounted as either idle or iowait depending on the presence of tasks blocked by I/O. No one complain about it for a long time. However:
Quoted 4 lines
Still trying to wrap my head around it, but conceptually
get_cpu_iowait_time_us()doesn't make any kind of sense. iowait isn't per cpu since effectively tasks that aren't running aren't assigned a cpu (as Oleg already pointed out).-- Peter Zijlstra
Now some kernel folks realized that accounting iowait as per-cpu does not make sense in SMP world. When we were in traditional UP era, cpu is considered to be waiting I/O if it is idle while
nr_iowait> 0. But in these days with SMP systems, tasks easily migrate from a cpu where they issued an I/O to another cpu where they are queued after I/O completion.Back to
NO_HZmechanism. Totally terrible thing here is that observer need to handle duration "delta" without knowing thatnr_iowaitof sleeping cpu can be changed easily by woken tasks even if cpu is sleeping. So it can happen that:given:
idle time stats: idle=1000, iowait=100
stamp at idle entry: entry=50
nr tasks waiting io: nr_iowait=1observer temporary assigns delta as iowait at 1st place, (but does not do update (=account delta to time stats)):
1st reader's query @ now = 60:
idle=1000
iowait=110 (=100+(60-50))then blocked task is queued to runqueue of other active cpu, woken up from io_schedule{,_timeout}() and do
atomic_dec()from the remote:
nr_iowait=0and at last in 2nd turn observer assign delta as idle:
2nd reader's query @ now = 70:
idle=1020 (=1000+(70-50))
iowait=100You will see that iowait is decreased from 110 to 100.
In summary, the original problem that iowait of
/proc/statscan go backward is a kind of hidden bug, while at the same time iowait accounting has fundamental problem and needs to be precisely reworked.[TARGET OF THIS PATCH]:
Complete rework for iowait accounting implies that some user interfaces might be replaced completely. It will introduce new counter or so, and kill per-cpu iowait counter which is known to being nonsense. It will take long time to be achieved, considering time to make the problem to a public knowledge, and also time for interface transition. Anyway as long as linux try to be reliable OS, such drastic change must be placed on mainline kernel in development sooner or later.
OTOH, drastic change like above is not acceptable for conservative environment, such as stable kernels, some distributor's kernel and so on, due to respect for compatibility. Still these kernel expects per-cpu iowait counter works as well as it might have been. Therefore for such environment, applying a simple patch to fix behavior of counter will be appreciated rather than replacing an over-used interface or changing an existing usage/semantics.
This patch targets latter kernels mainly. It does not do too much, but intend to fix the idle stats counters to be monotonic. I believe that mainline kernel should pick up this patch too, because it surely fix a hidden bug and does not intercept upcoming drastic change.
Again, in summary, this patch does not do drastic change to cope with problem 2. But it just fix behavior of idle/iowait counter of
/proc/stats.
Quoted 8 lines
[WHAT THIS PATCH PROPOSED]:
The main reason of the bug is that observers have no idea to determine the delta to be idle or iowait at the first place.
So I introduced delayed iowait accounting to allow observers to assign delta based on status of observed cpu at idle entry.
So the problem I have with this is that it makes CONFIG_NOHZ=[ny] kernels behave quite differently.
Ideally NOHZ=y and NOHZ=n behave the same, my proposed solution the other day does just that. This one OTOH seems to generate entirely different results between those kernels.
It also doesn't really simplify the code; there's quite a bit of complexity introduced to paper over this silly stuff.
Re: [PATCH 2/2] nohz: delayed iowait accounting for nohz idle time stats
Quoted 35 lines · Peter Zijlstra
Quoted 34 lines · Hidetoshi Seto
[TARGET OF THIS PATCH]:
Complete rework for iowait accounting implies that some user interfaces might be replaced completely. It will introduce new counter or so, and kill per-cpu iowait counter which is known to being nonsense. It will take long time to be achieved, considering time to make the problem to a public knowledge, and also time for interface transition. Anyway as long as linux try to be reliable OS, such drastic change must be placed on mainline kernel in development sooner or later.
OTOH, drastic change like above is not acceptable for conservative environment, such as stable kernels, some distributor's kernel and so on, due to respect for compatibility. Still these kernel expects per-cpu iowait counter works as well as it might have been. Therefore for such environment, applying a simple patch to fix behavior of counter will be appreciated rather than replacing an over-used interface or changing an existing usage/semantics.
This patch targets latter kernels mainly. It does not do too much, but intend to fix the idle stats counters to be monotonic. I believe that mainline kernel should pick up this patch too, because it surely fix a hidden bug and does not intercept upcoming drastic change.
Again, in summary, this patch does not do drastic change to cope with problem 2. But it just fix behavior of idle/iowait counter of
/proc/stats.[WHAT THIS PATCH PROPOSED]:
The main reason of the bug is that observers have no idea to determine the delta to be idle or iowait at the first place.
Quoted 6 lines
Quoted 3 lines
So I introduced delayed iowait accounting to allow observers to assign delta based on status of observed cpu at idle entry.
So the problem I have with this is that it makes
CONFIG_NOHZ=[ny]kernels behave quite differently.
It is not true.
There are already differences before applying my patches. The behavior of NOHZ=y kernel diverged from original since it was born.
Please note that no one complained about this difference.
I just want to fix a counter not to go backward.
It's a simple bug, isn't it?
Quoted 3 lines
Ideally NOHZ=y and NOHZ=n behave the same, my proposed solution the other day does just that. This one OTOH seems to generate entirely different results between those kernels.
As you know, behaviors of NOHZ=[ny] are both crap because of per-cpu iowait accounting.
I guess we should say:
Ideally NOHZ=[ny] behave the same "in the proper way."
What you proposed will do too much to make one nonsense to another nonsense. It will be unhelpful for people...
Quoted 2 lines
It also doesn't really simplify the code; there's quite a bit of complexity introduced to paper over this silly stuff.
Don't complicate things. I want to talk about a small simple bug.
a) today's NOHZ=n
- provides per-cpu iowait counter (nonsense but still loved by innocent userland)
b) today's NOHZ=y
- provides per-cpu iowait counter
- use it's own idle time accounting different from a)
- have a *bug* that counter might go backward
b') NOHZ=y + my patch
- provides per-cpu iowait counter
- use it's own idle time accounting same as b)
- *bug* in b) have gone
- instead accept gap in iowait value from b)
- "pending" will not bloat more than one iowait span
c) unified something for NOHZ=[ny] (you proposed)
- provides per-cpu iowait counter
- use new accounting different from a) and also b)
d) ideal goal (=not designed & realized yet)
- no per-cpu iowait counter any more
- use new proper accounting different from all of above
I just want to make b) to b') by a patch as small as possible.
What you proposed will make both of a) and b) to c). I think it does too much and changing a) is not required here. (from my conservative perspective, patch must be non-intrusive)
Our final goal must be making d) to replace all nasty things around there. But still there is no idea to do that. And such big jump will not fit to stable environments.
Again, I just want to fix a small bug here...
I believe my patch is enough simple to do a minimum fix. Please tell me if you have more simple/better way to fix this long-standing minor bug, not only for mainline in development but also for conservative stables like distributor's kernel.
Thanks,
H.Seto
Re: [PATCH 2/2] nohz: delayed iowait accounting for nohz idle time stats
Quoted 40 lines · Hidetoshi Seto
Quoted 39 lines · Peter Zijlstra
Quoted 37 lines · Hidetoshi Seto
[TARGET OF THIS PATCH]:
Complete rework for iowait accounting implies that some user interfaces might be replaced completely. It will introduce new counter or so, and kill per-cpu iowait counter which is known to being nonsense. It will take long time to be achieved, considering time to make the problem to a public knowledge, and also time for interface transition. Anyway as long as linux try to be reliable OS, such drastic change must be placed on mainline kernel in development sooner or later.
OTOH, drastic change like above is not acceptable for conservative environment, such as stable kernels, some distributor's kernel and so on, due to respect for compatibility. Still these kernel expects per-cpu iowait counter works as well as it might have been. Therefore for such environment, applying a simple patch to fix behavior of counter will be appreciated rather than replacing an over-used interface or changing an existing usage/semantics.
This patch targets latter kernels mainly. It does not do too much, but intend to fix the idle stats counters to be monotonic. I believe that mainline kernel should pick up this patch too, because it surely fix a hidden bug and does not intercept upcoming drastic change.
Again, in summary, this patch does not do drastic change to cope with problem 2. But it just fix behavior of idle/iowait counter of
/proc/stats.[WHAT THIS PATCH PROPOSED]:
The main reason of the bug is that observers have no idea to determine the delta to be idle or iowait at the first place.
So I introduced delayed iowait accounting to allow observers to assign delta based on status of observed cpu at idle entry.
Quoted 6 lines
Quoted 2 lines
So the problem I have with this is that it makes
CONFIG_NOHZ=[ny]kernels behave quite differently.It is not true.
There are already differences before applying my patches. The behavior of NOHZ=y kernel diverged from original since it was born.
But if you argue about not actually fixing iowait properly, then this difference is the only actual regression.
Quoted 1 line
Please note that no one complained about this difference.
Then why are you working on this? You're the one that said there was a regression between unnamed enterprise distro 5 and unnamed enterprise distro 6.
Quoted 2 lines
I just want to fix a counter not to go backward.
It's a simple bug, isn't it?
Seeing how we managed to send as many patches as we did, I'd say that's a fairly big clue as to how its not as simple.
Just make the value unconditionally 0 then. That's guaranteed not to go backwards and just about as useful as the random fwd walk you make it. Plus, its a lot easier.
Quoted 9 lines
Quoted 3 lines
Ideally NOHZ=y and NOHZ=n behave the same, my proposed solution the other day does just that. This one OTOH seems to generate entirely different results between those kernels.
As you know, behaviors of NOHZ=[ny] are both crap because of per-cpu iowait accounting.
I guess we should say:
Ideally NOHZ=[ny] behave the same "in the proper way."
No, the premise of NOHZ is that behaviour should not change, we found a change in behaviour, we should make it go away.
Secondly, with or without NOHZ iowait accounting is complete crap. We should also fix that.
Quoted 2 lines
What you proposed will do too much to make one nonsense to another nonsense. It will be unhelpful for people...
I proposed that if you don't want to fix the actual iowait is crap problem, you at least fix the NOHZ regression proper.
Quoted 5 lines
Quoted 2 lines
It also doesn't really simplify the code; there's quite a bit of complexity introduced to paper over this silly stuff.
Don't complicate things. I want to talk about a small simple bug.
Quoted 7 lines
a) today's NOHZ=n
- provides per-cpu iowait counter
(nonsense but still loved by innocent userland)
b) today's NOHZ=y
- provides per-cpu iowait counter
- use it's own idle time accounting different from a)
- have a *bug* that counter might go backward
Like said, the intent of NOHZ is to not change accounting, its often more complicated from a because well, no ticks.
But it really should give the same number, nonsense or not. We do the same for all other numbers -- we spend a ton of effort to fix the loadavg a year or two ago.
loadavg is also another dubious number, fwiw.
Quoted 11 lines
b') NOHZ=y + my patch
- provides per-cpu iowait counter
- use it's own idle time accounting same as b)
- *bug* in b) have gone
- instead accept gap in iowait value from b)
- "pending" will not bloat more than one iowait spanc) unified something for NOHZ=[ny] (you proposed)
- provides per-cpu iowait counter
- use new accounting different from a) and also b)
Quoted 5 lines
d) ideal goal (=not designed & realized yet)
- no per-cpu iowait counter any more
- use new proper accounting different from all of aboveI just want to make b) to b') by a patch as small as possible.
I really don't see the value of b', the actual value of its result is really no better than the constant 0, and the constant implementation is _way_ simpler.
Quoted 8 lines
What you proposed will make both of a) and b) to c). I think it does too much and changing a) is not required here. (from my conservative perspective, patch must be non-intrusive)
Our final goal must be making d) to replace all nasty things around there. But still there is no idea to do that. And such big jump will not fit to stable environments.
Quoted 6 lines
Again, I just want to fix a small bug here...
I believe my patch is enough simple to do a minimum fix. Please tell me if you have more simple/better way to fix this long-standing minor bug, not only for mainline in development but also for conservative stables like distributor's kernel.
I really think only fixing the backward motion is retarded. Its papering over so much crap its not funny.
The fact that it does go backwards is because b does not give the identical results from a, _that_ is a bug per the design principle of NOHZ.
Fixing it to just return some random number that doesn't go backwards doesn't fix it. It just makes the immediately observed problem go away; entirely the wrong mindset.
Re: [PATCH 2/2] nohz: delayed iowait accounting for nohz idle time stats
Quoted 44 lines · Peter Zijlstra
Quoted 43 lines · Hidetoshi Seto
Quoted 41 lines · Peter Zijlstra
Quoted 29 lines · Hidetoshi Seto
[TARGET OF THIS PATCH]:
Complete rework for iowait accounting implies that some user interfaces might be replaced completely. It will introduce new counter or so, and kill per-cpu iowait counter which is known to being nonsense. It will take long time to be achieved, considering time to make the problem to a public knowledge, and also time for interface transition. Anyway as long as linux try to be reliable OS, such drastic change must be placed on mainline kernel in development sooner or later.
OTOH, drastic change like above is not acceptable for conservative environment, such as stable kernels, some distributor's kernel and so on, due to respect for compatibility. Still these kernel expects per-cpu iowait counter works as well as it might have been. Therefore for such environment, applying a simple patch to fix behavior of counter will be appreciated rather than replacing an over-used interface or changing an existing usage/semantics.
This patch targets latter kernels mainly. It does not do too much, but intend to fix the idle stats counters to be monotonic. I believe that mainline kernel should pick up this patch too, because it surely fix a hidden bug and does not intercept upcoming drastic change.
Again, in summary, this patch does not do drastic change to cope with problem 2. But it just fix behavior of idle/iowait counter of
/proc/stats.Quoted 8 lines
[WHAT THIS PATCH PROPOSED]:
The main reason of the bug is that observers have no idea to determine the delta to be idle or iowait at the first place.
So I introduced delayed iowait accounting to allow observers to assign delta based on status of observed cpu at idle entry.
So the problem I have with this is that it makes
CONFIG_NOHZ=[ny]kernels behave quite differently.
Quoted 6 lines
Quoted 3 lines
It is not true.
There are already differences before applying my patches. The behavior of NOHZ=y kernel diverged from original since it was born.But if you argue about not actually fixing iowait properly, then this difference is the only actual regression.
A difference is not always a regression.
Quoted 5 lines
Quoted 1 line
Please note that no one complained about this difference.
Then why are you working on this? You're the one that said there was a regression between unnamed enterprise distro 5 and unnamed enterprise distro 6.
e.g.
regression: a counter loses monotonicity
improvement: drop power consumption by skipping tick during in idle not matter: useless fuzzy crap value turned to be another crap
If you say every difference is regression, then all kernel config must be nop.
Quoted 5 lines
Quoted 2 lines
I just want to fix a counter not to go backward.
It's a simple bug, isn't it?Seeing how we managed to send as many patches as we did, I'd say that's a fairly big clue as to how its not as simple.
I think the situation is that a simple small bug is glued to big complicated background. I want to separate them and handle only a small bug, therefore I wrote patch description a lot for background and my target.
Quoted 3 lines
Just make the value unconditionally 0 then. That's guaranteed not to go backwards and just about as useful as the random fwd walk you make it. Plus, its a lot easier.
I'd like to hire constant 0 if it is acceptable for stables.
I suppose it could be considered as improvement for mainline kernel since it will be the first step for upcoming drastic changes. But I also suppose it could be classified as regression for stable kernels because one counter ordinarily used stops its progress.
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Ideally NOHZ=y and NOHZ=n behave the same, my proposed solution the other day does just that. This one OTOH seems to generate entirely different results between those kernels.
As you know, behaviors of NOHZ=[ny] are both crap because of per-cpu iowait accounting.
Quoted 8 lines
Quoted 2 lines
I guess we should say:
Ideally NOHZ=[ny] behave the same "in the proper way."No, the premise of NOHZ is that behaviour should not change, we found a change in behaviour, we should make it go away.
Secondly, with or without NOHZ iowait accounting is complete crap. We should also fix that.
Humm..? I could not catch the reason why you say no here. I think wasting time to unify 2 craps into 1 crap is not necessary before replacing craps by a proper thing.
Quoted 5 lines
Quoted 2 lines
What you proposed will do too much to make one nonsense to another nonsense. It will be unhelpful for people...
I proposed that if you don't want to fix the actual iowait is crap problem, you at least fix the NOHZ regression proper.
I'd like to target only a part of differences which is considered as regression.
I'm being overly cautious since removing or stopping this crap counter could be new regression.
Quoted 13 lines
Quoted 5 lines
Quoted 2 lines
It also doesn't really simplify the code; there's quite a bit of complexity introduced to paper over this silly stuff.
Don't complicate things. I want to talk about a small simple bug.
Quoted 7 lines
a) today's NOHZ=n
- provides per-cpu iowait counter
(nonsense but still loved by innocent userland)
b) today's NOHZ=y
- provides per-cpu iowait counter
- use it's own idle time accounting different from a)
- have a *bug* that counter might go backward
Quoted 8 lines
Like said, the intent of NOHZ is to not change accounting, its often more complicated from a because well, no ticks.
But it really should give the same number, nonsense or not. We do the same for all other numbers -- we spend a ton of effort to fix the loadavg a year or two ago.
loadavg is also another dubious number, fwiw.
I can understand how you feel.
Quoted 15 lines
Quoted 15 lines
b') NOHZ=y + my patch
- provides per-cpu iowait counter
- use it's own idle time accounting same as b)
- *bug* in b) have gone
- instead accept gap in iowait value from b)
- "pending" will not bloat more than one iowait spanc) unified something for NOHZ=[ny] (you proposed)
- provides per-cpu iowait counter
- use new accounting different from a) and also b)d) ideal goal (=not designed & realized yet)
- no per-cpu iowait counter any more
- use new proper accounting different from all of above
Quoted 5 lines
Quoted 1 line
I just want to make b) to b') by a patch as small as possible.
I really don't see the value of b', the actual value of its result is really no better than the constant 0, and the constant implementation is _way_ simpler.
a) monotonic per-cpu counter providing useless value
b) non-monotonic per-cpu counter providing useless value b') monotonic per-cpu counter providing useless value
c) monotonic per-cpu counter providing useless value
d) monotonic counter(s) providing proper value
x) constant 0 counter
My evaluation is:
for stables:
(bad) (b,d,x) <<<<< (a,b',c) (good)
for mainline:
(bad) b <<<<< (a,b',c) <<<<< x <<<<< d (good)
Quoted 18 lines
Quoted 8 lines
What you proposed will make both of a) and b) to c). I think it does too much and changing a) is not required here. (from my conservative perspective, patch must be non-intrusive)
Our final goal must be making d) to replace all nasty things around there. But still there is no idea to do that. And such big jump will not fit to stable environments.
Quoted 6 lines
Again, I just want to fix a small bug here...
I believe my patch is enough simple to do a minimum fix. Please tell me if you have more simple/better way to fix this long-standing minor bug, not only for mainline in development but also for conservative stables like distributor's kernel.
I really think only fixing the backward motion is retarded. Its papering over so much crap its not funny.
Quoted 7 lines
The fact that it does go backwards is because b does not give the identical results from a, _that_ is a bug per the design principle of NOHZ.
Fixing it to just return some random number that doesn't go backwards doesn't fix it. It just makes the immediately observed problem go away; entirely the wrong mindset.
I feel like that you are going to use cardboards instead of papers...
BTW, I found that Denys posted his updated patch set:
https://lkml.org/lkml/2014/4/23/579
I think it looks like taking almost same approach as what you proposed. However I guess:
- It should not have #ifdef CONFIG_NOHZ_* because it make differences between behavior of NOHZ=[ny].
- last_iowait need to be located in rq rather than in struct tick_sched, considering cache hit at io_schedule*().
- last_iowait need to be referred in tick for NOHZ=n, not to make difference from NOHZ=y.
Anyway, though I still wonder what you proposed is acceptable fix for distro's quality assurance, I hope someone in charge may have broad mind like you and may resolve the matter favorably.
Is it worth to do if I make v5 based on your proposal and post it for review comparing with v4?
Thanks,
H.Seto
Ping?
(I'll have a week holidays from next week.
So thank you if you could give me your comments soon!)
Thanks,
H.Seto
Quoted 29 lines · Hidetoshi Seto
Hi all,
This patch set (rebased on v3.15-rc1) is my 4th try to fix an issue that idle/iowait of
/proc/statcan go backward. Originally reported by Tetsuo and Fernando at last year, Mar 2013.This v4 updates v3's approach, and adds description/comments to make patch's intent to be clear (I hope so).
Of course still reviews are welcome.
Thanks,
H.SetoHidetoshi Seto (2):
nohz: make updating sleep stats local
nohz: delayed iowait accounting for nohz idle time statsinclude/linux/tick.h | 6 +- kernel/time/tick-sched.c | 179 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 136 insertions(+), 49 deletions(-)--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/