Date: Thu, 14 Jun 2001 03:12:06 +0900 (JST) From: Hajimu UMEMOTO <ume@mahoroba.org> To: bmilekic@technokratis.com Cc: mjacob@feral.com, freebsd-current@FreeBSD.ORG Subject: Re: MBUF locking- this can't be right, can it? Message-ID: <20010614.031206.85718093.ume@mahoroba.org> In-Reply-To: <20010613134251.A8764@technokratis.com> References: <Pine.BSF.4.21.0106131033130.40934-100000@beppo.feral.com> <20010613134251.A8764@technokratis.com>
next in thread | previous in thread | raw e-mail | index | archive | help
>>>>> On Wed, 13 Jun 2001 13:42:51 -0400
>>>>> Bosko Milekic <bmilekic@technokratis.com> said:
bmilekic> Yes, I certainly didn't write that. I think it was KAME.
Yup, current KAME is based on 4.3-RELEASE which doesn't have
mtx_lock() issue. It is my mistake during merging it into 5-CURRENT.
The fix looks good to me. If there is no objection, I'll commit it.
bmilekic> On Wed, Jun 13, 2001 at 10:37:22AM -0700, Matthew Jacob wrote:
>
> 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
>
bmilekic> --
bmilekic> Bosko Milekic
bmilekic> bmilekic@technokratis.com
bmilekic> To Unsubscribe: send mail to majordomo@FreeBSD.org
bmilekic> with "unsubscribe freebsd-current" in the body of the message
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?20010614.031206.85718093.ume>