From: Kevin Hilman <hidden> Date: 2012-02-03 18:41:01
Rob Herring [off-list ref] writes:
On 02/02/2012 04:20 PM, Kevin Hilman wrote:
quoted
"Paul E. McKenney" [off-list ref] writes:
[...]
quoted
quoted
quoted
The two options I see are:
1. Rip tracing out of the inner idle loops and everything that
they invoke.
What I suggested above. But as I said I know sh*t about that tracing
implementation so that's an easy suggestion for me to make.
Works for me as well. ;-)
While I must admit not having a better suggestion, I for one would vote
strongly against removing tracing from the idle path.
Being a PM developer and maintainer, much of the code I work on and
maintain happens to be run in the bowels of the idle path. Not having
the ability to trace this code would be a major step backwards IMO.
How is it a step backwards if it is already broken.
Well, I didn't know it was broken. ;) And, as Paul mentioned, this has
been broken for a long time. Apparently it's been working well enough
for nobody to notice until recently.
Obviously you haven't actually used any tracing here because it
doesn't work right with things as is.
It's been working well enough for me to debug several idle path problems
with tracing. Admittedly, this has been primarily on UP systems, but
I've recently started using the tracing on SMP as well. (however, due
to "coupled" low-power states on OMAP, large parts of the idle path are
effectively UP since one CPU0 has to wait for CPU1 to hit a low-power
state before it can.)
What exactly do you want to trace at this level. By the point you are in
this code, the path is somewhat known and problems you have are likely
h/w issues.
Not really.
There is still quite a bit of software between the decision to enter
idle and the hardware taking over. On OMAP for example, we have power
domains, clock domains and clocks that are managed during idle, and
these layers contain tracepoints.
Add to that the runtime PM management of some devices that are coupled
to the CPU (because they share a power domain, etc). Runtime PM
contains tracepoints.
Add to that possible voltage scaling during idle using regulators.
Regulator framework has tracepoints.
That can lead to quite a bit of tracing info *after* the decision to
enter idle.
If you are trying to go thru a very precise sequence of
saving cpu state and flushing caches, you don't want calls out to
tracing code that could very easily change the behavior.
I'm more worried about the power domain and voltage domain transitions
(or lack thereof) when trying to debug why a particular low-power state
was not hit.
Kevin
From: Paul E. McKenney <hidden> Date: 2012-02-03 19:26:18
On Fri, Feb 03, 2012 at 10:41:01AM -0800, Kevin Hilman wrote:
Rob Herring [off-list ref] writes:
quoted
On 02/02/2012 04:20 PM, Kevin Hilman wrote:
quoted
"Paul E. McKenney" [off-list ref] writes:
[...]
quoted
quoted
quoted
The two options I see are:
1. Rip tracing out of the inner idle loops and everything that
they invoke.
What I suggested above. But as I said I know sh*t about that tracing
implementation so that's an easy suggestion for me to make.
Works for me as well. ;-)
While I must admit not having a better suggestion, I for one would vote
strongly against removing tracing from the idle path.
Being a PM developer and maintainer, much of the code I work on and
maintain happens to be run in the bowels of the idle path. Not having
the ability to trace this code would be a major step backwards IMO.
How is it a step backwards if it is already broken.
Well, I didn't know it was broken. ;) And, as Paul mentioned, this has
been broken for a long time. Apparently it's been working well enough
for nobody to notice until recently.
Yep. The probability is quite low, but the consequences are dire.
We might well have hit it occasionally -- it would be a random
inexplicable crash.
Thanx, Paul
quoted
Obviously you haven't actually used any tracing here because it
doesn't work right with things as is.
It's been working well enough for me to debug several idle path problems
with tracing. Admittedly, this has been primarily on UP systems, but
I've recently started using the tracing on SMP as well. (however, due
to "coupled" low-power states on OMAP, large parts of the idle path are
effectively UP since one CPU0 has to wait for CPU1 to hit a low-power
state before it can.)
quoted
What exactly do you want to trace at this level. By the point you are in
this code, the path is somewhat known and problems you have are likely
h/w issues.
Not really.
There is still quite a bit of software between the decision to enter
idle and the hardware taking over. On OMAP for example, we have power
domains, clock domains and clocks that are managed during idle, and
these layers contain tracepoints.
Add to that the runtime PM management of some devices that are coupled
to the CPU (because they share a power domain, etc). Runtime PM
contains tracepoints.
Add to that possible voltage scaling during idle using regulators.
Regulator framework has tracepoints.
That can lead to quite a bit of tracing info *after* the decision to
enter idle.
quoted
If you are trying to go thru a very precise sequence of
saving cpu state and flushing caches, you don't want calls out to
tracing code that could very easily change the behavior.
I'm more worried about the power domain and voltage domain transitions
(or lack thereof) when trying to debug why a particular low-power state
was not hit.
Kevin
On Fri, 2012-02-03 at 10:41 -0800, Kevin Hilman wrote:
quoted
How is it a step backwards if it is already broken.
Well, I didn't know it was broken. ;) And, as Paul mentioned, this has
been broken for a long time. Apparently it's been working well enough
for nobody to notice until recently.
quoted
Obviously you haven't actually used any tracing here because it
doesn't work right with things as is.
It's been working well enough for me to debug several idle path problems
with tracing. Admittedly, this has been primarily on UP systems, but
I've recently started using the tracing on SMP as well. (however, due
to "coupled" low-power states on OMAP, large parts of the idle path are
effectively UP since one CPU0 has to wait for CPU1 to hit a low-power
state before it can.)
It's used by all users of powertop, and we haven't heard about a bug
yet. This doesn't mean that the bug doesn't exist. The race is extremely
hard to hit. It's one of those "good bugs". You know, the kind that you
don't really have to worry about because you are more likely to win the
lottery, become President of the United States, and find a cure for
cancer (all those together, not just one) than the chance of hitting
this bug. But it's a bug regardless and should, unfortunately, be fixed.
But here's the explanation of the bug:
As Paul has stated, when rcu_idle_enter() is in effect, the calls to
rcu_read_lock_* are ignored. Thus we can pretend they don't exist.
The code in question is the __DO_TRACE() in include/linux/tracepoint.h:
rcu_read_lock_sched_notrace(); \
it_func_ptr = rcu_dereference_sched((tp)->funcs); \
if (it_func_ptr) { \
do { \
it_func = (it_func_ptr)->func; \
__data = (it_func_ptr)->data; \
((void(*)(proto))(it_func))(args); \
} while ((++it_func_ptr)->func); \
} \
rcu_read_unlock_sched_notrace();
As stated above, the rcu_read_(un)lock_sched_notrace() are worthless
when in rcu_idle_enter().
They protect the referencing of tp->funcs, which is an array of all
funcs that are attached to this tracepoint.
Now we need to look at kernel/tracepoint.c:
The protection is needed against a simultaneous insertion or deletion of
a tracepoint hook. This happens when a user enables or disables tracing.
Note, this race is even made harder to hit, because due to the static
branch that controls whether this gets called, will be off if no
tracepoints are attached. So the race can only happen after at least one
tracepoint is active.
But if two probes are are added to this tracepoint, then we can hit the
race. And it is possible to trigger with only one probe on removal.
When adding or removing a tracepoint, the array (the one that
it_func_ptr points to) is updated by allocating a new array, copying the
old array plus or minus the tracpoint being added or removed, setting
the tp->funcs to the new array, and then it calls call_rcu_sched() to
free it.
Now for the bug to hit, something had to be coming in or out of idle,
and jumping to this code. Between the time it got the it_func_ptr to the
time it accessed any of that pointer's data in the loop, the tp->func
had to be updated to the new array, and then all CPUs would have passed
a schedule point (except the rcu_idle CPUs).
On uniprocessor, this is not an issue, but on SMP, it is possible that
with two CPUs the first being in rcu_idle may be ignored, and the second
would have been adding the tracepoint and then going directly to freeing
the code. But as tracepoints are very low weight, it is most likely that
the tracepoints will finish before the first could even free the memory.
But the chance does exist. As the chance of me winning the lottery,
becoming President of the United States, and curing cancer also exists!
;-)
-- Steve
From: Paul E. McKenney <hidden> Date: 2012-02-04 14:21:23
On Fri, Feb 03, 2012 at 02:36:27PM -0500, Steven Rostedt wrote:
On Fri, 2012-02-03 at 10:41 -0800, Kevin Hilman wrote:
quoted
quoted
How is it a step backwards if it is already broken.
Well, I didn't know it was broken. ;) And, as Paul mentioned, this has
been broken for a long time. Apparently it's been working well enough
for nobody to notice until recently.
quoted
Obviously you haven't actually used any tracing here because it
doesn't work right with things as is.
It's been working well enough for me to debug several idle path problems
with tracing. Admittedly, this has been primarily on UP systems, but
I've recently started using the tracing on SMP as well. (however, due
to "coupled" low-power states on OMAP, large parts of the idle path are
effectively UP since one CPU0 has to wait for CPU1 to hit a low-power
state before it can.)
It's used by all users of powertop, and we haven't heard about a bug
yet. This doesn't mean that the bug doesn't exist. The race is extremely
hard to hit. It's one of those "good bugs". You know, the kind that you
don't really have to worry about because you are more likely to win the
lottery, become President of the United States, and find a cure for
cancer (all those together, not just one) than the chance of hitting
this bug. But it's a bug regardless and should, unfortunately, be fixed.
But here's the explanation of the bug:
As Paul has stated, when rcu_idle_enter() is in effect, the calls to
rcu_read_lock_* are ignored. Thus we can pretend they don't exist.
The code in question is the __DO_TRACE() in include/linux/tracepoint.h:
rcu_read_lock_sched_notrace(); \
it_func_ptr = rcu_dereference_sched((tp)->funcs); \
if (it_func_ptr) { \
do { \
it_func = (it_func_ptr)->func; \
__data = (it_func_ptr)->data; \
((void(*)(proto))(it_func))(args); \
} while ((++it_func_ptr)->func); \
} \
rcu_read_unlock_sched_notrace();
As stated above, the rcu_read_(un)lock_sched_notrace() are worthless
when in rcu_idle_enter().
They protect the referencing of tp->funcs, which is an array of all
funcs that are attached to this tracepoint.
Now we need to look at kernel/tracepoint.c:
The protection is needed against a simultaneous insertion or deletion of
a tracepoint hook. This happens when a user enables or disables tracing.
Note, this race is even made harder to hit, because due to the static
branch that controls whether this gets called, will be off if no
tracepoints are attached. So the race can only happen after at least one
tracepoint is active.
I agree that this race is hard to hit when running Linux on bare metal.
But consider a Linux kernel running as a guest OS. Then the host might
preempt the guest in the middle of a tracepoint. Then from the guest OS's
viewpoint, that VCPU has just stopped, possibly for a very long time --
easily long enough for all the other VCPUs to pass through quiescent
states. And the guest OS is ignoring that VCPU, so a too-short grace
period could easily happen in this scenario.
Thanx, Paul
But if two probes are are added to this tracepoint, then we can hit the
race. And it is possible to trigger with only one probe on removal.
When adding or removing a tracepoint, the array (the one that
it_func_ptr points to) is updated by allocating a new array, copying the
old array plus or minus the tracpoint being added or removed, setting
the tp->funcs to the new array, and then it calls call_rcu_sched() to
free it.
Now for the bug to hit, something had to be coming in or out of idle,
and jumping to this code. Between the time it got the it_func_ptr to the
time it accessed any of that pointer's data in the loop, the tp->func
had to be updated to the new array, and then all CPUs would have passed
a schedule point (except the rcu_idle CPUs).
On uniprocessor, this is not an issue, but on SMP, it is possible that
with two CPUs the first being in rcu_idle may be ignored, and the second
would have been adding the tracepoint and then going directly to freeing
the code. But as tracepoints are very low weight, it is most likely that
the tracepoints will finish before the first could even free the memory.
But the chance does exist. As the chance of me winning the lottery,
becoming President of the United States, and curing cancer also exists!
;-)
-- Steve
On Sat, 2012-02-04 at 06:21 -0800, Paul E. McKenney wrote:
On Fri, Feb 03, 2012 at 02:36:27PM -0500, Steven Rostedt wrote:
quoted
Note, this race is even made harder to hit, because due to the static
branch that controls whether this gets called, will be off if no
tracepoints are attached. So the race can only happen after at least one
tracepoint is active.
I agree that this race is hard to hit when running Linux on bare metal.
But consider a Linux kernel running as a guest OS. Then the host might
preempt the guest in the middle of a tracepoint. Then from the guest OS's
viewpoint, that VCPU has just stopped, possibly for a very long time --
easily long enough for all the other VCPUs to pass through quiescent
states. And the guest OS is ignoring that VCPU, so a too-short grace
period could easily happen in this scenario.
But there's one thing that you forget. The race only happens on adding
or removing of the tracepoint, which requires human intervention.
That is, this gap needs to exist when a human starts or stops tracing on
the guest. For the race to occur, the one guest CPU has to preempt at
that exact location, and then be busy doing other things as a user on
the guest enables or disables tracing. For the enabled part, something
else had to already be tracing that same tracepoint (which seldom
happens, and only if the users chooses to (root user)). Otherwise, the
race only exists on removing the tracepoint.
OK, I'll update statement for running this on a guest. It is less likely
to trigger than me winning the lottery, becoming president of the United
states, *or* curing cancer. Not all together, just one of the above ;-)
-- Steve