From owner-freebsd-net Tue Dec 12 15:17:39 2000 From owner-freebsd-net@FreeBSD.ORG Tue Dec 12 15:17:34 2000 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from pike.osd.bsdi.com (pike.osd.bsdi.com [204.216.28.222]) by hub.freebsd.org (Postfix) with ESMTP id C604637B402 for ; Tue, 12 Dec 2000 15:17:33 -0800 (PST) Received: from foo.osd.bsdi.com (root@foo.osd.bsdi.com [204.216.28.137]) by pike.osd.bsdi.com (8.11.1/8.9.3) with ESMTP id eBCNHLE01735; Tue, 12 Dec 2000 15:17:21 -0800 (PST) (envelope-from jhb@foo.osd.bsdi.com) Received: (from jhb@localhost) by foo.osd.bsdi.com (8.11.1/8.11.0) id eBCNGZf20964; Tue, 12 Dec 2000 15:16:35 -0800 (PST) (envelope-from jhb) Message-ID: X-Mailer: XFMail 1.4.0 on FreeBSD X-Priority: 3 (Normal) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit MIME-Version: 1.0 In-Reply-To: <20001212143214.H2312@canonware.com> Date: Tue, 12 Dec 2000 15:16:35 -0800 (PST) Organization: BSD, Inc. From: John Baldwin To: Jason Evans Subject: Re: MEXT_IS_REF broken. Cc: net@FreeBSD.ORG, Bosko Milekic , Alfred Perlstein Sender: jhb@foo.osd.bsdi.com Sender: owner-freebsd-net@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org On 12-Dec-00 Jason Evans wrote: > On Tue, Dec 12, 2000 at 01:51:00AM -0800, Alfred Perlstein wrote: >> * Alfred Perlstein [001212 01:44] wrote: >> > grr... >> > >> > considering this: >> > >> > #define MEXT_IS_REF(m) ((m)->m_ext.ref_cnt->refcnt > 1) >> > >> > #define MEXT_REM_REF(m) do { \ >> > KASSERT((m)->m_ext.ref_cnt->refcnt > 0, ("m_ext refcnt < 0")); \ >> > atomic_subtract_long(&((m)->m_ext.ref_cnt->refcnt), 1); \ >> > } while(0) >> > >> > this: >> > >> > #define MEXTFREE(m) do { \ >> > struct mbuf *_mmm = (m); \ >> > \ >> > if (MEXT_IS_REF(_mmm)) \ >> > MEXT_REM_REF(_mmm); \ >> > >> > >> > is not mpsafe. we _NEED_ some type that allows atomic dec and test >> > for 0. >> >> Sorry, I didn't explain why this is broken, it looks safe because >> if it's 1 then only "we" reference it. This is incorrect: >> >> (m)->m_ext.ref_cnt->refcnt == 2 >> >> thread a thread b >> >> if (MEXT_IS_REF(m)) >> if (MEXT_IS_REF(m)) >> MEXT_REM_REF(m); >> MEXT_REM_REF(m); >> >> now... (m)->m_ext.ref_cnt->refcnt == 0. >> >> oops, now the destructor never gets called and we just leaked a >> mbuf cluster. > > The current macro: > --------------------------------------------------------------------------- >#define MEXTFREE(m) do { \ > struct mbuf *_mmm = (m); \ > \ > if (MEXT_IS_REF(_mmm)) \ > MEXT_REM_REF(_mmm); \ > else if (_mmm->m_ext.ext_type != EXT_CLUSTER) { \ > (*(_mmm->m_ext.ext_free))(_mmm->m_ext.ext_buf, \ > _mmm->m_ext.ext_args); \ > _MEXT_DEALLOC_CNT(_mmm->m_ext.ref_cnt); \ > } else { \ > _MCLFREE(_mmm->m_ext.ext_buf); \ > _MEXT_DEALLOC_CNT(_mmm->m_ext.ref_cnt); \ > } \ > _mmm->m_flags &= ~M_EXT; \ > } while (0) > --------------------------------------------------------------------------- > > A safe version: > --------------------------------------------------------------------------- >#define MEXTFREE(m) do { \ > struct mbuf *_mmm = (m); \ > \ > MEXT_REM_REF(_mmm); \ > if (!atomic_cmpset_long(&_mmm->m_ext.ref_cnt->refcnt, 0, 1)) { \ > /* \ > * Do not free; there are still references, or another \ > * thread is freeing. \ > */ \ > } else if (_mmm->m_ext.ext_type != EXT_CLUSTER) { \ > (*(_mmm->m_ext.ext_free))(_mmm->m_ext.ext_buf, \ > _mmm->m_ext.ext_args); \ > _MEXT_DEALLOC_CNT(_mmm->m_ext.ref_cnt); \ > } else { \ > _MCLFREE(_mmm->m_ext.ext_buf); \ > _MEXT_DEALLOC_CNT(_mmm->m_ext.ref_cnt); \ > } \ > _mmm->m_flags &= ~M_EXT; \ > } while (0) > --------------------------------------------------------------------------- > > What this does is have all threads unconditionally atomically decrement. > Then, in order to determine whether to clean up, the threads to an atomic > compare and set on the refcount, so that if it is 0, only one thread > manages to modify the refcount, which then signifies that it will do the > cleanup. > > It looks like this should work to me, without the need for an atomic > refcount type. Yes, it requires two atomic operations, but it avoids the > need for a mutex, which seems to be your main concern. Since two atomic operations are all that are done during a mutex acquire and release, and since at least in teh x86 case the major cost _is_ the locked bus cycles for atomic operations, using two operations is probably just as expensive as using a mutex. What is wrong with having an encapsulated type so that you can do: if (refcount_release(&_mmm->m_ext.ref_cnt)) { /* * free object */ } You can then implement reference counts however you wish. Not mention that atomic_cmpset_long() may be very expensive on some archs. > Jason -- John Baldwin -- http://www.FreeBSD.org/~jhb/ PGP Key: http://www.Baldwin.cx/~john/pgpkey.asc "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message