Re: [PATCH] simplify update_times (avoid jiffies/jiffies_64 aliasing problem)

[Andrew Morton <akpm@osdl.org>]

Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
>
> akpm> I'm not sure how to resolve this, really.  Worried.  Have you
> akpm> socialised those changes with architecture maintainers?  If so,
> akpm> what was the feedback?
> 
> For a short term fix, barrier() might be a safe option.
> 
> 
> Add an optimization barrier to prevent prefetching jiffies before
> incrementing jiffies_64.
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> 
> diff --git a/kernel/timer.c b/kernel/timer.c
> index fc6646f..fdd12a5 100644
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -925,6 +925,7 @@ static inline void update_times(void)
>  void do_timer(struct pt_regs *regs)
>  {
>  	jiffies_64++;
> +	barrier();
>  	update_times();
>  	softlockup_tick(regs);
>  }

a) Please never ever add any form of barrier without adding a comment
   explaining why it's there.  Unless it's extremely obvious (and it rarely
   is), it's hard for the reader to work out what on earth it's doing
   there.

   Example?  Take a look at the smp_rmb() in ext3_get_inode_block(),
   work out why it's there.  Time yourself.

b) On 64-bit machines jiffies and jiffies_64 always have the same
   address (don't they?) Is the compiler really going to move a read of an
   absolute address ahead of a modification of the same address?

   <looks>

   The address of jiffies isn't known until link time, so yup, the risk
   is there.

c) jiffies is declared volatile.  In practice, if I know my gcc, it's
   just not going to play these reordering games with a volatile.

   If that's true, and if some standard (presumably c99) says that it
   must be true then I don't think we need the patch.