From owner-freebsd-arch@FreeBSD.ORG Sat Mar 14 00:49:24 2015 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 7CF5E2B2; Sat, 14 Mar 2015 00:49:24 +0000 (UTC) Received: from mail-we0-x230.google.com (mail-we0-x230.google.com [IPv6:2a00:1450:400c:c03::230]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 0339BF61; Sat, 14 Mar 2015 00:49:24 +0000 (UTC) Received: by wegp1 with SMTP id p1so2895155weg.1; Fri, 13 Mar 2015 17:49:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=Rb88us7MY+gscrWDR+boGIDDfqK2dlcjoKMeAex1E+E=; b=dKD0sIbIhI1JOR5aI5YdOpXVKJ5+fo2bU+DH7Wxt7qpkgZcG1RzIeL8mfIFfZ2XYAD 3bvc5M/whAozrm0mtSdwLA48NH3pBoJxeVNqETFnLslHeJulcDB/46fQeOQ7tOt18R41 UZMzb8Oqk4U+Rk1+W2Q/XROcouL/+IoC37AKZMc5L8KZL5nColSGvsISodmP67AXR+kh ejOAuN0PM5mzTsNCcb8N4w77SULy3UyhLyvKiVj0L7sQqwKDfPGIsyZmGFun8PvqBe67 59JoB18bPL6sRrJh5TGxZLbJr5L1nuSBHpsq1ahdpXVMrt0wwWtQqSdHUvpiTdizLWA1 RMuA== X-Received: by 10.180.9.134 with SMTP id z6mr23789686wia.81.1426294162095; Fri, 13 Mar 2015 17:49:22 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id vq9sm4947429wjc.6.2015.03.13.17.49.20 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 13 Mar 2015 17:49:21 -0700 (PDT) Date: Sat, 14 Mar 2015 01:49:17 +0100 From: Mateusz Guzik To: John-Mark Gurney Subject: Re: refcount_release_take_##lock Message-ID: <20150314004917.GD32157@dft-labs.eu> References: <20141025184448.GA19066@dft-labs.eu> <201410281413.58414.jhb@freebsd.org> <20141028193404.GB12014@dft-labs.eu> <201411111427.15407.jhb@freebsd.org> <20150313231607.GB32157@dft-labs.eu> <20150313235838.GM32288@funkthat.com> <20150314001526.GC32157@dft-labs.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150314001526.GC32157@dft-labs.eu> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-arch@freebsd.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 14 Mar 2015 00:49:24 -0000 On Sat, Mar 14, 2015 at 01:15:26AM +0100, Mateusz Guzik wrote: > On Fri, Mar 13, 2015 at 04:58:38PM -0700, John-Mark Gurney wrote: > > Mateusz Guzik wrote this message on Sat, Mar 14, 2015 at 00:16 +0100: > > > In the meantime I wrote a new version. > > > > > > Apart from locking-handling primitives this time we get > > > refcount_acquire_if_greater and refcount_release_if_greater helpers. > > > > I don't see how this is of any benefit... The smallest value you can > > provide is 0, which means the only time a reference can be obtained is > > if the caller already has a reference. If you don't have a reference > > (making it =0), it isn't safe to call this function on the object, as > > it could be free'd, and point to a different type of object... Even if > > you implement type-safe memory (which we shouldn't use more of), it's > > less than ideal, since you then have to check if the object is the same > > object you were expecting, and need to release it... > > > > The release_if is even more problematic IMO... > > > > I see I forgot to note the rationale in my e-mail. > > The kernel already uses 'refing an object with ref = 0' extensively in > vfs. > > For instance entering a name to the namecache does not increase hold > count of the vnode. > > A thread doing lookup locks the cache shared, locks the interlock, > unlocks the cache and calls vget which blindly vholds the vnode, > which quite often does transition 0->1. > > What prevents freeing of the vnode is name cache lock and later the > interlock. > > All v_holdcnt manipulation is done with the interlock held. Crucial > value changes are 0->1 and 1->0 and we need the lock here to ensure > consistency. > > However, as long as we modify this counter in a way which does not go > 0->1 nor 1->0 we don't have take the interlock and not doing so increases > scalability. > > So for instance in aforementioned case of namecache, the vnode is kept > stable by namecache lock and if v_holdcnt is >=1, we can increase it > without taking the interlock which I plan to do. > > But in order to do that I need primitives which wrap such functionality. > > Once more, stability of the object in question has to be ensured in > other manners. > > > After reading the previous discussion, I really don't like this. If > > this gets approved (others override my objection), we need some docs > > that say this should never be used, and it's use is only in the unsafe > > case where the containing data structure does NOT have a reference to > > the object. > > Well it should be quite obvious you can't just ref random objects. :> > One of the things i don't like is the fact that we don't have a refcount_t. If we did, we could do the following: with debug enabled it would be extended with a function pointer. On init you would: refcount_init(&refc, asserting_func); So that if you use 0->1 transition, you can provide a function which asserts that appropriate locks are held if you happen to trigger such a case. I doubt there is any runtime relible way to check that you could ref in general. Would this help with your concerns? In genearl, I don't see how you can go around 0->1 transitions in some cases without introducing a bunch of ugly code which only increases complexity. If you are so concerned that *_if functions can encourage refcount mismanage, we can put a big fat warning no problem. -- Mateusz Guzik