Patch 2.5.25: Ensure xtime_lock and timerlist_lock are on difft cachelines

I've noticed that xtime_lock and timerlist_lock ends up on the same cacheline all the time (atleaset on x86). Not a good thing for loads with high xxx_timer and do_gettimeofday counts I guess (networking etc).

Here's a 2.5.25 objdump:

c0302780 g     O .data  00000004 time_freq
c0302784 g     O .data  00000004 timerlist_lock
c0302788 g     O .data  00000004 tqueue_lock
c030278c g     O .data  00000004 xtime_lock
c0302790 l     O .data  00000004 count.3
c0302794 l     O .data  00000004 uidhash_lock
c0302798 g     O .data  00000018 root_user

Here's a trivial 2.5.28 based fix.

-Kiran

diff -ruN -X dontdiff linux-2.5.28/kernel/timer.c align_locks/kernel/timer.c
@@ -169,7 +169,7 @@
 }
 
 /* Initialize both explicitly - let's try to have them in the same cache line */
-spinlock_t timerlist_lock = SPIN_LOCK_UNLOCKED;
+spinlock_t timerlist_lock ____cacheline_aligned_in_smp = SPIN_LOCK_UNLOCKED;
 
 #ifdef CONFIG_SMP
 volatile struct timer_list * volatile running_timer;

@@ -327,7 +327,7 @@
 	spin_unlock_irq(&timerlist_lock);
 }
 
-spinlock_t tqueue_lock = SPIN_LOCK_UNLOCKED;
+spinlock_t tqueue_lock __cacheline_aligned_in_smp = SPIN_LOCK_UNLOCKED;
 
 void tqueue_bh(void)
 {

@@ -633,7 +633,7 @@
  * This read-write spinlock protects us from races in SMP while
  * playing with xtime and avenrun.
  */
-rwlock_t xtime_lock = RW_LOCK_UNLOCKED;
+rwlock_t xtime_lock __cacheline_aligned_in_smp = RW_LOCK_UNLOCKED;
 unsigned long last_time_offset;
 
 static inline void update_times(void)

In message <[email protected]> you write:

..

Better might be to use the x86-64 trick of using sequence counters around do_gettimeofday, and avoid the xtime lock altogether. That will improve gettimeofday performance as well. Or you could try changing xtime lock to a brlock.

FYI: as policy, I don't take optimization patches without measurements. I'm just not that smart.

Thanks,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Ok, I'll look at the x86-64 code

This patch was not meant to be a definitive fix for do_gettimeofday. I thought having diffrent locks on the same cacheline was bad. Atleast, I don't think there'd be any negative performance impact due to my patch. Pls correct me if I am wrong.

I want to get some nos too .. and probably will...(still waiting for my turn to use the 4way here :-) ). But, I decided to post this patch as a follow up to the 2.5 profiler discussion on lse-tech. Anywayz, point taken. Next time I submit an optimization patch to you, I'll post the measuements too.

Thanks,
Kiran

In message <[email protected]> you write:

Did you ever wonder why we don't declare spinlock to be ____cacheline_aligned? While it's probably justified in this case, you pay for it in a slight increase in size...

Sure!

Thanks,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Yep...and for long enough...(I think..)..or there'd have been an RFC or a stupid question to lkml sometime from me :)

I thought you were of the opinion that "memory is cheap" ;-)

-Kiran

In message <[email protected]> you write:

Absolutely: if you're trading it for programming time.

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.