Date: Tue, 12 Dec 2000 14:32:15 -0800 From: Jason Evans <jasone@canonware.com> To: Alfred Perlstein <bright@wintelcom.net> Cc: Bosko Milekic <bmilekic@technokratis.com>, net@FreeBSD.ORG, jhb@FreeBSD.ORG Subject: Re: MEXT_IS_REF broken. Message-ID: <20001212143214.H2312@canonware.com> In-Reply-To: <20001212015059.Z16205@fw.wintelcom.net>; from bright@wintelcom.net on Tue, Dec 12, 2000 at 01:51:00AM -0800 References: <20001211014837.W16205@fw.wintelcom.net> <Pine.BSF.4.21.0012111223350.21769-100000@jehovah.technokratis.com> <20001212014429.Y16205@fw.wintelcom.net> <20001212015059.Z16205@fw.wintelcom.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Jason
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?20001212143214.H2312>