From owner-freebsd-current@FreeBSD.ORG Tue Aug 15 15:09:11 2006 Return-Path: X-Original-To: freebsd-current@freebsd.org Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id CF28E16A523; Tue, 15 Aug 2006 15:09:11 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (66-23-211-162.clients.speedfactory.net [66.23.211.162]) by mx1.FreeBSD.org (Postfix) with ESMTP id 50DA943E79; Tue, 15 Aug 2006 15:08:20 +0000 (GMT) (envelope-from jhb@freebsd.org) Received: from localhost.corp.yahoo.com (john@localhost [127.0.0.1]) (authenticated bits=0) by server.baldwin.cx (8.13.6/8.13.6) with ESMTP id k7FF84WD028048; Tue, 15 Aug 2006 11:08:08 -0400 (EDT) (envelope-from jhb@freebsd.org) From: John Baldwin To: freebsd-current@freebsd.org Date: Tue, 15 Aug 2006 11:01:30 -0400 User-Agent: KMail/1.9.1 References: <20060814170418.GA89686@stud.fit.vutbr.cz> <44E1D8E2.9060200@FreeBSD.org> In-Reply-To: <44E1D8E2.9060200@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200608151101.30951.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [127.0.0.1]); Tue, 15 Aug 2006 11:08:09 -0400 (EDT) X-Virus-Scanned: ClamAV 0.88.3/1663/Tue Aug 15 09:08:37 2006 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx Cc: emulation@freebsd.org, Divacky Roman , Suleiman Souhlal Subject: Re: SoC: linuxolator update: first patch 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: Tue, 15 Aug 2006 15:09:12 -0000 On Tuesday 15 August 2006 10:23, Suleiman Souhlal wrote: > > Why not use KASSERT(p != NULL, ("process not found in proc_init\n")); ? > > + p->p_emuldata = em; > + PROC_UNLOCK(p); > + } else { > + /* lookup the old one */ > + em = em_find(td->td_proc, EMUL_UNLOCKED); > > According to sys/proc.h you're supposed to hold the proc lock when > accessing p->p_emuldata. That is possibly sketchier as if an ABI always sets p_emuldata on fork/exec and only curthread accesses curproc->p_emuldata you can probably just not have to do any locking at all as it will actually be effectively read-only. The emuldata fields can have different strategies in different ABIs because they will be used differently. Note how so_emuldata is locked by SOCK_LOCK() in the Linux ABI, but isn't locked in the svr4/streams code because it's set on socket creation and static until socket close. > + KASSERT(em != NULL, ("proc_init: emuldata not found in exec case.\n")); > + } > + > + em->child_clear_tid = NULL; > + em->child_set_tid = NULL; > + > + /* allocate the shared struct only in clone()/fork cases > + * in the case of clone() td = calling proc and child = pid of > + * the newly created proc > + */ > + if (child != 0) { > + if (flags & CLONE_VM) { > + /* lookup the parent */ > + p_em = em_find(td->td_proc, EMUL_LOCKED); > + KASSERT(p_em != NULL, ("proc_init: parent emuldata not found for > CLONE_VM\n")); > + em->shared = p_em->shared; > + em->shared->refs++; > > This is unsafe. Please use the functions in sys/refcount.h. Well, in this case he's already holding a lock. If he always holds a lock when accessing and modifying refs, then refcount_*() would only add overhead. > + } else { > + /* handled earlier to avoid malloc(M_WAITOK) with rwlock held */ > + } > + } > + > + > + if (child != 0) { > + EMUL_SHARED_WLOCK(&emul_shared_lock); > + LIST_INSERT_HEAD(&em->shared->threads, em, threads); > + EMUL_SHARED_WUNLOCK(&emul_shared_lock); > + > + p = pfind(child); > + PROC_UNLOCK(p); > + /* we might have a sleeping linux_schedtail */ > + wakeup(&p->p_emuldata); > > Again, you need to hold the proc lock when accessing p->p_emuldata. See above, but there is another concern here in that once you drop the proc lock, the 'p' process might go away, be free'd, etc. (Especially if you are preempted right after the PROC_UNLOCK()) > + > +void > +linux_schedtail(void *arg __unused, struct proc *p) > +{ > + struct linux_emuldata *em; > + int error = 0; > +#ifdef DEBUG > + struct thread *td = FIRST_THREAD_IN_PROC(p); > +#endif > + int *child_set_tid; > + > + if (p->p_sysent != &elf_linux_sysvec) > + return; > + > +retry: > + /* find the emuldata */ > + em = em_find(p, EMUL_UNLOCKED); > + > + if (em == NULL) { > + /* We might have been called before proc_init for this process so > + * tsleep and be woken up by it. We use p->p_emuldata for this > + */ > + > + error = tsleep(&p->p_emuldata, PLOCK, "linux_schedtail", hz); > + if (error == 0) > + goto retry; > > Why are you setting a timeout if you just retry when it expires? In this case it is a workaround for lost wakeups since it's not an interlocked sleep and wakeup. :) > ... > + > + pp = p->p_pptr; /* switch to parent */ > + PROC_LOCK(pp); > + PROC_UNLOCK(p); > + > > You might have to hold the proctree_lock here. Actually, for this he is fine, as the proc lock is sufficient to access p->p_pptr, and the parent process can't go away w/o acquiring the child's lock and reparenting it to init. -- John Baldwin