M_NOFREE removal (was Re: svn commit: r254520 - in head/sys: kern sys)

[Navdeep Parhar]

On 08/21/13 11:18, Andre Oppermann wrote:
> On 21.08.2013 18:38, Navdeep Parhar wrote:
>> On 08/21/13 08:08, Andre Oppermann wrote:
>>> On 20.08.2013 00:38, Peter Grehan wrote:
>> <snip>
>>>
>>>>    If there's an alternative to M_NOFREE, I'd be more than happy to use
>>>> that.
>>>
>>> Set up your own (*ext_free) function and omit freeing of the mbuf
>>> itself.  Make
>>> sure to properly track your mbufs to avoid leaking them.
>>>
>>
>> How is this an alternate to M_NOFREE?  Your suggestion indicates that
>> you may have removed M_NOFREE thinking it did something other than what
>> it actually did.  And this suggestion doesn't even work -- note the
>> uma_zfree(zone_mbuf, m) at the end of mb_free_ext that would run after
>> any custom ext_free.
>>
>> To recap: M_NOFREE was the way to tell the mbuf subsystem that the mbuf
>> does not come from zone_mbuf.  Nothing more and nothing less.  The mbuf
>> was in other ways like any other mbuf and could have a pkthdr (or not),
>> internal storage (or not), external cluster (or not), etc. etc.
> 
> Mea culpa.  You're totally right.  I have mixed up my mental model with
> another working tree I carry along for quite some time.  In it (*ext_free)
> completely replaces mb_free_ext() making it very easy to keep the mbuf.
> We should move that way hopefully sooner than later, simplifying a couple
> of things including externally managed refcounts.
> 
> Sorry for the chaos and not getting it.
> 
> M_NOFREE is back with r254605.
> 

I saw that, thanks!

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.

Regards,
Navdeep