On 12-Dec-00 Jason Evans wrote:
> 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.
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 <[EMAIL PROTECTED]> -- 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 [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
