Re: [PATCH 4/9] time: Update timekeeper structure using a localshadow

[John Stultz]

On Fri, 2012-03-02 at 08:38 +0100, Ingo Molnar wrote:
> * John Stultz <john.stultz@xxxxxxxxxx> wrote:
> 
> > 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.

So I actually removed the locks out from the timekeeper structure to try
to address this concern.

> 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()?

Just about all of timekeeper state is used and modified in the
update_wall_time. 

> 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.)

So, just to be clear, you want me to push basically everything in the
timekeeper structure, except the lock (which would be re-added), into a
time_state sub-structure?

Sorry for being dense here (its been a long day), but maybe could you
clarify a bit more about the differences you're describing between
time-state and global-state wrt the timekeeper?

thanks
-john

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/