Skip site navigation (1)Skip section navigation (2)
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>