lkml.org 
[lkml]   [2012]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2012-03-02 08:41    [from the cache]
©2003-2011 Jasper Spaans