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

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

(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