From owner-freebsd-hackers@FreeBSD.ORG Tue Mar 27 03:58:01 2007 Return-Path: X-Original-To: hackers@freebsd.org Delivered-To: freebsd-hackers@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 6A7AD16A406 for ; Tue, 27 Mar 2007 03:58:01 +0000 (UTC) (envelope-from kris@obsecurity.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id 589F713C45A for ; Tue, 27 Mar 2007 03:58:01 +0000 (UTC) (envelope-from kris@obsecurity.org) Received: from obsecurity.dyndns.org (elvis.mu.org [192.203.228.196]) by elvis.mu.org (Postfix) with ESMTP id 3FB2B1A4D86; Mon, 26 Mar 2007 20:58:01 -0700 (PDT) Received: by obsecurity.dyndns.org (Postfix, from userid 1000) id 3786A513E1; Mon, 26 Mar 2007 23:58:00 -0400 (EDT) Date: Mon, 26 Mar 2007 23:57:59 -0400 From: Kris Kennaway To: Divacky Roman Message-ID: <20070327035759.GA47764@xor.obsecurity.org> References: <20070304141136.GA4935@stud.fit.vutbr.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20070304141136.GA4935@stud.fit.vutbr.cz> User-Agent: Mutt/1.4.2.2i Cc: hackers@freebsd.org Subject: Re: investigation of Giant usage in kernel X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Mar 2007 03:58:01 -0000 On Sun, Mar 04, 2007 at 03:11:36PM +0100, Divacky Roman wrote: > hi > > I looked at where Giant is held in the kernel and I found these interesting things: > > 1) in fs/fifofs/fifo_vnops.c we lock Giant when calling sorecieve()/sosend() > this is a bandaid for fixing a race that doesnt have to exist anymore. ie. > it needs some testing and can be remvoed I have removed this on my test machines and will see if the problem recurs. It is probably not necessary. > 2) in geom/geom_dev.c we lock Giant for: > > 2a) calling make_dev() which seems unecessary because make_dev protects > itself with devmtx > 2b) setting a flag in cdev which I think is unecessary as well I removed this in CVS. > 3) in kern/kern_descrip.c we lock Giant for the whole life of flock() I dont see > a need for this protection becuase > > 3a) access to fp is protected with FILE_LOCK()ing > 3b) otherwise it operates on local variables > 3c) it also calls VOP_* functions but those dont require Giant, right? I am not sure about this one. It might be tied in to 4). > 4) in kern/kern_lockf.c we lock Giant for the whole life of lf_advlock() I dont > think this is necessary because > > 4a) it operates on local variables > 4b) it calls various lf_*lock() functions which seems to either protect > themselves or not needed protection at all There is no synchronization between e.g. the tailq manipulation from different threads, so locking is needed. I have replaced this with a global lockf_mtx in my p4 branch but a finer grained solution is clearly desirable. This will probably involve rewriting or restructuring the code though. > 5) in kern/kern_time.c we lock Giant to protect > > 5a) tc_setclock() call - I dont know if this is necessary or not > 5b) lease_updatetime call which is a function pointer that seems to be > only assigned at one place (line 119 in kern_time.c) to a NOP function. > can this be removed? I guess you are protecting against two threads setting the clock at once. This does not seem to be a big deal, I don't imagine this will ever be in the critical path of anything. > 6) in vm/device_pager.c we lock Giant to (also) protect cdevsw->d_mmap but the device mmap > is either MPSAFE or assigned to giant_mmap (a wrapper that locks GIant and calls the original > d_mmap) so this seems unecessary. I'm not sure about this one either. Kris