Messages in this thread Patch in this message | | | Date | Fri, 20 Nov 1998 11:53:03 +0100 (CET) | From | Andrea Arcangeli <> | Subject | Re: arca-24 [Re: new arca-23 released] |
| |
On Thu, 19 Nov 1998, Linus Torvalds wrote:
>On Fri, 20 Nov 1998, Andrea Arcangeli wrote: >> >> @@ -420,8 +433,8 @@ >> >> spin_lock_irqsave(&timerlist_lock, flags); >> ret = detach_timer(timer); >> - timer->next = timer->prev = 0; >> spin_unlock_irqrestore(&timerlist_lock, flags); >> + timer->next = timer->prev = 0; >> return ret; >> } >> > >The above is definitely wrong. You're changing "next" outside the timer >lock, which means that suddenly the consistency of next is not guaranteed >(imagine another CPU waiting on the lock to do an "add_timer()" on that >timer when you exit from the above - now you're going to have a race where >both CPU's potentially change the entries at the same time.
Woops, now I see the point. Excuse me. My thought was: "when a timer is detached has not more longer sense and I can play with it without been atomic as done by the init function of every driver (that recalls init_timer()...)". It' s been not an exaustive thought...
>> - expire = timespec_to_jiffies(&t) + (t.tv_sec || t.tv_nsec); >> + expire = (long) (timespec_to_jiffies(&t) + (t.tv_sec || t.tv_nsec)); >> + /* >> + * Handle a too high timeout for the scheduler after the >> + * struct timespec to jiffies conversion. -arca >> + */ >> + if (expire < 0) >> + expire = MAX_SCHEDULE_TIMEOUT; > >Naah, much better to just say that "max-jiffies" as returned from the >timespec_to_jiffies() routine must allow people to add one for rounding >purposes. That just changes a test against MAX_LONG to (MAX_LONG-1) >instead of introducing strange things like the above.
I agree even if I implemented the thing a bit different (since there are not runtime issues because timeval-functions are inline).
Thanks.
Index: linux/kernel/signal.c diff -u linux/kernel/signal.c:1.1.1.1 linux/kernel/signal.c:1.1.1.1.2.2 --- linux/kernel/signal.c:1.1.1.1 Fri Nov 20 00:01:12 1998 +++ linux/kernel/signal.c Fri Nov 20 11:39:11 1998 @@ -742,8 +742,7 @@ timeout = MAX_SCHEDULE_TIMEOUT; if (uts) - timeout = (timespec_to_jiffies(&ts) - + (ts.tv_sec || ts.tv_nsec)); + timeout = timespec_to_jiffies_plus_one(&ts); current->state = TASK_INTERRUPTIBLE; timeout = schedule_timeout(timeout); Index: linux/kernel/sched.c diff -u linux/kernel/sched.c:1.1.1.1 linux/kernel/sched.c:1.1.1.1.2.8 --- linux/kernel/sched.c:1.1.1.1 Fri Nov 20 00:01:12 1998 +++ linux/kernel/sched.c Fri Nov 20 11:39:05 1998 @@ -380,15 +392,6 @@ spinlock_t timerlist_lock = SPIN_LOCK_UNLOCKED; -void add_timer(struct timer_list *timer) -{ - unsigned long flags; - - spin_lock_irqsave(&timerlist_lock, flags); - internal_add_timer(timer); - spin_unlock_irqrestore(&timerlist_lock, flags); -} - static inline int detach_timer(struct timer_list *timer) { struct timer_list *prev = timer->prev; @@ -402,6 +405,16 @@ return 0; } +void add_timer(struct timer_list *timer) +{ + unsigned long flags; + + spin_lock_irqsave(&timerlist_lock, flags); + if (!timer_pending(timer)) + internal_add_timer(timer); + spin_unlock_irqrestore(&timerlist_lock, flags); +} + void mod_timer(struct timer_list *timer, unsigned long expires) { unsigned long flags; @@ -1604,7 +1620,7 @@ asmlinkage int sys_nanosleep(struct timespec *rqtp, struct timespec *rmtp) { struct timespec t; - unsigned long expire; + long expire; if(copy_from_user(&t, rqtp, sizeof(struct timespec))) return -EFAULT; @@ -1626,7 +1642,7 @@ return 0; } - expire = timespec_to_jiffies(&t) + (t.tv_sec || t.tv_nsec); + expire = timespec_to_jiffies_plus_one(&t); current->state = TASK_INTERRUPTIBLE; expire = schedule_timeout(expire); Index: linux/include/linux/time.h diff -u linux/include/linux/time.h:1.1.1.1 linux/include/linux/time.h:1.1.1.1.2.2 --- linux/include/linux/time.h:1.1.1.1 Fri Nov 20 00:01:14 1998 +++ linux/include/linux/time.h Fri Nov 20 11:43:03 1998 @@ -3,6 +3,7 @@ #include <asm/param.h> #include <linux/types.h> +#include <linux/kernel.h> #ifndef _STRUCT_TIMESPEC #define _STRUCT_TIMESPEC @@ -16,21 +17,47 @@ * change timeval to jiffies, trying to avoid the * most obvious overflows.. */ -static __inline__ unsigned long +static __inline__ long timespec_to_jiffies(struct timespec *value) { - unsigned long sec = value->tv_sec; - long nsec = value->tv_nsec; + long sec = value->tv_sec, nsec = value->tv_nsec, retval; - if (sec > ((long)(~0UL >> 1) / HZ)) - return ~0UL >> 1; nsec += 1000000000L / HZ - 1; nsec /= 1000000000L / HZ; - return HZ * sec + nsec; + retval = HZ * sec + nsec; + /* + * Here we are careful to return a positive value. This helps + * when the retval will be a parameter of schedule_timeout() + */ + if (retval < 0) + return LONG_MAX; + else + return retval; } +/* + * change timeval to jiffies, plus 1 if the timespec values are not null + */ +static __inline__ long +timespec_to_jiffies_plus_one(struct timespec *value) +{ + long sec = value->tv_sec, nsec = value->tv_nsec, retval; + + nsec += 1000000000L / HZ - 1; + nsec /= 1000000000L / HZ; + retval = HZ * sec + nsec + (value->tv_sec || value->tv_nsec); + /* + * Here we are careful to return a positive value. This helps + * when the retval will be a parameter of schedule_timeout() + */ + if (retval < 0) + return LONG_MAX; + else + return retval; +} + static __inline__ void -jiffies_to_timespec(unsigned long jiffies, struct timespec *value) +jiffies_to_timespec(long jiffies, struct timespec *value) { value->tv_nsec = (jiffies % HZ) * (1000000000L / HZ); value->tv_sec = jiffies / HZ;
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |