Re: PR/49328 CVS commit: src/sys/dev/pci/ixgbe

[christos%zoulas.com@localhost (Christos Zoulas)]

On Feb 1, 10:53am, chuq%chuq.com@localhost (Chuck Silvers) wrote:
-- Subject: Re: PR/49328 CVS commit: src/sys/dev/pci/ixgbe

| sure, adding an DIAGNOSTIC assertion somewhere to catch this would be
| an improvement.  I'm not sure how you'd actually make the check though.
| outside LOCKDEBUG we don't track what mutexes are held.
| it might work to say that pmap_growkernel() should only be called with
| no interrupts blocked, but I don't think there's an MI way to check that.

Yes, perhaps this is a worthwhile addition, I am not sure.

| well, pmap_growkernel() *is* the abstraction.  :-)
| if you'd like to add a UVM wrapper around it so that we can add MI assertions
| in just one place, that would be fine.  (it would be nice to have that for
| all of the pmap API really.)
| 
| the check in uvm_page.c is actually bogus:
| 
|                 uvm_maxkaddr = pmap_growkernel(addr + size);
|                 if (uvm_maxkaddr < (addr + size))
|                         panic("uvm_pageboot_alloc: pmap_growkernel() failed");
| 
| 
| but the manpage says:
| 
|                    The pmap_growkernel() function returns the new maximum
|                    kernel virtual address that can be mapped with the
|                    resources it has available.  If new resources cannot be
|                    allocated, pmap_growkernel() must panic.
| 
| a couple of the pmap implementations are a bit lax about the panic part.
| 
| but separate from that, most pmap_growkernel() implmentations use sleep locks,
| which is apparently intended to be legitimate.  I'd think it would be good if
| we continue to allow that, and it would be even better if we removed the
| requirement to panic and instead allowed pmap_growkernel() to sleep to wait
| for memory.  to avoid turning those panic cases into hangs, we might want to
| do something like add a check that the pagedaemon thread only sleeps when
| it's idle.  we could even allow the pagedaemon to sleep when there are pageouts
| already pending if we tighten up the drivers to not need to allocate any more
| memory to complete an I/O after the strategy call returns, but I suspect that
| lots of drivers don't guarantee that currently.

All sounds good to me.

There is also this:

        rc = vmem_alloc(vm, size, flags, &va);
        if (rc != 0)
                return rc;

#ifdef PMAP_GROWKERNEL
        /* 
         * These VA allocations happen independently of uvm_map
         * so this allocation must not extend beyond the current limit.
         */
        KASSERTMSG(uvm_maxkaddr >= va + size,
            "%#"PRIxVADDR" %#"PRIxPTR" %#zx",
            uvm_maxkaddr, va, size);
#endif

Which does not explain who failed to do the pmap_growkernel.


And here is the jxgbe.h patch to change the core lock not to block IPL_NET...

christos

Index: ixgbe.h
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/ixgbe/ixgbe.h,v
retrieving revision 1.1
diff -u -u -r1.1 ixgbe.h
--- ixgbe.h	12 Aug 2011 21:55:29 -0000	1.1
+++ ixgbe.h	1 Feb 2015 19:11:06 -0000
@@ -476,7 +476,7 @@
 
 
 #define IXGBE_CORE_LOCK_INIT(_sc, _name) \
-        mutex_init(&(_sc)->core_mtx, MUTEX_DEFAULT, IPL_NET)
+        mutex_init(&(_sc)->core_mtx, MUTEX_DEFAULT, IPL_NONE)
 #define IXGBE_CORE_LOCK_DESTROY(_sc)      mutex_destroy(&(_sc)->core_mtx)
 #define IXGBE_TX_LOCK_DESTROY(_sc)        mutex_destroy(&(_sc)->tx_mtx)
 #define IXGBE_RX_LOCK_DESTROY(_sc)        mutex_destroy(&(_sc)->rx_mtx)


christos