From owner-freebsd-hackers@FreeBSD.ORG Thu Aug 4 14:25:30 2005 Return-Path: X-Original-To: freebsd-hackers@freebsd.org Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 29E0E16A473 for ; Thu, 4 Aug 2005 14:25:30 +0000 (GMT) (envelope-from jhb@FreeBSD.org) Received: from mv.twc.weather.com (mv.twc.weather.com [65.212.71.225]) by mx1.FreeBSD.org (Postfix) with ESMTP id C3E2543D45 for ; Thu, 4 Aug 2005 14:25:29 +0000 (GMT) (envelope-from jhb@FreeBSD.org) Received: from [10.50.40.201] (Not Verified[65.202.103.25]) by mv.twc.weather.com with NetIQ MailMarshal (v6, 0, 3, 8) id ; Thu, 04 Aug 2005 10:40:03 -0400 From: John Baldwin To: hselasky@c2i.net Date: Thu, 4 Aug 2005 09:53:58 -0400 User-Agent: KMail/1.8 References: <200508030023.04748.hselasky@c2i.net> <200508031321.32276.jhb@FreeBSD.org> <200508041340.58851.hselasky@c2i.net> In-Reply-To: <200508041340.58851.hselasky@c2i.net> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200508040953.59316.jhb@FreeBSD.org> Cc: freebsd-hackers@freebsd.org Subject: Re: How to do proper locking 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: Thu, 04 Aug 2005 14:25:30 -0000 On Thursday 04 August 2005 07:40 am, Hans Petter Selasky wrote: > On Wednesday 03 August 2005 19:21, John Baldwin wrote: > > On Tuesday 02 August 2005 06:23 pm, Hans Petter Selasky wrote: > > > Hi, > > > > > > I am looking for a safe way to access structures that can be freed. The > > > solution I am looking for must not: > > > > > > - hinder scaleability > > > - lead to use of a single lock > > > - lead to lock order reversal > > There is one more thing. When the structure is freed it should not block > waiting for any reference counts. > > > These aren't a very clear set of requirements. > > If you think about the situation of accessing a structure in parallell with > freeing it, and want to do it 100 percent safe, you will see that you have > to do some kind of test before accessing this structure to see if it is > still present. So how should this test look like if you want to keep the > points I've listed above in mind? > > > Note that you can't hold a > > lock across malloc(), so your allocation code isn't safe. > > I was thinking that the M_NOWAIT flag was passed malloc(). Else in this > case, I think it is safe to unlock/lock "lock_A" when malloc is called. > > > You can try taking at look at some existing refcounted structures such > > as ucreds, p_args, etc. or looking at structures held in a global list > > like proc. > > It looks to me like some parent lock is held, or that these structures are > only accessed from a single thread, to prevent synchronization problems. You hold a parent lock (which is what your lock_A is if you think about it) while you bump the reference count, yes. > This is a copy and paste from the kernel sources: > > struct ucred * > crhold(struct ucred *cr) > { > The problem is, what happens if the kernel switches thread right here, and > then the other thread calls "crfree()" on this structure, that will > "free()" memory pointed to by "cr". Then the first line of code below will > access freed memory, and then we are going for a segment fault or worse. > > mtx_lock(cr->cr_mtxp); > cr->cr_ref++; > mtx_unlock(cr->cr_mtxp); > return (cr); > } crhold() is always called when the caller knows that it has a reference to cr that can't go away. Thus, in the case of p_ucred, one holds the associated PROC_LOCK() while doing crhold(). After the crhold() you can drop the proc lock, so that lock is only held for a very short period of time. You then have your own reference to the cred structure that is valid until you call crfree(). In the case of curthread->td_ucred, the reference is valid until curthread releases it on a cred update at the start of a syscall, so your reference is valid without needing locks for the life of a syscall. The key here is that you solve this problem at a higher level, not within the structure itself. > > What actual problem are you trying to solve? > > Generic callbacks: > > - setting up timers. Make sure that callback is never called after that > timer has been stopped. See callout_stop() and callout_drain(). callout_drain() won't return until the callout has both been stopped and has finished executing if it was already in progress. > - setting up devices. Make sure that read/write/ioctl/poll callbacks are > never called after that the device has been freed. This is managed with refcounts on the dev_t object by devfs. Once you call destroy_dev() devfs manages the rest. > - setting up interrupts. Make sure that interrupt routine is never called > after that it is been teared down. bus_teardown_intr() already does this. > And more. There are lots of situations in the kernel that run into the same > scheme, without a proper solution. Are there any other places you can think of that don't handle this already? The approach FreeBSD's kernel has taken is to solve this problem by duplicating a lot of checks all over the place for each data structure, but to solve it in the primitives themselves instead (hence callout_drain(), bus_teardown_intr(), destroy_dev(), etc.) -- John Baldwin <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve" = http://www.FreeBSD.org