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

[Chuck Silvers <chuq%chuq.com@localhost>]

The following reply was made to PR kern/49328; it has been noted by GNATS.

From: Chuck Silvers <chuq%chuq.com@localhost>
To: Christos Zoulas <christos%zoulas.com@localhost>
Cc: 6bone%6bone.informatik.uni-leipzig.de@localhost, kern-bug-people%netbsd.org@localhost,
	netbsd-bugs%netbsd.org@localhost, tech-kern%netbsd.org@localhost
Subject: Re: PR/49328 CVS commit: src/sys/dev/pci/ixgbe
Date: Sun, 1 Feb 2015 10:53:58 -0800

 (moving this discussion from gnats mail to tech-kern...)
 
 On Sun, Feb 01, 2015 at 12:56:51PM -0500, Christos Zoulas wrote:
 > On Feb 1,  9:33am, chuq%chuq.com@localhost (Chuck Silvers) wrote:
 > -- Subject: Re: PR/49328 CVS commit: src/sys/dev/pci/ixgbe
 > 
 > | that is not legal.  pmap_growkernel() is not optional for kmem allocations
 > | which increase the maximum kmem address in use.
 > | 
 > | and before you try to dive in and make a nowait version of pmap_growkernel(),
 > | let's discuss if that's the way we want to resolve this.  it's currently
 > | illegal to allocate kernel memory (even with nowait/nosleep) while holding a
 > | spin mutex.  if we want to keep that restriction, then we just need to change
 > | this one driver to not do that.  if we want to relax that restriction,
 > | there a bunch of UVM and pmap changes needed.
 > 
 > I was not planning to do that (make a nowait version of pmap_growkernel)...
 > I was questioning exactly that: if a NOWAIT allocation should fail instead
 > of grabbing a mutex and only failing when LOCKDEBUG is active. Ideally code
 > that always fails in LOCKDEBUG mode, should not "work by accident" in regular
 > mode.
 
 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.
 
 
 > This pmap_growkernel code seems strange to me because it seems that it
 > is not allowed to fail (the result is not being checked in 2 of 3 places),
 > and perhaps the whole thing should be abstracted...
 
 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.
 
 -Chuck