From owner-freebsd-current@FreeBSD.ORG Thu Mar 31 18:20:13 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E83CE1065675; Thu, 31 Mar 2011 18:20:12 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-gy0-f182.google.com (mail-gy0-f182.google.com [209.85.160.182]) by mx1.freebsd.org (Postfix) with ESMTP id 8A9528FC1D; Thu, 31 Mar 2011 18:20:12 +0000 (UTC) Received: by gyg13 with SMTP id 13so1259007gyg.13 for ; Thu, 31 Mar 2011 11:20:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=+zLOjZUbaaWEpv03j1mDXkRSK+U5frfxwGxNrSkBxd4=; b=F6sWDUECs8R8NMKkMgHubD3wqDxCdndMtQmn0hVSBfADU+9+qlbKUb8ksmcc+04nWm mGDMPktPRPAcVoxpH6SvLrOKS0k25lFOcwztDIVd7SKq0l0x1pW/qQjR0S1awLSKWINY xKYL2Dyc8Tl4nRvlfB9ElTKz+HnPjOXS8iwDY= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=ti5vjDYpzzFL1e3Lv9jq0L1cD5qwWD4ONk90VWedW2sSemYsV/LVoXT/xL5ytNzVWo bENW/rcmFqBoqmXZzPzUgMc9tiFv1ArNCaKIqJT3xPiK/O/fkU0ZQNLZZCmzURJ9oaY4 o68b9D6YJ98nxK+Gc25YWAJhfw8om+nS5byW8= MIME-Version: 1.0 Received: by 10.236.195.3 with SMTP id o3mr1756285yhn.133.1301595611747; Thu, 31 Mar 2011 11:20:11 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.236.110.20 with HTTP; Thu, 31 Mar 2011 11:20:11 -0700 (PDT) In-Reply-To: <201103311418.31658.jhb@freebsd.org> References: <201103310958.51416.jhb@freebsd.org> <201103311418.31658.jhb@freebsd.org> Date: Thu, 31 Mar 2011 14:20:11 -0400 X-Google-Sender-Auth: 7SM8zFdefeJmdLATPai2zUpu-5k Message-ID: From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: freebsd-current@freebsd.org, Svatopluk Kraus Subject: Re: schedcpu() in /sys/kern/sched_4bsd.c calls thread_lock() on thread with un-initialized td_lock X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 Mar 2011 18:20:13 -0000 2011/3/31 John Baldwin : > On Thursday, March 31, 2011 12:34:31 pm Attilio Rao wrote: >> 2011/3/31 John Baldwin : >> > On Thursday, March 31, 2011 7:32:26 am Svatopluk Kraus wrote: >> >> Hi, >> >> >> >> =C2=A0 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 >> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> > --- sched_4bsd.c =C2=A0 =C2=A0 =C2=A0 =C2=A0(revision 220190) >> > +++ sched_4bsd.c =C2=A0 =C2=A0 =C2=A0 =C2=A0(working copy) >> > @@ -463,6 +463,10 @@ schedcpu(void) >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0sx_slock(&allproc_lock); >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0FOREACH_PROC_IN_SYSTEM(p) { >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PROC_LOCK(p); >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (p->p_state =3D= =3D PRS_NEW) { >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 PROC_UNLOCK(p); >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 continue; >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0FOREACH_THREAD_= IN_PROC(p, td) { >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0awake =3D 0; >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0thread_lock(td); >> > >> >> 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. > > But the thread state isn't valid anyway. =C2=A04BSD 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. Attilio --=20 Peace can only be achieved by understanding - A. Einstein