vm_object: use pciters to remove pages

[dougm]

Summary

Rename vm_page_object_remove to vm_page_remove_radixdone, and remove
from it the responsibility for removing a page from its radix tree,
and pass that responsibility on to its callers.

For one of those callers, vm_page_rename, pass a pages pctrie_iter,
rather than a page, and use the iterator to remove the page from its
radix tree.

Define functions vm_page_iter_remove() and vm_page_iter_free() that
are like vm_page_remove() and vm_page_free(), respectively, except
that they take an iterator as parameter rather than a page, and use
the iterator to remove the page from the radix tree instead of
searching the radix tree. Function vm_page_iter_free() assumes that
the page is associated with an object, and calls
vm_page_free_object_prep to do the part of vm_page_free_prep that is
object-related, so that vm_page_free_prep can assume that objects
associated with pages are not iterator-related and can be removed from
the radix tree in the original way.

In functions vm_object_split and vm_object_collapse_scan, use a
pctrie_iter to walk over the pages of the object, and use
vm_radix_iter_remove to remove the page from the object tree without
searching for its location.

Similarly, rewrite vm_object_page_remove and _kmem_unback to use
iterators and vm_page_iter_free.

[dougm]

Process vm_object_collapse_scan similarly.

[dougm]

Apply iterators to vm_kern.c:_kmem_unback.

[dougm]

Define no-radix-tree-modification versions of vm_page_remove and vm_page_free and use them.

[dougm]

Also add an iterator version of lookup_le.

[dougm]

Update after vm_radix.h changes.

[markj]

sys/vm/vm_kern.c

659–660

The old code would have panicked if a page in the range wasn't resident, which is nice since that condition would have indicated a bug. Now we silently exit the loop in that case.

sys/vm/vm_object.c

1604

If I understand properly, first we drop the object reference from the page, then unbusy it, then free it to the physical memory allocator, then remove it from the tree. But doesn't that mean that there's a window where unlocked lookups can observe the unbusied, to-be-freed page?

[dougm]

Reorder deletion and tree removal in vm_object_split.

sys/vm/vm_kern.c

659–660

The iterator code uses "_next" to correspond to usages like vm_page_next, where the index is increased by 1 and the result could be either null or non-null. The iterator code uses "_step" to correspond to usages like TAILQ_NEXT(), where the index is increased by one or more to find a non-null value - except when there is no greater value. So I don't think there's a change here like you describe.

sys/vm/vm_object.c

1604

Yes, I should reorder these lines.

[markj]

sys/kern/subr_pctrie.c

1132 ↗

(On Diff #144660)

This part of the change should be committed first, on its own.

sys/vm/vm_kern.c

659–660

Right, vm_page_next() may return NULL if there is no page resident at the next pindex, but old code didn't check m != NULL in the loop header.

sys/vm/vm_object.c

1604

The same pattern exists in a few other functions below.

Then, rather than splitting vm_page_remove_notree(), I wonder if we can add a vm_page_remove_iter() which behaves like vm_page_remove(), except using the iterator to remove the page. In particular, I note that all calls to vm_page_remove_notree() are paired with a call to vm_radix_iter_remove().

sys/vm/vm_page.c

4181–4183

I find this decomposition kind of confusing. vm_page_object_remove() sounds like it removes the page from the radix tree, since the tree belongs to the object.

[dougm]

Rearrange into vm_page_iter_remove and vm_page_iter_free iter-based substitutes for the iter-less equivalents.

[dougm]

sys/kern/subr_pctrie.c

1132 ↗

(On Diff #144660)

After D47271, I won't need this part of the change.

sys/vm/vm_page.c

4181–4183

Then either vm_page_object_remove needs a new name, or it needs to have a (possibly NULL) iterator passed to it as a parameter always so that it can remain the only place that can remove a page from a radix tree. Are you suggesting that an iterator always be passed to it?

[markj]

sys/vm/vm_page.c

4181–4183

Maybe vm_page_reset() or vm_page_object_reset() would be a better name.

Passing an iterator also seems viable but it would also be nice to have an interface which doesn't require one, so as to keep simple those callers that won't benefit from the extra complexity of initializing and maintaining an interator.

[dougm]

Rename vm_page_object_remove.

[dougm]

Remove changes to subr_pctrie.c. Pass the iterator to vm_page_rename() to allow the page to be removed with the iterator and the appropriate page number.

[markj]

Overall this looks good to me.

sys/vm/vm_page.c

1403

Rather than having a boolean flag here, I would prefer to either duplicate the relevant portions of vm_page_free_toq() here and then set m->object = NULL before freeing the page, or factor the common code out into a subroutine called from here and from vm_page_free_toq(). Consider in particular that vm_object_terminate_single_page() plays similar games where it detaches the page from the object before freeing it.

I don't insist on it however, given that vm_page_free_toq() is private to vm_page.c.

2043–2044

This comment needs to be updated.

[dougm]

Update some comments.

sys/vm/vm_page.c

1403

I don't understand this suggestion well enough to implement it.

[dougm]

Drop the bool argument from vm_page_free_toq. Turn vm_page_free_toq into a wrapper for the real implementation function, to which it passes 'true' as the bool argument.

[dougm]

Avoid the object != NULL check in vm_page_iter_free.

[pho]

I completed a full stress2 test without seing any issues.

[alc]

sys/vm/vm_object.c

1984–1986

This new version using iterators takes on average 20% more cycles (over the course of a builtworld).

[alc]

sys/vm/vm_kern.c

652

This can be performed before the lock is acquired.

663

It takes one register-to-register move instruction to pass m. I don't see a good reason not to do so. Instead, a function call is being performed inside vm_page_iter_free().

[alc]

sys/vm/vm_kern.c

634–635

This is the one changed function that is faster.

sys/vm/vm_object.c

1520–1521

This new version using iterators takes on average 4% more cycles (over the course of a buildworld).