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

[Ingo Molnar]

* John Stultz <john.stultz@xxxxxxxxxx> wrote:

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

The ->clock pointer is unused as well AFAICS [we pass in 
'offset'] - and that together with the lock is already two 
fields. It's 8 bytes copied back and forth unnecessarily, 
amongst other things.

But yes, moving those two fields out is an equivalent solution 
too - although I do agree with the whole clean-up direction of 
going away from standalone global variables and collecting those 
fields into a single structure. It's your call which one you 
prefer - but mixing the two types does not look clean to me.

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

Moving the lock and any other field not used internally out of 
it is fine as well - plus not using a global shadow_copy but 
making it local to update_wall_time().

Thanks,

	Ingo
--
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/