Date: Sat, 25 Oct 2014 23:42:58 -0700 From: Adrian Chadd <adrian@freebsd.org> To: Ian Lepore <ian@freebsd.org> Cc: John-Mark Gurney <jmg@funkthat.com>, Mateusz Guzik <mjguzik@gmail.com>, "freebsd-arch@freebsd.org" <freebsd-arch@freebsd.org> Subject: Re: refcount_release_take_##lock Message-ID: <CAJ-VmokmY8SRvHyxqkgdw9eaQCDuRz-9vsZ9YGuYS5bD40rdQQ@mail.gmail.com> In-Reply-To: <1414265035.12052.646.camel@revolution.hippie.lan> References: <20141025184448.GA19066@dft-labs.eu> <20141025190407.GU82214@funkthat.com> <1414265035.12052.646.camel@revolution.hippie.lan>
next in thread | previous in thread | raw e-mail | index | archive | help
This is exactly why refcount==0 should be the only prelude to freeing
the object. There should be no way to actually take a reference on an
object that has a refcount of 0, because (surprise) at this stage
noone is referencing it anymore.
Ie, once the refcount hits 0, this means that nothing references it at
all - including any data structures that may be storing it. For
example, if an rtentry is in a radix tree, its refcount should be 1 or
more, not 0.
It's the only way this can work.
(The net80211 stack suffers from this and I'm about to set it on fire
until I fix it. It's been a source of crashes for almost 6 years now.)
-adrian
On 25 October 2014 12:23, Ian Lepore <ian@freebsd.org> wrote:
> On Sat, 2014-10-25 at 12:04 -0700, John-Mark Gurney wrote:
>> Mateusz Guzik wrote this message on Sat, Oct 25, 2014 at 20:44 +0200:
>> > The following idiom is used here and there:
>> >
>> > int old;
>> > old = obj->ref;
>> > if (old > 1 && atomic_cmpset_int(&obj->ref, old, old -1))
>> > return;
>> > lock(&something);
>> > if (refcount_release(&obj->ref) == 0) {
>> > unlock(&something);
>> > return;
>> > }
>> > free up
>> > unlock(&something);
>> >
>> > ==========
>>
>> Couldn't this be better written as:
>> if (__predict_false(refcount_release(&obj->ref) == 0)) {
>
> Could you not get preempted at this point, whereupon another thread
> acquires then releases obj, deletes it because it keeps running through
> this point, then eventually your original thread wakes up, gets the
> lock, and dereferences the now-defunct obj pointer?
>
> (Also, I think that should be != 0, above?)
>
> -- Ian
>
>> lock(&something);
>> if (__predict_true(!obj->ref)) {
>> free up
>> }
>> unlock(&something);
>> }
>>
>> The reason I'm asking is that I changed how IPsec SA ref counting was
>> handled, and used something similar...
>>
>> My code gets rid of a branch, and is better in that it uses refcount
>> API properly, instead of using atomic_cmpset_int...
>>
>> > I decided to implement it as a common function.
>> >
>> > We have only refcount.h and I didn't want to bloat all including code
>> > with additional definitions and as such I came up with a macro that has
>> > to be used in .c file and that will define appropriate inline func.
>> >
>> > I'm definitely looking for better names for REFCOUNT_RELEASE_TAKE_USE_
>> > macro, assuming it has to stay.
>>
>> You could shorten it to REFCNT_REL_TAKE_
>>
>> > Comments?
>>
>> Will you update the refcount(9) man page w/ documentation before
>> committing?
>>
>
>
> _______________________________________________
> freebsd-arch@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-VmokmY8SRvHyxqkgdw9eaQCDuRz-9vsZ9YGuYS5bD40rdQQ>