From owner-freebsd-emulation@FreeBSD.ORG  Tue Aug 15 15:21:15 2006
Return-Path: <owner-freebsd-emulation@FreeBSD.ORG>
X-Original-To: emulation@freebsd.org
Delivered-To: freebsd-emulation@FreeBSD.ORG
Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125])
	by hub.freebsd.org (Postfix) with ESMTP id 6AFB616A4DD;
	Tue, 15 Aug 2006 15:21:15 +0000 (UTC)
	(envelope-from ssouhlal@FreeBSD.org)
Received: from elvis.mu.org (elvis.mu.org [192.203.228.196])
	by mx1.FreeBSD.org (Postfix) with ESMTP id 0164E43D73;
	Tue, 15 Aug 2006 15:21:14 +0000 (GMT)
	(envelope-from ssouhlal@FreeBSD.org)
Received: from [172.30.10.238] (unknown [216.239.55.7])
	by elvis.mu.org (Postfix) with ESMTP id 081E61A3C20;
	Tue, 15 Aug 2006 08:21:13 -0700 (PDT)
Message-ID: <44E1E64F.6020205@FreeBSD.org>
Date: Tue, 15 Aug 2006 17:20:47 +0200
From: Suleiman Souhlal <ssouhlal@FreeBSD.org>
User-Agent: Mozilla Thunderbird 1.0.7 (X11/20051204)
X-Accept-Language: en-us, en
MIME-Version: 1.0
To: John Baldwin <jhb@freebsd.org>
References: <20060814170418.GA89686@stud.fit.vutbr.cz>
	<44E1D8E2.9060200@FreeBSD.org> <200608151101.30951.jhb@freebsd.org>
In-Reply-To: <200608151101.30951.jhb@freebsd.org>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: emulation@freebsd.org, freebsd-current@freebsd.org
Subject: Re: SoC: linuxolator update: first patch
X-BeenThere: freebsd-emulation@freebsd.org
X-Mailman-Version: 2.1.5
Precedence: list
List-Id: Development of Emulators of other operating systems
	<freebsd-emulation.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-emulation>, 
	<mailto:freebsd-emulation-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-emulation>
List-Post: <mailto:freebsd-emulation@freebsd.org>
List-Help: <mailto:freebsd-emulation-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-emulation>, 
	<mailto:freebsd-emulation-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Tue, 15 Aug 2006 15:21:15 -0000

John Baldwin wrote:
> 
>>+		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.

Isn't he holding the wrong lock (emul_lock vs emul_shared_lock)?

>>+
>>+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. :)

Ew..

Thanks,

-- Suleiman