From owner-freebsd-current@FreeBSD.ORG Tue Aug 15 14:55:31 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 5BAE116A4DA; Tue, 15 Aug 2006 14:55:31 +0000 (UTC) (envelope-from xdivac02@stud.fit.vutbr.cz) Received: from eva.fit.vutbr.cz (eva.fit.vutbr.cz [147.229.10.14]) by mx1.FreeBSD.org (Postfix) with ESMTP id 8598843D72; Tue, 15 Aug 2006 14:55:25 +0000 (GMT) (envelope-from xdivac02@stud.fit.vutbr.cz) Received: from eva.fit.vutbr.cz (localhost [127.0.0.1]) by eva.fit.vutbr.cz (envelope-from xdivac02@eva.fit.vutbr.cz) (8.13.7/8.13.7) with ESMTP id k7FEtHwi058348 (version=TLSv1/SSLv3 cipher=EDH-RSA-DES-CBC3-SHA bits=168 verify=NO); Tue, 15 Aug 2006 16:55:17 +0200 (CEST) Received: (from xdivac02@localhost) by eva.fit.vutbr.cz (8.13.7/8.13.3/Submit) id k7FEtHhU058347; Tue, 15 Aug 2006 16:55:17 +0200 (CEST) Date: Tue, 15 Aug 2006 16:55:17 +0200 From: Divacky Roman To: John Baldwin Message-ID: <20060815145517.GA50007@stud.fit.vutbr.cz> References: <20060814170418.GA89686@stud.fit.vutbr.cz> <200608150910.51434.jhb@freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200608150910.51434.jhb@freebsd.org> User-Agent: Mutt/1.4.2i X-Scanned-By: MIMEDefang 2.54 on 147.229.10.14 Cc: emulation@freebsd.org, freebsd-current@freebsd.org 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 14:55:31 -0000 On Tue, Aug 15, 2006 at 09:10:50AM -0400, John Baldwin wrote: > On Monday 14 August 2006 13:04, Divacky Roman wrote: > > Hi, > > > > I am a student doing SoC linuxolator update. The work involved updating > > linuxolator to be able to work with 2.6.x (2.6.16 in my case) kernel emulation. > > > > To be able to run 2.6.x emulation I had to implement NPTL, which means NPTL, futexes > > and thread area, pid mangling + various hacks to make it work. > > > > This is the first patch meant for public revision/testing: > > > > www.stud.fit.vutbr.cz/~xdivac02/linuxolator26-diff.patch > > Some comments: > > - You shouldn't add new nested includes (such as in > amd64/linux32/linux.h), especially sys/lock.h, sys/mutex.h, and > sys/sx.h. (If you need to examine mutex internals you would include > sys/_mutex.h in a header, but for normal API usage you need to > include sys/mutex.h and sys/lock.h in the appropriate C files. > sys/param.h is far too large of a header to be used in a nested > include. 1) fixed. thnx! > - Check style(9). For example, block comments should look like this: > > /* > * This is a really long comment that takes up > * more than one line. > */ > > rather than this: > > /* this is a really long comment that takes up > * more than one line > */ 2) fixed. thnx! > - You use the EMUL_LOCK in em_find() to look at p_emuldata but don't > hold it when writing to p_emuldata in linux_proc_init(). There you > only hold the proc lock. 3) fixed. thnx! > - You should probably hold the proc lock until after the wakeup() > in linux_proc_init() since the pfind() / proc_unlock() is what holds > a reference to keep the child from going away. 4) fixed. thnx! > - In linux_proc_init() you pass EMUL_LOCKED to em_find() in the CLONE_VM > case when the lock isn't held. 5) fixed by fix #5, thnx! > - Personal style: please use malloc() and free() rather than MALLOC() and > FREE(). We haven't inlined malloc() in a long, long time and the macros > are really there for older code. 6) fixed. thnx! > - You tsleep() in linux_schedtail() while holding one lock and do the > wakeup() in linux_proc_init() while holding no locks. You've got lost > wakeup races that way that you work around by having to use a goto and > a sleep with a timeout. 7) I dont understand this. can you explain more? thnx > - You should include the appropriate header to get the declaration of 'hz' > rather than just adding an 'extern' directly in your code. 8) I didnt find such header. > - In futex_get() you don't handle the race of two concurrent creates. 9) hopefully fixed. > - In futex_sleep(), you probably should be using an interlocked sleep such > as msleep() or cv_wait() to avoid lost wakeups. 10) open issue, this is currently under investigation because we in fact happen to have problems with futexes > - In futex_sleep(), you should probably loop rather than recurse to avoid > blowing the stack. 11) I am not sure about the logic but I think the recursion is done exactly once I'll coordinate with manu@netbsd > - In futex_atomic_op(), if you take a page fault on one of your operations > it's going to blow up because of the page fault during a critical section. > Your futex ops should be atomic (perhaps you can do them all using > casuptr()?) and you shouldn't use a critical section here. 12) the operations should be atomic (the operation on shared data is atomic) so I just removed the crit section. > - You should stick the sx locks in a header and not litter 'externs' in > source files. 13) fixed. thnx! > - Should come up with a better name than 'schedtail' for the eventhandler > in fork_exit(). Maybe 'process_fork_exit'? 14) linux uses this name. I might change it but I think its ok to be in sync with linux. > - You should probably ask Peter to review the link_elf_obj change. An > alternative is to use linker_lookup_set() in your module load routine to > lookup your linker set details. 15) yes... when I start working on amd64 I will definitely do something about it > - Regarding the locking, I'm not sure why you have the two separate sx locks, > and why you don't just merge them into a single lock instead. 16) it makes the locking easier and they cover different things. I like it this way :) thnx for exhausting review! I commited some fixes to p4, you might want to check them roman