De-virtualize V_pf_mtag_z to eliminate kernel panics.

[Craig Rodrigues]

Gleb,

Since you did a lot of work in GRN 240233
to fix PF issues, especially for VIMAGE, I thought I would
ask your opinion on the attached patch.

In this post:

http://lists.freebsd.org/pipermail/freebsd-virtualization/2013-July/001405.html

I reported multiple PF-related panics when VIMAGE was enabled
in my kernel config.

In these posts:
http://lists.freebsd.org/pipermail/freebsd-virtualization/2013-July/001413.html
http://lists.freebsd.org/pipermail/freebsd-virtualization/2013-July/001420.html

Marko Zec seemed to think that de-virtualizing V_pf_mtag_z
would be a valid solution to this problem, and that keeping
V_pf_mtag_z as a per-vnet variable is not necessary.

What do you think of Marko's comments, and this patch?

Thanks.
--
Craig
Index: sys/netpfil/pf/pf.c
===
--- sys/netpfil/pf/pf.c (revision 253346)
+++ sys/netpfil/pf/pf.c (working copy)
@@ -187,8 +187,7 @@
 
 static VNET_DEFINE(uma_zone_t, pf_sources_z);
 #defineV_pf_sources_z  VNET(pf_sources_z)
-static VNET_DEFINE(uma_zone_t, pf_mtag_z);
-#defineV_pf_mtag_z VNET(pf_mtag_z)
+uma_zone_t pf_mtag_z;
 VNET_DEFINE(uma_zone_t, pf_state_z);
 VNET_DEFINE(uma_zone_t, pf_state_key_z);
 
@@ -749,7 +748,7 @@
V_pf_altqs_inactive = V_pf_altqs[1];
 
/* Mbuf tags */
-   V_pf_mtag_z = uma_zcreate(pf mtags, sizeof(struct m_tag) +
+   pf_mtag_z = uma_zcreate(pf mtags, sizeof(struct m_tag) +
sizeof(struct pf_mtag), NULL, NULL, pf_mtag_init, NULL,
UMA_ALIGN_PTR, 0);
 
@@ -803,7 +802,7 @@
mtx_destroy(pf_overloadqueue_mtx);
mtx_destroy(pf_unlnkdrules_mtx);
 
-   uma_zdestroy(V_pf_mtag_z);
+   uma_zdestroy(pf_mtag_z);
uma_zdestroy(V_pf_sources_z);
uma_zdestroy(V_pf_state_z);
uma_zdestroy(V_pf_state_key_z);
@@ -827,7 +826,7 @@
 pf_mtag_free(struct m_tag *t)
 {
 
-   uma_zfree(V_pf_mtag_z, t);
+   uma_zfree(pf_mtag_z, t);
 }
 
 struct pf_mtag *
@@ -838,7 +837,7 @@
if ((mtag = m_tag_find(m, PACKET_TAG_PF, NULL)) != NULL)
return ((struct pf_mtag *)(mtag + 1));
 
-   mtag = uma_zalloc(V_pf_mtag_z, M_NOWAIT);
+   mtag = uma_zalloc(pf_mtag_z, M_NOWAIT);
if (mtag == NULL)
return (NULL);
bzero(mtag + 1, sizeof(struct pf_mtag));
___
freebsd-virtualization@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
freebsd-virtualization-unsubscr...@freebsd.org

Re: De-virtualize V_pf_mtag_z to eliminate kernel panics.

[Marko Zec]

On Saturday 27 July 2013 08:31:48 Craig Rodrigues wrote:
 Gleb,

 Since you did a lot of work in GRN 240233
 to fix PF issues, especially for VIMAGE, I thought I would
 ask your opinion on the attached patch.

 In this post:

 http://lists.freebsd.org/pipermail/freebsd-virtualization/2013-July/00140
5.html

 I reported multiple PF-related panics when VIMAGE was enabled
 in my kernel config.

 In these posts:
 http://lists.freebsd.org/pipermail/freebsd-virtualization/2013-July/00141
3.html
 http://lists.freebsd.org/pipermail/freebsd-virtualization/2013-July/00142
0.html

 Marko Zec seemed to think that de-virtualizing V_pf_mtag_z
 would be a valid solution to this problem, and that keeping
 V_pf_mtag_z as a per-vnet variable is not necessary.

 What do you think of Marko's comments, and this patch?

Hi Craig,

while in principle I agree with the intent to de-virtualize V_pf_mtag_z 
(after all, this was my suggestion in the first place), your proposed patch 
isn't the valid solution, since on each vnet creation you'll be clobbering 
(now global) variable pf_mtag_z, which would imminently cause double-free 
or similar issues later during runtime.  You should move

pf_mtag_z = uma_zcreate(...)

to some other function which isn't called for each vnet, or at least as a 
crude kludge, do this conditionally if (curvnet == vnet0).  The same goes 
for uma_zdestroy(pf_mtag_z)...

Cheers,

Marko
___
freebsd-virtualization@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
freebsd-virtualization-unsubscr...@freebsd.org

Re: De-virtualize V_pf_mtag_z to eliminate kernel panics.

[Adrian Chadd]

I'm happy keeping it virtual (it lets us do things like set per-vimage
mbuf tag limits, and having per-vimage mbufs may be a useful long term
stretch goal to have).. we just have to think about this stuff in more
detail.



-adrian
___
freebsd-virtualization@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
freebsd-virtualization-unsubscr...@freebsd.org