Messages in this thread |  | | | Date | Fri, 2 Mar 2012 08:38:48 +0100 | | From | Ingo Molnar <> | | Subject | Re: [PATCH 4/9] time: Update timekeeper structure using a local shadow |
| |
* John Stultz <john.stultz@linaro.org> wrote:
> Uses a local shadow structure to update the timekeeper. This > will allow for reduced timekeeper.rlock hold time. > > CC: Ingo Molnar <mingo@elte.hu> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: Eric Dumazet <eric.dumazet@gmail.com> > CC: Richard Cochran <richardcochran@gmail.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > kernel/time/timekeeping.c | 43 +++++++++++++++++++++++-------------------- > 1 files changed, 23 insertions(+), 20 deletions(-) > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > index f9ee96c..09460c1 100644 > --- a/kernel/time/timekeeping.c > +++ b/kernel/time/timekeeping.c > @@ -74,6 +74,7 @@ struct timekeeper { > }; > > static struct timekeeper timekeeper; > +static struct timekeeper shadow_tk;
Sigh.
As I said it in the first round of review, it's fundamentally wrong to copy live fields like locks or the clocksource pointer around.
It's doubly wrong to do it in a global variable that no-one else but the copying function (update_wall_time()) is supposed to access.
There are over a dozen fields in 'struct timekeeper' - exactly which ones of them are used on this private copy, as update_wall_time() does the cycle accumulation and calls down into timkeeping_adjust()?
The right solution would be to separate timekeeping time state from global state:
struct timekeeper { spinlock_t lock; struct time_state time_state; }; And then standardize the time calculation code on passing around not 'struct timekeeper *' but 'struct time_state *' ! Then you can have a local shadow copy of the global state:
struct time_state time_state_copy;
and copy it from the global one and then pass it down to calculation functions.
This also gives the freedom to add other global state fields beyond the lock. (Right now the lock appears to be the only global state field - there might be more.)
Thanks,
Ingo
|  |