Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 13 Jun 2001 10:37:22 -0700 (PDT)
From:      Matthew Jacob <mjacob@feral.com>
To:        freebsd-current@freebsd.org
Subject:   MBUF locking- this can't be right, can it?
Message-ID:  <Pine.BSF.4.21.0106131033130.40934-100000@beppo.feral.com>

next in thread | raw e-mail | index | archive | help

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 majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0106131033130.40934-100000>