Date: Sat, 15 Feb 2003 15:40:24 +1100 From: Tim Robbins <tjr@freebsd.org> To: freebsd-smp@freebsd.org Subject: Locking down itimers Message-ID: <20030215154024.A53601@dilbert.robbins.dropbear.id.au>
next in thread | raw e-mail | index | archive | help
This patch protects p_realtimer with the proc mutex instead of Giant,
and obtains sched_lock around accesses to p_stats->p_timer[] to avoid
a potential race with hardclock. It makes getitimer(), setitimer()
and the realitexpire() callout MP-safe.
It was necessary to restructure setitimer() a bit to guarantee that
the swap:
ovalue = p_realtimer, p_realtimer = value;
occurs atomically with respect to other threads in the same process
calling setitimer() and the callout.
Does the patch seem reasonable? Any comments?
Tim
Index: sys/proc.h
===================================================================
RCS file: /x/freebsd/src/sys/sys/proc.h,v
retrieving revision 1.293
diff -U5 -p -r1.293 proc.h
--- sys/proc.h 9 Feb 2003 08:49:02 -0000 1.293
+++ sys/proc.h 12 Feb 2003 07:44:15 -0000
@@ -565,11 +565,11 @@ struct proc {
/* The following fields are all zeroed upon creation in fork. */
#define p_startzero p_oppid
pid_t p_oppid; /* (c + e) Save ppid in ptrace. XXX */
struct vmspace *p_vmspace; /* (b) Address space. */
u_int p_swtime; /* (j) Time swapped in or out. */
- struct itimerval p_realtimer; /* (h?/k?) Alarm timer. */
+ struct itimerval p_realtimer; /* (c) Alarm timer. */
struct bintime p_runtime; /* (j) Real time. */
int p_profthreads; /* (c) Num threads in addupc_task */
int p_traceflag; /* (o) Kernel trace points. */
struct vnode *p_tracep; /* (c + o) Trace to vnode. */
sigset_t p_siglist; /* (c) Sigs arrived, not delivered. */
Index: kern/kern_time.c
===================================================================
RCS file: /x/freebsd/src/sys/kern/kern_time.c,v
retrieving revision 1.96
diff -U5 -p -r1.96 kern_time.c
--- kern/kern_time.c 3 Feb 2003 19:49:34 -0000 1.96
+++ kern/kern_time.c 12 Feb 2003 14:07:00 -0000
@@ -406,45 +406,42 @@ struct getitimer_args {
};
#endif
/*
* MPSAFE
*/
-/* ARGSUSED */
int
getitimer(struct thread *td, struct getitimer_args *uap)
{
struct proc *p = td->td_proc;
struct timeval ctv;
struct itimerval aitv;
- int s;
if (uap->which > ITIMER_PROF)
return (EINVAL);
- mtx_lock(&Giant);
-
- s = splclock(); /* XXX still needed ? */
if (uap->which == ITIMER_REAL) {
/*
* Convert from absolute to relative time in .it_value
* part of real time timer. If time for real time timer
* has passed return 0, else return difference between
* current time and time for the timer to go off.
*/
+ PROC_LOCK(p);
aitv = p->p_realtimer;
+ PROC_UNLOCK(p);
if (timevalisset(&aitv.it_value)) {
getmicrouptime(&ctv);
if (timevalcmp(&aitv.it_value, &ctv, <))
timevalclear(&aitv.it_value);
else
timevalsub(&aitv.it_value, &ctv);
}
} else {
+ mtx_lock_spin(&sched_lock);
aitv = p->p_stats->p_timer[uap->which];
+ mtx_unlock_spin(&sched_lock);
}
- splx(s);
- mtx_unlock(&Giant);
return (copyout(&aitv, uap->itv, sizeof (struct itimerval)));
}
#ifndef _SYS_SYSPROTO_H_
struct setitimer_args {
@@ -453,63 +450,61 @@ struct setitimer_args {
};
#endif
/*
* MPSAFE
*/
-/* ARGSUSED */
int
setitimer(struct thread *td, struct setitimer_args *uap)
{
struct proc *p = td->td_proc;
- struct itimerval aitv;
+ struct itimerval aitv, oitv;
struct timeval ctv;
- struct itimerval *itvp;
- int s, error = 0;
+ int error;
+
+ if (uap->itv == NULL) {
+ uap->itv = uap->oitv;
+ return (getitimer(td, (struct getitimer_args *)uap));
+ }
if (uap->which > ITIMER_PROF)
return (EINVAL);
- itvp = uap->itv;
- if (itvp && (error = copyin(itvp, &aitv, sizeof(struct itimerval))))
+ if ((error = copyin(uap->itv, &aitv, sizeof(struct itimerval))))
return (error);
-
- mtx_lock(&Giant);
-
- if ((uap->itv = uap->oitv) &&
- (error = getitimer(td, (struct getitimer_args *)uap))) {
- goto done2;
- }
- if (itvp == 0) {
- error = 0;
- goto done2;
- }
- if (itimerfix(&aitv.it_value)) {
- error = EINVAL;
- goto done2;
- }
- if (!timevalisset(&aitv.it_value)) {
+ if (itimerfix(&aitv.it_value))
+ return (EINVAL);
+ if (!timevalisset(&aitv.it_value))
timevalclear(&aitv.it_interval);
- } else if (itimerfix(&aitv.it_interval)) {
- error = EINVAL;
- goto done2;
- }
- s = splclock(); /* XXX: still needed ? */
+ else if (itimerfix(&aitv.it_interval))
+ return (EINVAL);
+
if (uap->which == ITIMER_REAL) {
+ PROC_LOCK(p);
if (timevalisset(&p->p_realtimer.it_value))
callout_stop(&p->p_itcallout);
if (timevalisset(&aitv.it_value))
callout_reset(&p->p_itcallout, tvtohz(&aitv.it_value),
realitexpire, p);
getmicrouptime(&ctv);
timevaladd(&aitv.it_value, &ctv);
+ oitv = p->p_realtimer;
p->p_realtimer = aitv;
+ PROC_UNLOCK(p);
+ if (timevalisset(&oitv.it_value)) {
+ if (timevalcmp(&oitv.it_value, &ctv, <))
+ timevalclear(&oitv.it_value);
+ else
+ timevalsub(&oitv.it_value, &ctv);
+ }
} else {
+ mtx_lock_spin(&sched_lock);
+ oitv = p->p_stats->p_timer[uap->which];
p->p_stats->p_timer[uap->which] = aitv;
+ mtx_unlock_spin(&sched_lock);
}
- splx(s);
-done2:
- mtx_unlock(&Giant);
- return (error);
+ if (uap->oitv == NULL)
+ return (0);
+ return (copyout(&oitv, uap->oitv, sizeof(struct itimerval)));
}
/*
* Real interval timer expired:
* send process whose timer expired an alarm signal.
@@ -525,35 +520,31 @@ done2:
void
realitexpire(void *arg)
{
struct proc *p;
struct timeval ctv, ntv;
- int s;
p = (struct proc *)arg;
PROC_LOCK(p);
psignal(p, SIGALRM);
if (!timevalisset(&p->p_realtimer.it_interval)) {
timevalclear(&p->p_realtimer.it_value);
PROC_UNLOCK(p);
return;
}
for (;;) {
- s = splclock(); /* XXX: still neeeded ? */
timevaladd(&p->p_realtimer.it_value,
&p->p_realtimer.it_interval);
getmicrouptime(&ctv);
if (timevalcmp(&p->p_realtimer.it_value, &ctv, >)) {
ntv = p->p_realtimer.it_value;
timevalsub(&ntv, &ctv);
callout_reset(&p->p_itcallout, tvtohz(&ntv) - 1,
realitexpire, p);
- splx(s);
PROC_UNLOCK(p);
return;
}
- splx(s);
}
/*NOTREACHED*/
}
/*
Index: kern/init_main.c
===================================================================
RCS file: /x/freebsd/src/sys/kern/init_main.c,v
retrieving revision 1.224
diff -U5 -p -r1.224 init_main.c
--- kern/init_main.c 4 Feb 2003 18:47:17 -0000 1.224
+++ kern/init_main.c 12 Feb 2003 08:18:51 -0000
@@ -384,11 +384,11 @@ proc0_init(void *dummy __unused)
p->p_leader = p;
bcopy("swapper", p->p_comm, sizeof ("swapper"));
- callout_init(&p->p_itcallout, 0);
+ callout_init(&p->p_itcallout, 1);
callout_init(&td->td_slpcallout, 1);
/* Create credentials. */
p->p_ucred = crget();
p->p_ucred->cr_ngroups = 1; /* group 0 */
Index: compat/linux/linux_misc.c
===================================================================
RCS file: /x/freebsd/src/sys/compat/linux/linux_misc.c,v
retrieving revision 1.134
diff -U5 -p -r1.134 linux_misc.c
--- compat/linux/linux_misc.c 17 Oct 2002 22:00:30 -0000 1.134
+++ compat/linux/linux_misc.c 13 Feb 2003 05:06:13 -0000
@@ -162,11 +162,10 @@ linux_sysinfo(struct thread *td, struct
int
linux_alarm(struct thread *td, struct linux_alarm_args *args)
{
struct itimerval it, old_it;
struct timeval tv;
- int s;
#ifdef DEBUG
if (ldebug(alarm))
printf(ARGS(alarm, "%u"), args->secs);
#endif
@@ -176,22 +175,22 @@ linux_alarm(struct thread *td, struct li
it.it_value.tv_sec = (long)args->secs;
it.it_value.tv_usec = 0;
it.it_interval.tv_sec = 0;
it.it_interval.tv_usec = 0;
- s = splsoftclock();
+ PROC_LOCK(td->td_proc);
old_it = td->td_proc->p_realtimer;
getmicrouptime(&tv);
if (timevalisset(&old_it.it_value))
callout_stop(&td->td_proc->p_itcallout);
if (it.it_value.tv_sec != 0) {
callout_reset(&td->td_proc->p_itcallout, tvtohz(&it.it_value),
realitexpire, td->td_proc);
timevaladd(&it.it_value, &tv);
}
td->td_proc->p_realtimer = it;
- splx(s);
+ PROC_UNLOCK(td->td_proc);
if (timevalcmp(&old_it.it_value, &tv, >)) {
timevalsub(&old_it.it_value, &tv);
if (old_it.it_value.tv_usec != 0)
old_it.it_value.tv_sec++;
td->td_retval[0] = old_it.it_value.tv_sec;
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-smp" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030215154024.A53601>