On 21.08.2013 21:40, Navdeep Parhar wrote:
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.
Next try: ;)
Index: kern/uipc_mbuf.c
===================================================================
--- kern/uipc_mbuf.c (revision 254605)
+++ kern/uipc_mbuf.c (working copy)
@@ -284,9 +284,12 @@
KASSERT(m->m_ext.ref_cnt != NULL, ("%s: ref_cnt not set", __func__));
/*
- * check if the header is embedded in the cluster
+ * Do not free if the mbuf is embedded in the cluster
+ * but make sure to get rid of any tag chains first.
*/
skipmbuf = (m->m_flags & M_NOFREE);
+ if (skipmbuf && (m->m_flags & M_PKTHDR))
+ m_tag_delete_chain(m, NULL);
/* Free attached storage if this mbuf is the only reference to it. */
if (*(m->m_ext.ref_cnt) == 1 ||
--
Andre
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
