Messages in this thread | | | Date | Tue, 28 Feb 2012 09:12:22 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH 4/7] time: Update timekeeper structure using a local shadow |
| |
* John Stultz <john.stultz@linaro.org> wrote:
> static void update_wall_time(void) > { > struct clocksource *clock; > + struct timekeeper tk; > cycle_t offset; > int shift = 0, maxshift; > unsigned long flags; > @@ -1063,10 +1064,11 @@ static void update_wall_time(void) > if (unlikely(timekeeping_suspended)) > goto out; > > - clock = timekeeper.clock; > + tk = timekeeper;
'tk' is now an on-stack copy of a very non-trivial (read: large) structure - including locks, amongst other things. That's a no-no.
> + timekeeper = tk;
You just broke lockdep here.
It's also ugly code: global data structures should almost always be updated in situ, instead of updating a local copy and then copying it back blindly ...
Thanks,
Ingo
| |