Re: uma_zalloc_arg: zone "256" with non-sleepable exclusive rw ifnet_rw @ /usr/src/sys/net/if.c:414

On Wednesday, January 19, 2011 9:51:59 am Sergey Kandaurov wrote:
> On 18 January 2011 22:04, John Baldwin <jhb_at_freebsd.org> wrote:
> > On Tuesday, January 18, 2011 1:22:24 pm Sergey Kandaurov wrote:
> >> On 18 January 2011 17:54, John Baldwin <jhb_at_freebsd.org> wrote:
> >> > On Monday, January 17, 2011 12:55:26 pm Sergey Kandaurov wrote:
> >> >> Hi,
> >> >>
> >> >> I see this "malloc with non-sleepable" on current during boot.
> >> >> It's strange that I don't see it if I boot via pxe/nfs.
> >> >>
> >> >> if_alloc() calls ifindex_alloc_locked() under IFNET_WLOCK() which
> >> >> might call if_grow().
> >> >> Looks like a regression from r196553.
> >> >
> >> > I'm guessing that ifindex_alloc() should drop the lock and retry the
> >> > allocation after calling if_grow()?  This compiles, but I haven't 
booted
> > it
> >> > yet:
> >>
> >> vnet_if_init() calls if_grow() without lock.
> >
> > So it does. :(  I've added locking to the sysinit to handle this:
> 
> Seems a bit more work there. The vnet_if_init() sysinit cannot use ifnet
> locking since it runs before another sysinit initialized those ifnet locks.
> On the other side it's safe to call if_grow() from vnet_if_init() w/o 
locking:
> 
> db> bt
> Tracing pid 0 tid 100000 td 0xffffffff80ccffc0
> _sx_xlock_hard() at _sx_xlock_hard+0xa0
> _sx_xlock() at _sx_xlock+0xbb
> vnet_if_init() at vnet_if_init+0x4a
> mi_startup() at mi_startup+0x77
> btext() at btext+0x2c
> 
> After said certain locking changes I came to the next problem.
> A modified ifindex_alloc_locked() function doesn't update/increment 
V_if_index
> in successful if_grow() case, so it ends up in quick memory exhaustion:
> 
> bce0: <Broadcom NetXtreme II BCM5709 1000Base-T (C0)> mem
> 0x92000000-0x93ffffff irq 28 at device 0.0 on pci4
> panic: kmem_malloc(-2147483648): kmem_map too small: 1110716416 total 
allocated
> cpuid = 0
> KDB: enter: panic
> [ thread pid 0 tid 100000 ]
> Stopped at      kdb_enter+0x3d: movq    $0,0x700140(%rip)
> db> bt
> Tracing pid 0 tid 100000 td 0xffffffff80ccffc0
> kdb_enter() at kdb_enter+0x3d
> panic() at panic+0x180
> kmem_malloc() at kmem_malloc+0x25d
> uma_large_malloc() at uma_large_malloc+0x4a
> malloc() at malloc+0x15d
> if_grow() at if_grow+0x98
> if_alloc() at if_alloc+0xd8
> bce_attach() at bce_attach+0x18de
> [...]
> > show malloc
>    ifnet            2      1048578K           25
> 
> So I added V_if_index increment into if_grow() itself.
> At least it boots now :)

Ah, the increment belongs in ifindex_alloc_locked() not if_grow(), but that 
was a bug certainly. :)  The issue is that we should only grow if the new 
index would exceed V_if_indexlim, not V_if_index and that should fix it.  For 
the locking, I just swapped the order of the SYSINIT's so that the locks are 
initialized before the various data structures.

Index: if.c
===================================================================
--- if.c	(revision 217544)
+++ if.c	(working copy)
_at__at_ -266,6 +266,7 _at__at_ ifindex_alloc_locked(u_short *idxp)
 
 	IFNET_WLOCK_ASSERT();
 
+retry:
 	/*
 	 * Try to find an empty slot below V_if_index.  If we fail, take the
 	 * next slot.
_at__at_ -278,10 +279,12 _at__at_ ifindex_alloc_locked(u_short *idxp)
 	/* Catch if_index overflow. */
 	if (idx < 1)
 		return (ENOSPC);
+	if (idx >= V_if_indexlim) {
+		if_grow();
+		goto retry;
+	}
 	if (idx > V_if_index)
 		V_if_index = idx;
-	if (V_if_index >= V_if_indexlim)
-		if_grow();
 	*idxp = idx;
 	return (0);
 }
_at__at_ -351,10 +354,12 _at__at_ vnet_if_init(const void *unused __unused)
 
 	TAILQ_INIT(&V_ifnet);
 	TAILQ_INIT(&V_ifg_head);
+	IFNET_WLOCK();
 	if_grow();				/* create initial table */
+	IFNET_WUNLOCK();
 	vnet_if_clone_init();
 }
-VNET_SYSINIT(vnet_if_init, SI_SUB_INIT_IF, SI_ORDER_FIRST, vnet_if_init,
+VNET_SYSINIT(vnet_if_init, SI_SUB_INIT_IF, SI_ORDER_SECOND, vnet_if_init,
     NULL);
 
 /* ARGSUSED*/
_at__at_ -365,7 +370,7 _at__at_ if_init(void *dummy __unused)
 	IFNET_LOCK_INIT();
 	if_clone_init();
 }
-SYSINIT(interfaces, SI_SUB_INIT_IF, SI_ORDER_SECOND, if_init, NULL);
+SYSINIT(interfaces, SI_SUB_INIT_IF, SI_ORDER_FIRST, if_init, NULL);
 
 
 #ifdef VIMAGE
_at__at_ -385,16 +390,25 _at__at_ VNET_SYSUNINIT(vnet_if_uninit, SI_SUB_INIT_IF, SI_
 static void
 if_grow(void)
 {
+	int oldlim;
 	u_int n;
 	struct ifindex_entry *e;
 
-	V_if_indexlim <<= 1;
-	n = V_if_indexlim * sizeof(*e);
+	IFNET_WLOCK_ASSERT();
+	oldlim = V_if_indexlim;
+	IFNET_WUNLOCK();
+	n = (oldlim << 1) * sizeof(*e);
 	e = malloc(n, M_IFNET, M_WAITOK | M_ZERO);
+	IFNET_WLOCK();
+	if (V_if_indexlim != oldlim) {
+		free(e, M_IFNET);
+		return;
+	}
 	if (V_ifindex_table != NULL) {
 		memcpy((caddr_t)e, (caddr_t)V_ifindex_table, n/2);
 		free((caddr_t)V_ifindex_table, M_IFNET);
 	}
+	V_if_indexlim <<= 1;
 	V_ifindex_table = e;
 }
 

-- 
John Baldwin