On Tue, Dec 12, 2000 at 01:51:00AM -0800, Alfred Perlstein wrote:
> * Alfred Perlstein <[EMAIL PROTECTED]> [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 [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
