lkml.org 
[lkml]   [1998]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: 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/

\
 
 \ /
  Last update: 2005-03-22 13:45    [W:0.068 / U:1.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site