This change increased the cycles spent in both vm_object_split() and vm_object_collapse_scan() during a buildworld. It appears that we're not moving enough pages to overcome the startup costs of the iterator.

Shouldn't this be abandoned?

Update in response to reviewer comments.

In D47829#1090358, @dougm wrote:

Comments in vm_reserv.c need to be updated to reflect this change.

If you have measured the improvement, how big is it?

Update in response to reviewer comments.

Let's limit this change to vm_page_iter_free. It reduces the average cycles in _kmem_unback and vm_object_page_remove by 7% and 5.3%, respectively.

By itself, this is going to be slower. This should be a part of a larger patch that removes the memq, so that we can evaluate whether the elimination of the memq makes up for the higher cost of the iterators.

Cycles to perform a 2MB aligned vm_page_alloc_contig() for shm_create_largepage() on a Ryzen 5900X:

x base
+ iter
+------------------------------------------------------------------------------+
|  +                                                             x             |
|  ++                                                            x             |
|  ++                                                           xx             |
| ++++        +                                                xxx xx         x|
||_MA___|                                                     |__MA____|       |
+------------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x  10         16813         17866       16989.5       17074.6     294.94715
+  10         12691         13496         12759       12833.1     239.58643
Difference at 95.0% confidence
        -4241.5 +/- 252.466
        -24.841% +/- 1.2701%
        (Student's t, pooled s = 268.696)

Is this related to the KASSERT that was recently added to vm_object_terminate_single_page?

In D47046#1079047, @dougm wrote:

In D47036, I put it after vm_radix_iter_step, mostly because PCTRIE_ITER_LOOKUP_LE comes shortly after PCTRIE_ITER_STEP_GE in pctrie.h. 'step' and 'jump' are built on top of lookup_ge, so I kept them grouped with lookup_ge.

I had a comment, and there should be one here for consistency with all the other little functions in this file.

I agree that we should have a sensible order to things, and that we don't. I suggest non-iter before iter, then insert before lookup before remove before replace before disposal, then lookup == before lookup >= before lookup <=.

To be clear, there is no question that this function will get used, so adding it sooner, rather than later, would be okay with me.

I'm looking at patches from both you and @dougm that define this function. However, each of you adds it in a different location within the file. :-)

Incorporating D47277 has increased the average number of cycles to perform vm_page_alloc() to 1325.

Incorporating D47277 yielded the lowest average cycles in vm_page_alloc_contig() that I've seen.

Switching from ..._lookup_le() to ..._lookup_lt() increased the average number of cycles in vm_page_alloc().

D47207 has reduced the average number of cycles to perform vm_page_alloc() from 1295 to 1265.

The test plan should say what the units are.

Can you separate out the function renames, and commit that first.

In D46945#1070648, @kib wrote:

In D46945#1070486, @markj wrote:

In D46945#1070436, @kib wrote:

In D46945#1070315, @markj wrote:

In D46945#1070223, @kib wrote:

Consider vm_page_grab_valid(). It allocates a page, and tries to read the page' content from pager. If the pager failed, the page is freed. I do not believe that the freed invalid page is enqueued there.

But there the page is freed immediately, not during object teardown, where the assertion is added.

The situation where invalid page was left on the object queue was quite common until recently.

Sure, but even in that case the page must still be added to a page queue, otherwise it is leaked until the VM object is destroyed. The assertion is meant to catch exactly this kind of bug.

Yes and yes. But it happens (happen), and the assert is too late to catch the moment.

I do not object against this change, only pointing out my experience.

There are too many things going on here at once. Can you generate a simple patch that fixes the correctness bug that Mark pointed out.

In D46620#1067113, @dougm wrote:

With this patch:
{F96917947}
I compared the cycles consumed freeing 1000000 pages without and with this patch. The results from ministat:

x old
+ new
+------------------------------------------------------------------------------+
|    +   ++ +++ ++   ++ +                              x x x  x x  x x x   x  +|
||_____________M____A__________________|                  |_____A_____|        |
+------------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x  12      11315490      11602387      11444362      11447090     88533.923
+  12      10575194      11651487      10713880      10792243     283272.81
Difference at 95.0% confidence
        -654847 +/- 177689
        -5.72064% +/- 1.54457%
        (Student's t, pooled s = 209859)

I don't see any correctness problems.

No, this is not what @markj meant.

This is much better.

Just out of curiosity, is this a first step in a larger set of changes?