MBUF locking- this can't be right, can it?

[Matthew Jacob]

A recent change to the MFREE macro was made as noted below:

/*
 * MFREE(struct mbuf *m, struct mbuf *n)
 * Free a single mbuf and associated external storage.
 * Place the successor, if any, in n.
 * 
 * we do need to check non-first mbuf for m_aux, since some of existing
 * code does not call M_PREPEND properly.
 * (example: call to bpf_mtap from drivers)
 */
#define MFREE(m, n) do {                                                \
        struct mbuf *_mm = (m);                                         \
                                                                        \
        KASSERT(_mm->m_type != MT_FREE, ("freeing free mbuf"));         \
        if (_mm->m_flags & M_EXT)                                       \
                MEXTFREE(_mm);                                          \
        mtx_lock(&mbuf_mtx);                                            \
        mbtypes[_mm->m_type]--;                                         \
++        if ((_mm->m_flags & M_PKTHDR) != 0 && _mm->m_pkthdr.aux) {      \
++                m_freem(_mm->m_pkthdr.aux);                             \
++                _mm->m_pkthdr.aux = NULL;                               \
++        }                                                               \
        _mm->m_type = MT_FREE;                                          \
        mbtypes[MT_FREE]++;                                             \
        (n) = _mm->m_next;                                              \
        _mm->m_next = mmbfree.m_head;                                   \
        mmbfree.m_head = _mm;                                           \
        MBWAKEUP(m_mballoc_wid, &mmbfree.m_starved);                    \
        mtx_unlock(&mbuf_mtx);                                          \
} while (0)


Unfortunately, m_freem also calls MFREE. Tsk. Now, it seems to me that even in
cases where you *could* allow recursive locks, the right idea here would be to
change it to:

/*
 * MFREE(struct mbuf *m, struct mbuf *n)
 * Free a single mbuf and associated external storage.
 * Place the successor, if any, in n.
 * 
 * we do need to check non-first mbuf for m_aux, since some of existing
 * code does not call M_PREPEND properly.
 * (example: call to bpf_mtap from drivers)
 */
#define MFREE(m, n) do {                                                \
        struct mbuf *_mm = (m);                                         \
        struct mbuf *_aux;                                              \
                                                                        \
        KASSERT(_mm->m_type != MT_FREE, ("freeing free mbuf"));         \
        if (_mm->m_flags & M_EXT)                                       \
                MEXTFREE(_mm);                                          \
        mtx_lock(&mbuf_mtx);                                            \
        mbtypes[_mm->m_type]--;                                         \
        if ((_mm->m_flags & M_PKTHDR) != 0 && _mm->m_pkthdr.aux) {      \
                _aux = _mm->m_pkthdr.aux;                               \
                _mm->m_pkthdr.aux = NULL;                               \
        } else {                                                        \
                _aux = NULL;                                            \
        }                                                               \
        _mm->m_type = MT_FREE;                                          \
        mbtypes[MT_FREE]++;                                             \
        (n) = _mm->m_next;                                              \
        _mm->m_next = mmbfree.m_head;                                   \
        mmbfree.m_head = _mm;                                           \
        MBWAKEUP(m_mballoc_wid, &mmbfree.m_starved);                    \
        mtx_unlock(&mbuf_mtx);                                          \
        if (_aux)                                                       \
                m_freem(_aux);                                          \
} while (0)


Comments?

-matt



To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message