From owner-freebsd-current  Wed Jun 13 10:37:27 2001
Delivered-To: freebsd-current@freebsd.org
Received: from beppo.feral.com (beppo.feral.com [192.67.166.79])
	by hub.freebsd.org (Postfix) with ESMTP id 7A0F537B401
	for <freebsd-current@freebsd.org>; Wed, 13 Jun 2001 10:37:23 -0700 (PDT)
	(envelope-from mjacob@feral.com)
Received: from beppo (mjacob@beppo [192.67.166.79])
	by beppo.feral.com (8.11.3/8.11.3) with ESMTP id f5DHbMg41021
	for <freebsd-current@freebsd.org>; Wed, 13 Jun 2001 10:37:22 -0700 (PDT)
	(envelope-from mjacob@feral.com)
Date: Wed, 13 Jun 2001 10:37:22 -0700 (PDT)
From: Matthew Jacob <mjacob@feral.com>
Reply-To: 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>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: owner-freebsd-current@FreeBSD.ORG
Precedence: bulk
List-ID: <freebsd-current.FreeBSD.ORG>
List-Archive: <http://docs.freebsd.org/mail/> (Web Archive)
List-Help: <mailto:majordomo?subject=help> (List Instructions)
List-Subscribe: <mailto:majordomo?subject=subscribe%20freebsd-current>
List-Unsubscribe: <mailto:majordomo?subject=unsubscribe%20freebsd-current>
X-Loop: FreeBSD.ORG


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