From owner-freebsd-emulation@FreeBSD.ORG Thu May 1 08:14:06 2008 Return-Path: Delivered-To: emulation@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 60710106566B for ; Thu, 1 May 2008 08:14:06 +0000 (UTC) (envelope-from rdivacky@vlk.vlakno.cz) Received: from vlakno.cz (vlk.vlakno.cz [62.168.28.247]) by mx1.freebsd.org (Postfix) with ESMTP id 17EA08FC1F for ; Thu, 1 May 2008 08:14:05 +0000 (UTC) (envelope-from rdivacky@vlk.vlakno.cz) Received: from localhost (localhost [127.0.0.1]) by vlakno.cz (Postfix) with ESMTP id 4C86E67D5CF; Thu, 1 May 2008 10:13:36 +0200 (CEST) X-Virus-Scanned: amavisd-new at vlakno.cz Received: from vlakno.cz ([127.0.0.1]) by localhost (vlk.vlakno.cz [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id V-OXxk0qljG1; Thu, 1 May 2008 10:13:35 +0200 (CEST) Received: from vlk.vlakno.cz (localhost [127.0.0.1]) by vlakno.cz (Postfix) with ESMTP id E16AB67D5CD; Thu, 1 May 2008 10:13:34 +0200 (CEST) Received: (from rdivacky@localhost) by vlk.vlakno.cz (8.14.2/8.14.2/Submit) id m418DYLT055232; Thu, 1 May 2008 10:13:34 +0200 (CEST) (envelope-from rdivacky) Date: Thu, 1 May 2008 10:13:34 +0200 From: Roman Divacky To: Alexander Kabaev Message-ID: <20080501081334.GA54624@freebsd.org> References: <20080430081806.GA81772@freebsd.org> <20080430121513.33f9452b@kan.dnsalias.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080430121513.33f9452b@kan.dnsalias.net> User-Agent: Mutt/1.4.2.3i Cc: emulation@freebsd.org Subject: Re: [PATCH]: robust futexes X-BeenThere: freebsd-emulation@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Development of Emulators of other operating systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 May 2008 08:14:06 -0000 On Wed, Apr 30, 2008 at 12:15:13PM -0400, Alexander Kabaev wrote: > On Wed, 30 Apr 2008 10:18:06 +0200 > Roman Divacky wrote: > > > hi > > > > I implemented robust futexes in linuxulator and I need to get it > > reviewed/tested. The best way to test it is (according to linux > > documnetation) to run yum and kill -9 it while it runs. > > > > The patch is here: > > http://www.vlakno.cz/~rdivacky/linux_robust_futex.patch > > > > the patch should be ok as I followed linux code very closely (most of > > the code runs in userspace so kernel has very well defined work). I > > tested it lightly on i386. > > > > I'd like to commit this quite soon so please help. > > > > thnx! > > > > roman > Hi, > > some comments: > > linux_emul.c: > > @@ -86,6 +86,7 @@ > em = malloc(sizeof *em, M_LINUX, M_WAITOK | M_ZERO); > em->pid = child; > em->pdeath_signal = 0; > + em->robust_futexes = NULL; > > M_ZERO is not quite zero enough? :) I prefer it to be initialized so people can see immediately that its '0/NULL' dont think it matters much :) if this penalizes fork() by more than 10% I'll remove that :-D > linux_futex.c in release_futexes: > > + head = em->robust_futexes; > + > + if (fetch_robust_entry(&entry, &head->list.next, &pi)) > + return; > > Aren't you taking a fault in copyin unconditionally if > em->robust_mutexes happens to be NULL? Why not check is for NULL first? I think it can be done this way and it makes sense... I wonder why linux does not check it, they usually optimize everything... > Also, is sched_relinguish really necessary after each each futex > recovery _except_ from the 'pending' futex one? linux calls cond_resched() in this place and I believe there's a reason for this. I dont know much what the userspace part is doing but I believe it makes sense to reschedule after each futex recovery so other threads can "do stuff". anyway.. this is what linux does and I believe we should stick to it. Do you have any particular reason why you mind this? > i386/conf/GENERIC: > > Does not belong in this patch, probably included in by mistake. yes.. this is included by mistake.. thnx a lot for the review! an updated patch can be found at: www.vlakno.cz/~rdivacky/linux_robust_futex.2.patch roman