svn commit: r368523 - head/sys/vm

[Mark Johnston]

On Tue, Dec 15, 2020 at 01:59:22PM +0100, Hans Petter Selasky wrote:
> On 12/10/20 9:44 PM, Bryan Drewery wrote:
> > Author: bdrewery
> > Date: Thu Dec 10 20:44:29 2020
> > New Revision: 368523
> > URL: https://svnweb.freebsd.org/changeset/base/368523
> > 
> > Log:
> >    contig allocs: Don't retry forever on M_WAITOK.
> >    
> >    This restores behavior from before domain iterators were added in
> >    r327895 and r327896.
> >    
> >    The vm_domainset_iter_policy() will do a vm_wait_doms() and then
> >    restart its iterator when M_WAITOK is set.  It will also force
> >    the containing loop to have M_NOWAIT.  So we get an unbounded
> >    retry loop rather than the intended bounded retries that
> >    kmem_alloc_contig_pages() already handles.
> >    
> >    This also restores M_WAITOK to the vmem_alloc() call in
> >    kmem_alloc_attr_domain() and kmem_alloc_contig_domain().
> >    
> >    Reviewed by:	markj, kib
> >    MFC after:	2 weeks
> >    Sponsored by:	Dell EMC
> >    Differential Revision:	https://reviews.freebsd.org/D27507
> > 
> > Modified:
> >    head/sys/vm/vm_kern.c
> > 
> > Modified: head/sys/vm/vm_kern.c
> > ==============================================================================
> > --- head/sys/vm/vm_kern.c	Thu Dec 10 20:44:05 2020	(r368522)
> > +++ head/sys/vm/vm_kern.c	Thu Dec 10 20:44:29 2020	(r368523)
> > @@ -264,9 +264,15 @@ kmem_alloc_attr_domainset(struct domainset *ds, vm_siz
> >   {
> >   	struct vm_domainset_iter di;
> >   	vm_offset_t addr;
> > -	int domain;
> > +	int domain, iflags;
> >   
> > -	vm_domainset_iter_policy_init(&di, ds, &domain, &flags);
> > +	/*
> > +	 * Do not allow the domainset iterator to override wait flags.  The
> > +	 * contiguous memory allocator defines special semantics for M_WAITOK
> > +	 * that do not match the iterator's implementation.
> > +	 */
> > +	iflags = (flags & ~M_WAITOK) | M_NOWAIT;
> > +	vm_domainset_iter_policy_init(&di, ds, &domain, &iflags);
> >   	do {
> >   		addr = kmem_alloc_attr_domain(domain, size, flags, low, high,
> >   		    memattr);
> > @@ -346,9 +352,15 @@ kmem_alloc_contig_domainset(struct domainset *ds, vm_s
> >   {
> >   	struct vm_domainset_iter di;
> >   	vm_offset_t addr;
> > -	int domain;
> > +	int domain, iflags;
> >   
> > -	vm_domainset_iter_policy_init(&di, ds, &domain, &flags);
> > +	/*
> > +	 * Do not allow the domainset iterator to override wait flags.  The
> > +	 * contiguous memory allocator defines special semantics for M_WAITOK
> > +	 * that do not match the iterator's implementation.
> > +	 */
> > +	iflags = (flags & ~M_WAITOK) | M_NOWAIT;
> > +	vm_domainset_iter_policy_init(&di, ds, &domain, &iflags);
> >   	do {
> >   		addr = kmem_alloc_contig_domain(domain, size, flags, low, high,
> >   		    alignment, boundary, memattr);
> > 
> 
> Hi,
> 
> Should "iflags" also be passed to kmem_alloc_contig_domain() !?

It is intentional, iflags is modified to ensure that the domainset
iterator does not loop until the allocation is successful, and
kmem_alloc_contig_pages() implements its own M_WAITOK handling.

> I'm seeing the following panic:
> 
> panic("vm_wait in early boot")
> vm_wait_domain()
> kmem_alloc_contig_pages()
> kmem_alloc_contig_domainset()
> kmem_alloc_contig()
> contigmalloc()
> x86bios_alloc()
> vesa_configure()
> vesa_mod_event()
> vesa_module_register_init()
> mi_startup()

Is it on a NUMA system?  I see that the new logic won't work properly if
there are empty domains, so this suggests that we really do need a
special contig iterator as discussed in the review.