schedcpu() in /sys/kern/sched_4bsd.c calls thread_lock() on thread with un-initialized td_lock
Svatopluk Kraus
I've got a page fault (because of NULL td_lock) in
thread_lock_flags() called from schedcpu() in /sys/kern/sched_4bsd.c
file. During process fork, new thread is linked to new process which
is linked to allproc list and both allproc_lock and new process lock
are unlocked before sched_fork() is called, where new thread td_lock
is initialized. Only PRS_NEW process status is on sentry but not
checked in schedcpu().
Svata
_______________________________________________
freebsd...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-curre...@freebsd.org"
John Baldwin
> Hi,
>
> I've got a page fault (because of NULL td_lock) in
> thread_lock_flags() called from schedcpu() in /sys/kern/sched_4bsd.c
> file. During process fork, new thread is linked to new process which
> is linked to allproc list and both allproc_lock and new process lock
> are unlocked before sched_fork() is called, where new thread td_lock
> is initialized. Only PRS_NEW process status is on sentry but not
> checked in schedcpu().
I think this should fix it:
Index: sched_4bsd.c
===================================================================
--- sched_4bsd.c (revision 220190)
+++ sched_4bsd.c (working copy)
@@ -463,6 +463,10 @@ schedcpu(void)
sx_slock(&allproc_lock);
FOREACH_PROC_IN_SYSTEM(p) {
PROC_LOCK(p);
+ if (p->p_state == PRS_NEW) {
+ PROC_UNLOCK(p);
+ continue;
+ }
FOREACH_THREAD_IN_PROC(p, td) {
awake = 0;
thread_lock(td);
--
John Baldwin
Andriy Gapon
> Hi,
>
> I've got a page fault (because of NULL td_lock) in
> thread_lock_flags() called from schedcpu() in /sys/kern/sched_4bsd.c
> file. During process fork, new thread is linked to new process which
> is linked to allproc list and both allproc_lock and new process lock
> are unlocked before sched_fork() is called, where new thread td_lock
> is initialized. Only PRS_NEW process status is on sentry but not
> checked in schedcpu().
How recent is your current?
This sounds like something that could have been recently fixed.
--
Andriy Gapon
Svatopluk Kraus
> on 31/03/2011 14:32 Svatopluk Kraus said the following:
>> Hi,
>>
>> I've got a page fault (because of NULL td_lock) in
>> thread_lock_flags() called from schedcpu() in /sys/kern/sched_4bsd.c
>> file. During process fork, new thread is linked to new process which
>> is linked to allproc list and both allproc_lock and new process lock
>> are unlocked before sched_fork() is called, where new thread td_lock
>> is initialized. Only PRS_NEW process status is on sentry but not
>> checked in schedcpu().
>
> How recent is your current?
> This sounds like something that could have been recently fixed.
>
My current is from 20.3.2011 but I have looked at CVS on freebsd.org
just before my post. It didn't look to be solved.
Svatopluk Kraus
> On Thursday, March 31, 2011 7:32:26 am Svatopluk Kraus wrote:
>>
>> I've got a page fault (because of NULL td_lock) in
>> thread_lock_flags() called from schedcpu() in /sys/kern/sched_4bsd.c
>> file. During process fork, new thread is linked to new process which
>> is linked to allproc list and both allproc_lock and new process lock
>> are unlocked before sched_fork() is called, where new thread td_lock
>> is initialized. Only PRS_NEW process status is on sentry but not
>> checked in schedcpu().
>
>
> Index: sched_4bsd.c
> ===================================================================
> --- sched_4bsd.c (revision 220190)
> +++ sched_4bsd.c (working copy)
> @@ -463,6 +463,10 @@ schedcpu(void)
> sx_slock(&allproc_lock);
> FOREACH_PROC_IN_SYSTEM(p) {
> PROC_LOCK(p);
> + if (p->p_state == PRS_NEW) {
> + PROC_UNLOCK(p);
> + continue;
> + }
> FOREACH_THREAD_IN_PROC(p, td) {
> awake = 0;
> thread_lock(td);
>
Thanks for patch. Maybe, test p_state not to be PRS_NORMAL could be better?
I've got next (same reason) page fault in thread_lock_flags() called
from scheduler() in sys/vm/vm_glue.c. I try to search for
FOREACH_THREAD_IN_PROC() together with FOREACH_PROC_IN_SYSTEM() in
/sys subtree and next problem could be in deadlkres() in
sys/kern/kern_clock.c at least.
Attilio Rao
I don't really think this fix is right because otherwise, when using
sched_4bsd anytime we are going to scan the thread list within a proc
we need to check for PRS_NEW.
We likely need to change the init scheme for the td_lock by having a
scheduler primitive setting it and doing that on thread_init() UMA
constructor, or similar approach.
Attilio
--
Peace can only be achieved by understanding - A. Einstein
John Baldwin
But the thread state isn't valid anyway. 4BSD shouldn't be touching the
thread since it is in an incomplete / undefined state.
--
John Baldwin
Attilio Rao
Yep, in this case I'd then want to just add the threads to proc once
they are fully initialized.
It is pointless (and dangerous) to replicate this check all over,
besides we want scheduler agnostic code, which means every iterations
of p_threads will need to check for a valid state of threads.
Attilio
--
Peace can only be achieved by understanding - A. Einstein
John Baldwin
> On Thu, Mar 31, 2011 at 3:58 PM, John Baldwin <j...@freebsd.org> wrote:
I thought about that, but zombies are always moved to zombproc atomically
with changing p_state (and under an exclusive allproc_lock) and all the
other places currently use this type of check.
> I've got next (same reason) page fault in thread_lock_flags() called
> from scheduler() in sys/vm/vm_glue.c. I try to search for
> FOREACH_THREAD_IN_PROC() together with FOREACH_PROC_IN_SYSTEM() in
> /sys subtree and next problem could be in deadlkres() in
> sys/kern/kern_clock.c at least.
Here is a larger patch:
Index: kern/kern_ktrace.c
===================================================================
--- kern/kern_ktrace.c (revision 220190)
+++ kern/kern_ktrace.c (working copy)
@@ -882,7 +882,8 @@
nfound = 0;
LIST_FOREACH(p, &pg->pg_members, p_pglist) {
PROC_LOCK(p);
- if (p_cansee(td, p) != 0) {
+ if (p->p_state == PRS_NEW ||
+ p_cansee(td, p) != 0) {
PROC_UNLOCK(p);
continue;
}
Index: kern/kern_sig.c
===================================================================
--- kern/kern_sig.c (revision 220190)
+++ kern/kern_sig.c (working copy)
@@ -1799,7 +1799,8 @@
PGRP_LOCK_ASSERT(pgrp, MA_OWNED);
LIST_FOREACH(p, &pgrp->pg_members, p_pglist) {
PROC_LOCK(p);
- if (checkctty == 0 || p->p_flag & P_CONTROLT)
+ if (p->p_state == PRS_NORMAL &&
+ (checkctty == 0 || p->p_flag & P_CONTROLT))
pksignal(p, sig, ksi);
PROC_UNLOCK(p);
}
@@ -3313,7 +3314,8 @@
PGRP_LOCK(sigio->sio_pgrp);
LIST_FOREACH(p, &sigio->sio_pgrp->pg_members, p_pglist) {
PROC_LOCK(p);
- if (CANSIGIO(sigio->sio_ucred, p->p_ucred) &&
+ if (p->p_state == PRS_NORMAL &&
+ CANSIGIO(sigio->sio_ucred, p->p_ucred) &&
(checkctty == 0 || (p->p_flag & P_CONTROLT)))
psignal(p, sig);
PROC_UNLOCK(p);
Index: kern/kern_clock.c
===================================================================
--- kern/kern_clock.c (revision 220190)
+++ kern/kern_clock.c (working copy)
@@ -201,6 +201,10 @@
tryl = 0;
FOREACH_PROC_IN_SYSTEM(p) {
PROC_LOCK(p);
+ if (p->p_state == PRS_NEW) {
+ PROC_UNLOCK(p);
+ continue;
+ }
FOREACH_THREAD_IN_PROC(p, td) {
/*
Index: kern/sched_4bsd.c
===================================================================
--- kern/sched_4bsd.c (revision 220190)
+++ kern/sched_4bsd.c (working copy)
@@ -463,6 +463,10 @@
sx_slock(&allproc_lock);
FOREACH_PROC_IN_SYSTEM(p) {
PROC_LOCK(p);
+ if (p->p_state == PRS_NEW) {
+ PROC_UNLOCK(p);
+ continue;
+ }
FOREACH_THREAD_IN_PROC(p, td) {
awake = 0;
thread_lock(td);
Index: kern/kern_resource.c
===================================================================
--- kern/kern_resource.c (revision 220190)
+++ kern/kern_resource.c (working copy)
@@ -129,7 +129,8 @@
sx_sunlock(&proctree_lock);
LIST_FOREACH(p, &pg->pg_members, p_pglist) {
PROC_LOCK(p);
- if (p_cansee(td, p) == 0) {
+ if (p->p_state == PRS_NORMAL &&
+ p_cansee(td, p) == 0) {
if (p->p_nice < low)
low = p->p_nice;
}
@@ -215,7 +216,8 @@
sx_sunlock(&proctree_lock);
LIST_FOREACH(p, &pg->pg_members, p_pglist) {
PROC_LOCK(p);
- if (p_cansee(td, p) == 0) {
+ if (p->p_state == PRS_NORMAL &&
+ p_cansee(td, p) == 0) {
error = donice(td, p, uap->prio);
found++;
}
@@ -230,7 +232,8 @@
sx_slock(&allproc_lock);
FOREACH_PROC_IN_SYSTEM(p) {
PROC_LOCK(p);
- if (p->p_ucred->cr_uid == uap->who &&
+ if (p->p_state == PRS_NORMAL &&
+ p->p_ucred->cr_uid == uap->who &&
p_cansee(td, p) == 0) {
error = donice(td, p, uap->prio);
found++;
Index: vm/vm_glue.c
===================================================================
--- vm/vm_glue.c (revision 220190)
+++ vm/vm_glue.c (working copy)
@@ -730,7 +730,8 @@
sx_slock(&allproc_lock);
FOREACH_PROC_IN_SYSTEM(p) {
PROC_LOCK(p);
- if (p->p_flag & (P_SWAPPINGOUT | P_SWAPPINGIN | P_INMEM)) {
+ if (p->p_state == PRS_NEW ||
+ p->p_flag & (P_SWAPPINGOUT | P_SWAPPINGIN | P_INMEM)) {
PROC_UNLOCK(p);
continue;
}
--
John Baldwin
John Baldwin
> 2011/3/31 John Baldwin <j...@freebsd.org>:
> > On Thursday, March 31, 2011 12:34:31 pm Attilio Rao wrote:
> >> 2011/3/31 John Baldwin <j...@freebsd.org>:
> >> sched_4bsd anytime we are going to scan the thread list within a proc
> >> we need to check for PRS_NEW.
> >>
> >> We likely need to change the init scheme for the td_lock by having a
> >> scheduler primitive setting it and doing that on thread_init() UMA
> >> constructor, or similar approach.
> >
> > But the thread state isn't valid anyway. 4BSD shouldn't be touching the
> > thread since it is in an incomplete / undefined state.
>
> Yep, in this case I'd then want to just add the threads to proc once
> they are fully initialized.
>
> It is pointless (and dangerous) to replicate this check all over,
> besides we want scheduler agnostic code, which means every iterations
> of p_threads will need to check for a valid state of threads.
Yes, we do have to check for PRS_NEW in many places with the current approach,
but we need some way to reserve the PID to avoid duplicates and unless we
expand the scope of allproc in fork by a whole lot or stop using the allproc
list to track "pids in use", we will be stuck with some sort of "process
is still being built" sentry.
Attilio Rao
Yes, you are right, I was assuming you wanted to work on a larger
patchset though.
If you are happy enough with the band-aid, for the moment, ok, but I
strongly raccomand to change this in the future (could be a nice task
to work through BSDCan, for example).
Attilio
--
Peace can only be achieved by understanding - A. Einstein
Julian Elischer
it was not put into the proc list until it was set up.
I know you don't believe me but that's how it was around 2000 I'm
pretty sure of it.
John Baldwin
Check out the PID allocation in fork() back when you committed KSE-M2 in 2002
as an example starting at line 336 in kern_fork.c. Note that much of the
for loop goes back to revision 1541 (effectively 1.1 of kern_fork.c in CVS
terms):
http://svn.freebsd.org/viewvc/base/head/sys/kern/kern_fork.c?annotate=83366
Even before trasz@ refactored fork1() and shuffled the findpid code around to
a new function, the annotate history for the loop to find a free pid still goes
back to 1.1:
http://svn.freebsd.org/viewvc/base/head/sys/kern/kern_fork.c?annotate=83366
FreeBSD has always used this process to find a free PID. SVN and CVS history
does not lie.
--
John Baldwin
Julian Elischer
it's possible I'm remembering a change that never made it in...