I don't quite understand this assertion. What if vm_fault_quick_hold_pages() fails?

Is there any advantage to sizing the array this way vs. just having vm_page_t upages[NVME_MAX_PAGES]?

hmmm... Yea, I should. I misread some code. We have a couple of issues, though, in the tree that I misread that might need attention:

sys/contrib/vchiq/interface/vchiq_arm/vchiq_2835_arm.c

actual_pages = vm_fault_quick_hold_pages(&p->p_vmspace->vm_map,
    (vm_offset_t)buf, count,
    (type == PAGELIST_READ ? VM_PROT_WRITE : 0 ) | VM_PROT_READ, pages, num_pages);

if (actual_pages != num_pages) {
        vm_page_unhold_pages(pages, actual_pages);
        free(pagelist, M_VCPAGELIST);
        return (-ENOMEM);
}

vm_page_unhold_pages will loop forever given how it's coded (and be UB signed int underflow to boot). There's a few others that are sloppy that just return in this case.

and in the linuxulator we have:

count = vm_fault_quick_hold_pages(map, start, len, prot, pages, nr_pages);
return (count == -1 ? -EFAULT : nr_pages);

which needs help, no?

Anyway, I'll clean these up and post a review to understand this interface better.

Less stack usage almost all the time. Usually a lot less since npages is typically < 10.
But I had coded it as a malloc, then an alloc and then this construct. Maybe you're right. We're shallow in the stack here and have big stacks these days due to ZFS (which won't be on the stack either before or after)

The stack thing is also used in this oops from tcp_usrreq.c:

	                nheld = atop(round_page(((vm_offset_t)sbp) +
                            (vm_size_t)outsbsz) - trunc_page((vm_offset_t)sbp));
                        vm_page_t ma[nheld];
                        if (vm_fault_quick_hold_pages(
	                    &curproc->p_vmspace->vm_map, (vm_offset_t)sbp,
		            outsbsz, VM_PROT_READ | VM_PROT_WRITE, ma,
                            nheld) < 0) {
                                error = EFAULT;
		                break;
                        }

Ah, I knew I'd seen the assert pattern somewhere:

        ps->npages = vm_fault_quick_hold_pages(map, start, end - start,
            VM_PROT_WRITE, ps->pages, n);

        DDP_LOCK(toep);
	if (ps->npages < 0) {
                free(ps, M_CXGBE);
                return (EFAULT);
        }

        KASSERT(ps->npages == n, ("hold_aio: page count mismatch: %d vs %d",
            ps->npages, n));

in t4_ddp.c and I'm not sure what's going on in t4_cpl_io.c doesn't seem to check it has enough pages. And I'm not at all sure what to do wtih vfs_vnops.c which seems to know how many, but doesn't check.

Though looking at the vm_fault_quick_hold_pages() code, it looks like it will always return the count of pages or -1, so maybe all these examples could be cleaned up the other direction.

IMO it'd be a bit cleaner to compute max_pages here and return it by making the max_pages parameter an out-param. Or, have a helper function that just wraps vm_fault_quick_hold_pages() and returns the number of pages held or returns EFAULT or EIO if vm_fault_quick_hold_pages() fails or the user asked for too many pages, respectively.

Using malloc() is fine too, since we're in the user ioctl path here I believe. I just prefer not to have this kind of dynamic stack allocation when possible.

But you have to know how many pages to allocate for the array...
I'm not sure that I like either of these alternatives.

Except that we're trading one malloc for another. The whole point of eliminating vmapbuf is to avoid allocating a buf, which was a choke point in performance measurements one of our vendors ran. While it is the user ioctl path, people are pushing it faster and faster.

I don't see what's wrong with doing the stack allocation like this. Why does that give you heartburn?

Are all added headers needed?

I agree. The fixed size array takes exactly 1K of stack, which is large, but should be fine for top kernel code.

You still have to check whether nvme_npages(addr, len) > NVME_MAX_PAGES before calling vm_fault_quick_hold_pages(). Note that vm_fault_quick_hold_pages() panics the kernel if you don't provide a large enough array (which is kind of a silly interface to be fair).

Yes, we can be more friendly and just return the error in this case.

This is squashing both error cases (request too big; failed to fault pages) to EFAULT.

Yes. I know. In both places. I thought about it, though: if we don't have enough pages elsewhere, we can return this value. And if we made vm_fault_quick_hold_pages return an error in that case, the rest of the VM system would return EFAULT, and we'd not know... the only way around that is to check where we check for max_xfer_size. de-facto, max_xfer_size is more like 128k or 256k, so it will catch this issue there (in a rather unfortunately noisy way). I can add a second condition there (and in the other routine) as well. That would be marginally better at not too great a cost, I think.

I'll redo with kib's thing, but we do need check somewhere we have enough pages before we write to the array. Ideally, that would be an assert level check since it's easier to unwind earlier in the function. But we'll see how it turns out. I think the separate functions would be more complicated, though... I'll explore that, though, when i refactor for kib's new interface.

IMO the interface would be somewhat cleaner if the function returned 0/error, and take a pointer to *req to return the request.