Date: Wed, 21 Aug 2013 12:40:09 -0700 From: Navdeep Parhar <np@FreeBSD.org> To: Andre Oppermann <andre@freebsd.org> Cc: svn-src-head@freebsd.org, freebsd-net@freebsd.org, Peter Grehan <grehan@freebsd.org> Subject: Re: M_NOFREE removal (was Re: svn commit: r254520 - in head/sys: kern sys) Message-ID: <52151799.3020809@FreeBSD.org> In-Reply-To: <52151368.40803@freebsd.org> References: <201308191116.r7JBGsc6065793@svn.freebsd.org> <521256CE.6070706@FreeBSD.org> <5212587A.2080202@freebsd.org> <52128937.1010407@freebsd.org> <52129E55.30803@freebsd.org> <5214D7E8.1080106@freebsd.org> <5214ED19.40907@FreeBSD.org> <5215047C.8080107@freebsd.org> <521505A3.4020103@FreeBSD.org> <52151368.40803@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 08/21/13 12:22, Andre Oppermann wrote:
> On 21.08.2013 20:23, Navdeep Parhar wrote:
>> I believe we need an extra patch to get M_NOFREE correct. I've had it
>> forever in some of my internal repos but never committed it upstream
>> (just plain forgot). Since this stuff is fresh in your mind, can you
>> review this:
>>
>> diff -r cd78031b7885 sys/sys/mbuf.h
>> --- a/sys/sys/mbuf.h Fri Aug 16 13:35:26 2013 -0700
>> +++ b/sys/sys/mbuf.h Wed Aug 21 10:55:57 2013 -0700
>> @@ -535,6 +535,8 @@ m_free(struct mbuf *m)
>> {
>> struct mbuf *n = m->m_next;
>>
>> + if ((m->m_flags & (M_PKTHDR|M_NOFREE)) == (M_PKTHDR|M_NOFREE))
>> + m_tag_delete_chain(m, NULL);
>> if (m->m_flags & M_EXT)
>> mb_free_ext(m);
>> else if ((m->m_flags & M_NOFREE) == 0)
>>
>> It prevents leaks of tags from M_NOFREE+pkthdr mbufs.
>
> Good point. Looks correct.
>
> But then I wonder if it is really a smart thing to allow single
> mbufs (w/o M_EXT) to be M_NOFREE at the same time. They easily
> get lost. If it doesn't have an external buffer attached there
> is no way to know when and if it was freed.
>
> If M_NOFREE is only allowed together with M_EXT then the tag chain
> delete should happen in mb_ext_free() next to 'skipmbuf' instead.
>
> Index: kern/uipc_mbuf.c
> ===================================================================
> --- kern/uipc_mbuf.c (revision 254605)
> +++ kern/uipc_mbuf.c (working copy)
> @@ -283,11 +283,6 @@
> KASSERT((m->m_flags & M_EXT) == M_EXT, ("%s: M_EXT not set",
> __func__));
> KASSERT(m->m_ext.ref_cnt != NULL, ("%s: ref_cnt not set",
> __func__));
>
> - /*
> - * check if the header is embedded in the cluster
> - */
> - skipmbuf = (m->m_flags & M_NOFREE);
> -
> /* Free attached storage if this mbuf is the only reference to
> it. */
> if (*(m->m_ext.ref_cnt) == 1 ||
> atomic_fetchadd_int(m->m_ext.ref_cnt, -1) == 1) {
> @@ -328,8 +323,14 @@
> ("%s: unknown ext_type", __func__));
> }
> }
> - if (skipmbuf)
> +
> + /*
> + * Do not free if the mbuf is embedded in the cluster.
> + */
> + if (m->m_flags & M_NOFREE) {
> + m_tag_delete_chain(m, NULL);
> return;
> + }
The problem with this is that the mbuf may already be gone if it was
embedded in the cluster that was just freed by the ext free. That's why
skipmbuf is used to cache the M_NOFREE bit.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?52151799.3020809>