From owner-freebsd-current@FreeBSD.ORG Tue Aug 15 13:14:57 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 82B8816A4E0; Tue, 15 Aug 2006 13:14:57 +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 9D0E543D49; Tue, 15 Aug 2006 13:14:56 +0000 (GMT) (envelope-from jhb@freebsd.org) Received: from zion.baldwin.cx (zion.baldwin.cx [192.168.0.7]) (authenticated bits=0) by server.baldwin.cx (8.13.6/8.13.6) with ESMTP id k7FDEp8h027268; Tue, 15 Aug 2006 09:14:51 -0400 (EDT) (envelope-from jhb@freebsd.org) From: John Baldwin To: freebsd-current@freebsd.org Date: Tue, 15 Aug 2006 09:10:50 -0400 User-Agent: KMail/1.9.1 References: <20060814170418.GA89686@stud.fit.vutbr.cz> In-Reply-To: <20060814170418.GA89686@stud.fit.vutbr.cz> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200608150910.51434.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [192.168.0.1]); Tue, 15 Aug 2006 09:14:52 -0400 (EDT) X-Virus-Scanned: ClamAV 0.88.3/1662/Tue Aug 15 04:29:25 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 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 13:14:57 -0000 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. - 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 */ - 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. - 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. - In linux_proc_init() you pass EMUL_LOCKED to em_find() in the CLONE_VM case when the lock isn't held. - 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. - 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. - You should include the appropriate header to get the declaration of 'hz' rather than just adding an 'extern' directly in your code. - In futex_get() you don't handle the race of two concurrent creates. - In futex_sleep(), you probably should be using an interlocked sleep such as msleep() or cv_wait() to avoid lost wakeups. - In futex_sleep(), you should probably loop rather than recurse to avoid blowing the stack. - 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. - You should stick the sx locks in a header and not litter 'externs' in source files. - Should come up with a better name than 'schedtail' for the eventhandler in fork_exit(). Maybe 'process_fork_exit'? - 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. - 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. -- John Baldwin