From owner-freebsd-current@FreeBSD.ORG Thu Mar 31 18:40:46 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 1D860106564A; Thu, 31 Mar 2011 18:40:46 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-gw0-f54.google.com (mail-gw0-f54.google.com [74.125.83.54]) by mx1.freebsd.org (Postfix) with ESMTP id B59308FC15; Thu, 31 Mar 2011 18:40:45 +0000 (UTC) Received: by gwb15 with SMTP id 15so1269128gwb.13 for ; Thu, 31 Mar 2011 11:40:45 -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=PP1Sud/MuIN+9ebJzpEEbuHT8Tahc9MN27AzOMYbPCE=; b=oRLsswDYKRGjCt1C8r8lcoCE3TgIOYAivE0IHnydddpKPP3ol0BjobFnIZ30ha8yky WXH6fGPUXdcO2WzejdwSSruqpyqSBLWRywxu5oDC1aZThpIJ7cnmOjkscj1zGvKLVkvy /OyQn4je6mI25mdr2ur5DKD8UHmQsD2KoUjPY= 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=pikYltx0uC9mMimUhAL5KAE5eTDWNHDx56IMoCJEgdGyMGcwEwnp1M5lzQQKliBs+Z u6ZyOdStcrzB5a9KcTQgpvKjmeSRbt/LL4ly7Jirw5ZEMnrv7EDlxlMVfXnYHXPXN96d pTikXd/ZRTDhBCnK/rVLJOyB+DR9ygYFJUIsU= MIME-Version: 1.0 Received: by 10.236.190.33 with SMTP id d21mr1795796yhn.233.1301596844824; Thu, 31 Mar 2011 11:40:44 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.236.110.20 with HTTP; Thu, 31 Mar 2011 11:40:44 -0700 (PDT) In-Reply-To: <201103311437.19682.jhb@freebsd.org> References: <201103311418.31658.jhb@freebsd.org> <201103311437.19682.jhb@freebsd.org> Date: Thu, 31 Mar 2011 14:40:44 -0400 X-Google-Sender-Auth: 2vFec7t1dj_wKdhvi_wIfJYTe88 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:40:46 -0000 2011/3/31 John Baldwin : > On Thursday, March 31, 2011 2:20:11 pm Attilio Rao wrote: >> 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 whi= ch >> >> >> is linked to allproc list and both allproc_lock and new process lo= ck >> >> >> are unlocked before sched_fork() is called, where new thread td_lo= ck >> >> >> 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_THRE= AD_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 touch= ing 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 appr= oach, > 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 allp= roc > list to track "pids in use", we will be stuck with some sort of "process > is still being built" sentry. 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 --=20 Peace can only be achieved by understanding - A. Einstein