Date: Tue, 12 Dec 2000 15:16:35 -0800 (PST) From: John Baldwin <jhb@FreeBSD.ORG> To: Jason Evans <jasone@canonware.com> Cc: net@FreeBSD.ORG, Bosko Milekic <bmilekic@technokratis.com>, Alfred Perlstein <bright@wintelcom.net> Subject: Re: MEXT_IS_REF broken. Message-ID: <XFMail.001212151635.jhb@FreeBSD.org> In-Reply-To: <20001212143214.H2312@canonware.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 12-Dec-00 Jason Evans wrote: > On Tue, Dec 12, 2000 at 01:51:00AM -0800, Alfred Perlstein wrote: >> * Alfred Perlstein <bright@wintelcom.net> [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 <jhb@FreeBSD.org> -- 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?XFMail.001212151635.jhb>