Rather than lifting out the vm_page_remove() calls, perhaps pass the iterator to sgx_page_remove() as well, and have it decide whether to call vm_page_remove() or vm_page_iter_remove() based on whether the iterator pointer is NULL.

Do you have some other uses for NOCREAT_LT planned? This function is called rarely (kernel stacks are cached by UMA and reused many times without going through this function), so the extra complexity feels a bit unnecessary.

Not quite related to this change, but once it lands, I'd like to add one 8-byte field to plinks. I believe that'd be enough to let us start embedding UMA slab headers (i.e. uma_slab_t) within the vm_page structure. This is an alternative approach to D22768.

Bikeshedding, but VM_RADIX_FOREACH_FROM reads a bit more naturally to me, and we already have TAILQ_FOREACH_FROM etc..

This is more expensive, but at some point I presume it should be possible to pass an interator down from the page allocator and use that instead of doing a full lookup here.

I have no other uses planned. I was just looking for a way to avoid a redundant lookup.

I don't object to using FROM, but the reason I didn't is that FOREACH_FROM takes two arguments, and this macro takes three. FOREACH_FROM assumes that the lookup has already happened and FOREACH_START has to do the lookup. Those differences are the reason that I chose to use a distinct name. But it doesn't matter much to me.

I agree that it would be possible.

Can't we use vm_radix_iter_next() here?

This can presumably be committed on its own. It looks reasonable to me.

The assertion can come first, as in dmar_domain_free_pgtbl().

Elsewhere you are careful to initialize the iterator outside the scope of the object lock, but not here.

Does it make sense to have a macro for this usage too? Perhaps we could use VM_RADIX_FOREACH and assert that pindices are consecutive?

I don't have a concrete suggestion to offer at the moment, but here we're doing some unnecessary work: vm_map_pmap_enter(), the sole caller of this function, uses an iterator to find a contiguous run of pindices that correspond to a run of valid pages; then, it passes the bounds of this run to pmap_enter_object(), which uses a separate iterator to re-lookup each page in the run. That seems redundant, and we should perhaps rework the pmap_enter_object() interface to avoid it.

One possibility would be modifying vm_map_pmap_enter() to store a fixed-size array of vm_page pointers on the stack, which gets passed to pmap_enter_object() once full or the end of a run is detected or a superpage is detected.

How about if vm_map_pmap_enter stops trying to find a max page range between calls to pmap_enter_object, and instead just calls it every time vm_page_all_valid is true, passing along the start and psind vals? The pmap_lock would be acquired once, before the loop in vm_map_pmap_enter, and released once, and pmap_enter_object wouldn't have a loop at all. Since I don't know what the rwlock is about, I don't know how the potential extra locking/unlocking of the rwlock would affect things.

Cancel the previous idea.

Can we use p->plinks.s to build a list of the valid pages, and pass that list to pmap_enter_object to avoid the second iterator? Using an array leads to a question of how big to make the array, where using plinks.s avoids that - assuming that someone else isn't using plinks for something while vm_map_pmap_enter is happening. I don't know whether I can safely assume that.

At the moment, we cannot use plinks.s: it's overlaid with plinks.q, the fields used to link pages together in the global page queues. The pages mapped here will in general be managed (which among other things means that they are reclaimable and thus are kept in these page queues), so plinks is already in use.

That said, in this diff you remove two pointer-sized fields from struct vm_page, and I would later like to add one back for use in UMA (the kernel memory allocator). We could make use of such a field here: that would not overlap with UMA's use of it.