Date: Sat, 14 Mar 2015 01:49:17 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: John-Mark Gurney <jmg@funkthat.com> Cc: freebsd-arch@freebsd.org Subject: Re: refcount_release_take_##lock Message-ID: <20150314004917.GD32157@dft-labs.eu> In-Reply-To: <20150314001526.GC32157@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>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150314004917.GD32157>