From owner-freebsd-arch@FreeBSD.ORG Sun Oct 26 06:43:01 2014 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 44D96B3; Sun, 26 Oct 2014 06:43:01 +0000 (UTC) Received: from mail-wi0-x236.google.com (mail-wi0-x236.google.com [IPv6:2a00:1450:400c:c05::236]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id AEBA11D6; Sun, 26 Oct 2014 06:43:00 +0000 (UTC) Received: by mail-wi0-f182.google.com with SMTP id d1so456220wiv.15 for ; Sat, 25 Oct 2014 23:42:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=N6aa71F3326OK2SG7tqZl29vPvewLtk8qOAXufQd4OM=; b=PYqThDoK/0PvMcHdjvPnxOB5nvaPyViFVaPWe6HgEB6M+vKNLUoC9cx7E0ChryiXF/ 8VKSxEwOyMqzjrXXdzgpYluMnyVyQyVwptcuaFYPmICUOfjdGIxzW6/PTK2gF/+VfzFw mlVXtUnUTAqDiCZDBCzf8bOjLkoazGKmz6neFpsD/E8xkeKPrQ2I++puwPDYbik9UTtP 8vJ4+8Xn4hV2cyfChrkmxWergtt+0qUk+vT5cG+4LazL9joPJmBSC4ST0fewwddK3mXz l1PhQp/z0HAnGGcPRJVfaKxhqkhRw1Fwm2KdVPLx7sd1XdBzONtmLIhoBzfbSzVZudFi TB0w== MIME-Version: 1.0 X-Received: by 10.194.192.161 with SMTP id hh1mr15577974wjc.72.1414305778801; Sat, 25 Oct 2014 23:42:58 -0700 (PDT) Sender: adrian.chadd@gmail.com Received: by 10.216.106.136 with HTTP; Sat, 25 Oct 2014 23:42:58 -0700 (PDT) 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> Date: Sat, 25 Oct 2014 23:42:58 -0700 X-Google-Sender-Auth: q4f-Gfzx0pjuCYMyQ25RXz1o9RA Message-ID: Subject: Re: refcount_release_take_##lock From: Adrian Chadd To: Ian Lepore Content-Type: text/plain; charset=UTF-8 Cc: John-Mark Gurney , Mateusz Guzik , "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: Sun, 26 Oct 2014 06:43:01 -0000 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 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"