From owner-freebsd-net Tue Dec 12 14:32:36 2000 From owner-freebsd-net@FreeBSD.ORG Tue Dec 12 14:32:33 2000 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from magnesium.net (toxic.magnesium.net [207.154.84.15]) by hub.freebsd.org (Postfix) with SMTP id 0A4F337B404 for ; Tue, 12 Dec 2000 14:32:32 -0800 (PST) Received: (qmail 31281 invoked by uid 1142); 12 Dec 2000 22:32:30 -0000 Date: 12 Dec 2000 14:32:30 -0800 Date: Tue, 12 Dec 2000 14:32:15 -0800 From: Jason Evans To: Alfred Perlstein Cc: Bosko Milekic , net@FreeBSD.ORG, jhb@FreeBSD.ORG Subject: Re: MEXT_IS_REF broken. Message-ID: <20001212143214.H2312@canonware.com> References: <20001211014837.W16205@fw.wintelcom.net> <20001212014429.Y16205@fw.wintelcom.net> <20001212015059.Z16205@fw.wintelcom.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <20001212015059.Z16205@fw.wintelcom.net>; from bright@wintelcom.net on Tue, Dec 12, 2000 at 01:51:00AM -0800 Sender: jasone@magnesium.net Sender: owner-freebsd-net@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org 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. Jason To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message