Messages in this thread Patch in this message | | | Date | Fri, 07 Jul 2000 22:54:24 +1000 | From | Andrew Morton <> | Subject | Re: [PATCH] Minor timer.c cleanup test3-pre2 |
| |
Rusty Russell wrote: > > In message <39654DE5.389BEFB@uow.edu.au> you write: > > > sync_timers() -> finish all timers currently running (this can be > > > trivially implemented as spin_lock_wait(&global_bh_lock). > > > > Where does the requirement for sync_timers() appear? How would it be used? > > module unload, where module count is decremented by timer; timer must > be finished. Also: `if (!del_timer(timer)) sync_timers();' == > del_timer_sync() for self-destructing timers.
OK, that makes sense. It's one of the few sane ways of doing this.
How does the attached hastily-thrown-together-not-doing-timers-at-present patch look?
Notes:
- We really should get rid of the eighteen trillion RUN_AT macros and turn them into real functions - add_timer_at(), mod_timer_at(). Do the addition to `jiffies' in a single place.
- The test3-pre4 implementation has a potential bug:
volatile struct timer_list * running_timer;
should have been
struct timer_list * volatile running_timer;
Fortunately gcc throws its hands up and runs away as soon as it gets a whiff of `volatile'.
- The following architectures don't provide spin_unlock_wait:
* asm-arm * asm-m68k * asm-mips * asm-sh
And hence won't compile with the attached patch and CONFIG_SMP. It appears that these architectures don't support SMP anyway.
BTW: how do we make decisions about inlining things? I often see what appear to be dubious inlining decisions. For example, internal_add_timer(). That's a BIG function. Is there a net benefit in having up to three copies sitting in the icache?--- linux-2.4.0-test3-pre4/include/linux/timer.h Thu Jul 6 20:23:47 2000 +++ linux-akpm/include/linux/timer.h Fri Jul 7 22:25:48 2000 @@ -24,14 +24,23 @@ void (*function)(unsigned long); }; -extern volatile struct timer_list *running_timer; extern void add_timer(struct timer_list * timer); extern int del_timer(struct timer_list * timer); +#ifdef CONFIG_SMP +extern int del_timer_sync(struct timer_list * timer); +extern void sync_timers(void); +#else +#define del_timer_sync(t) del_timer(t) +#define sync_timers() do { } while (0) +#endif + /* * mod_timer is a more efficient way to update the expire field of an * active timer (if the timer is inactive it will be activated) - * mod_timer(a,b) is equivalent to del_timer(a); a->expires = b; add_timer(a) + * mod_timer(a,b) is equivalent to del_timer(a); a->expires = b; add_timer(a). + * If the timer is known to be not pending (ie, in the handler), mod_timer + * is less efficient than a->expires = b; add_timer(a). */ int mod_timer(struct timer_list *timer, unsigned long expires); @@ -46,20 +55,6 @@ { return timer->list.next != NULL; } - -#ifdef CONFIG_SMP -#define timer_enter(t) do { running_timer = t; mb(); } while (0) -#define timer_exit() do { running_timer = NULL; } while (0) -#define timer_is_running(t) (running_timer == t) -#define timer_synchronize(t) while (timer_is_running(t)) barrier() -extern int del_timer_sync(struct timer_list * timer); -#else -#define timer_enter(t) do { } while (0) -#define timer_exit() do { } while (0) -#define timer_is_running(t) (0) -#define timer_synchronize(t) do { (void)(t); barrier(); } while(0) -#define del_timer_sync(t) del_timer(t) -#endif /* * These inlines deal with timer wrapping correctly. You are --- linux-2.4.0-test3-pre4/kernel/timer.c Thu Jul 6 20:23:47 2000 +++ linux-akpm/kernel/timer.c Fri Jul 7 22:36:59 2000 @@ -162,7 +162,19 @@ /* Initialize both explicitly - let's try to have them in the same cache line */ spinlock_t timerlist_lock = SPIN_LOCK_UNLOCKED; -volatile struct timer_list *running_timer = NULL; +volatile struct timer_list * volatile running_timer = NULL; + +#ifdef CONFIG_SMP +#define timer_enter(t) do { running_timer = t; mb(); } while (0) +#define timer_exit() do { running_timer = NULL; } while (0) +#define timer_is_running(t) (running_timer == t) +#define timer_synchronize(t) while (timer_is_running(t)) barrier() +#else +#define timer_enter(t) do { } while (0) +#define timer_exit() do { } while (0) +#define timer_is_running(t) (0) +#define timer_synchronize(t) do { (void)(t); barrier(); } while(0) +#endif void add_timer(struct timer_list *timer) { @@ -216,6 +228,11 @@ } #ifdef CONFIG_SMP +void sync_timers(void) +{ + spin_unlock_wait(&global_bh_lock); +} + /* * SMP specific function to delete periodic timer. * Caller must disable by some means restarting the timer | |