Revise a comment.

In D58917#1352088, @kib wrote:

Does arm64 use recursive mapping? If yes, could the function be used on recursive mapping?

Move td = curthread; out of the critical section.

In D58708#1348047, @markj wrote:

In D58708#1347933, @alc wrote:

I ran a dozen -j8 buildworlds, each starting from an empty /usr/obj, with MALLOC_PRODUCTION enabled and LLVM assertions disabled on a GENERIC-NODEBUG kernel. Here is Claude's analysis of the data:

I wonder where these (ranged) invalidations are coming from, as I don't see very many when building on amd64. I guess they are mostly caused by break-before-make sequences triggered by L3C and L2 superpage promotion and demotion. I noticed that on amd64, munmap() will not trigger a ranged invalidation when unmapping a run of PTEs, we just call pmap_invalidate_all(). On arm64 this doesn't appear to be the case, it will use ranged invalidations. I wonder what fraction of ranged invalidations come from munmap() vs. promotion/demotion.

I'm waiting to hear if @andrew has any comments or questions.

As an aside, as the appearance of the word "harness" in the previous comment suggests, when I had Claude reviewing the changes, it actually built a user-space test harness containing the loop to check that every page in the range would be the target of an invalidation. And, it temporarily introduced a few bugs in the loop to check that those bugs were detected. Just thought this was interesting.

I ran a dozen -j8 buildworlds, each starting from an empty /usr/obj, with MALLOC_PRODUCTION enabled and LLVM assertions disabled on a GENERIC-NODEBUG kernel. Here is Claude's analysis of the data:

The change is a win.
		before		after		delta
sys		1,070.67	1,012.58	−58.1 s (−5.43%)
user		23,801.46	23,816.08	+14.6 s (+0.06%)
user+sys	24,872.13	24,828.66	−43.5 s (−0.17%)
wall		3,221.63 (53:41.63)	3,214.40 (53:34.40)	−7.2 s (−0.22%)

In D58723#1347638, @markj wrote:

Possibly vfp_restore_state_common() should rely on its caller to enter a critical section (with an assert).

While the conversion from a macro to an inline pmap_s1_invalidate_loop() had no real effect, just a couple instructions changed, pmap_s1_invalidate_strided() is not being inlined in some places due to its size. In that case, we are not benefiting from final_only always being a constant, but I think that is something for a different patch to deal with.

Convert macro to __always_inline function.

I'm surprised that we didn't see this on arm64.

Just out of curiosity, where are the pmap changes?

In D56432#1294557, @markj wrote:

In D56432#1294556, @kib wrote:

Effectively, you agree that we can drop the support and disable passing VM_ALLOC_ZERO to vm_page_grab_unlocked?

After kern_kexec is fixed, yes. But now I am trying to understand what that code is doing, and failing.

In D56432#1292906, @kib wrote:

I thought what could be a use for such call, and I really do not see how it can be useful. Both vnode and swap objects might have the page content on the volume, so the invalid page we found on the queue cannot be safely zeroed without consulting pager first. In the end, the decision to zero such page belongs to the caller.

For other object types, like OBJT_PHYS in kern_kexec.c, it is still somewhat useful since we do not have a vm_page_alloc_pages(), so pages have to be allocated one-by-one. But yes, also note that vm_page_grab_valid() does not handle VM_ALLOC_ZERO.

In D56458#1293651, @kevans wrote:

I'm going to plan to commit this within the next day or so. I think linux's use of "stride" instead of size or shift for their naming of the bit is very telling, and their general range invalidation function seems to specifically just use PTE stride as well.

"... pmap locks, than witness ..." -> "... pmap locks, then witness ..."

@markj Do we know if there is actually a mix of clean and dirty pages being mapped?

Specifically, for now, I would commit the version in Diff 168641.

At this point, I would go back to the simpler, O(n) version, and commit that.

The changes here have led the compiler to unroll the first and final loops, making the machine code larger.

In D54365#1242517, @kib wrote:

In D54365#1242424, @alc wrote:

Curiously, the blogger's previous post contains a link to this page

https://codegolf.stackexchange.com/questions/478/free-a-binary-tree/489#489P

which contains almost the same code as the 3rd ranked solution.

Allow me to reformulate, you mean that the algorithm presented is not unique and is relatively wide known, so I can put the proper FF copyright and license without causing the authorship issue. Am I right?

Curiously, the blogger's previous post contains a link to this page

I guess that this review didn't get automatically closed?

In D53965#1241686, @kib wrote:

In D53965#1241683, @alc wrote:

This change is now similar to D43263. Abandon this one?

I suspect that either the comment is mis-directed, or the referenced review has the wrong number.

Indeed, it should point to D54263

This change is now similar to D43263. Abandon this one?

P.S. The semester is winding down, so I have more spare time for reviews now.

In D54268#1240309, @markj wrote:

In D54268#1240216, @alc wrote:

Since we are starting to expand the usage of vmem, I want to point out something that I only noticed around the time we added support for nextfit allocation. What we call bestfit is really an implementation of firstfit allocation. (A comment pointing this out was actually deleted from the Netbsd original when vmem was imported.) And, what we call firstfit is what, I believe, Solaris calls instantfit. We have no actual implementation of bestfit allocation. That said, mav's long ago change to "create own free list for each of the first 32 possible allocation sizes" has the effect of bestfit allocation for small sizes.

Here is the comment that got deleted:

	} else { /* VM_BESTFIT */
		/*
		 * we assume that, for space efficiency, it's better to
		 * allocate from a smaller block.  thus we will start searching
		 * from the lower-order list than VM_INSTANTFIT.
		 * however, don't bother to find the smallest block in a free
		 * list because the list can be very long.  we can revisit it
		 * if/when it turns out to be a problem.

If I understand correctly, libuvmem was motivated by a need for an address space allocator for PCI BARs in bhyve. I think a proper M_BESTFIT probably isn't required there, but it might be good to implement one nonetheless. As a first step, perhaps we should restore the naming from Solaris.

Since we are starting to expand the usage of vmem, I want to point out something that I only noticed around the time we added support for nextfit allocation. What we call bestfit is really an implementation of firstfit allocation. (A comment pointing this out was actually deleted from the Netbsd original when vmem was imported.) And, what we call firstfit is what, I believe, Solaris calls instantfit. We have no actual implementation of bestfit allocation. That said, mav's long ago change to "create own free list for each of the first 32 possible allocation sizes" has the effect of bestfit allocation for small sizes.

In D53891#1231869, @kib wrote:

In D53891#1231852, @markj wrote:

In D53891#1231400, @kib wrote:

In D53891#1231399, @alc wrote:

Can you elaborate on what you saw when debugging this problem? In particular, whether the map entries had OBJ_ONEMAPPING set? In principle, the ref_count and size shouldn't be a concern if the object has OBJ_ONEMAPPING set. For example, suppose there is an anonymous mapping (with OBJ_ONEMAPPING set) for the range [A, D). Further, suppose we punch a hole in the middle of that mapping, by calling munmap() on the range [B, C) where A < B < C < D. Now, we have two mappings, [A, B) and [C, D), that both reference the original object, and that object should still have OBJ_ONEMAPPING set. Because OBJ_ONEMAPPING is set, the munmap() should have freed any physical pages and swap space from the object that fell within the range [B, C). So, if a new anonymous mapping is created starting at either B or D, we should be able to safely coalesce it.

I did not have access to the object state there (all debugging was done remotely).

The situation you described is one of the cases that concerned me. I am not sure that we have a guarantee that doing the coalesce on the object with OBJ_ONEMAPPING flag but ref_count > 1 would not corrupt some other mapping. We need to do vm_object_page_remove(), and in principle that could remove pages which belong to other fragment.

I believe OBJ_ONEMAPPING means, "each page in the object is mapped at most once", so in the case you describe, OBJ_ONEMAPPING should not be set to begin with.

But this is exactly the part of my concern: even if OBJ_ONEMAPPING is not set, other conditions would allow the coalesce.

In D53891#1231852, @markj wrote:

In D53891#1231400, @kib wrote:

In D53891#1231399, @alc wrote:

Can you elaborate on what you saw when debugging this problem? In particular, whether the map entries had OBJ_ONEMAPPING set? In principle, the ref_count and size shouldn't be a concern if the object has OBJ_ONEMAPPING set. For example, suppose there is an anonymous mapping (with OBJ_ONEMAPPING set) for the range [A, D). Further, suppose we punch a hole in the middle of that mapping, by calling munmap() on the range [B, C) where A < B < C < D. Now, we have two mappings, [A, B) and [C, D), that both reference the original object, and that object should still have OBJ_ONEMAPPING set. Because OBJ_ONEMAPPING is set, the munmap() should have freed any physical pages and swap space from the object that fell within the range [B, C). So, if a new anonymous mapping is created starting at either B or D, we should be able to safely coalesce it.

I did not have access to the object state there (all debugging was done remotely).

The situation you described is one of the cases that concerned me. I am not sure that we have a guarantee that doing the coalesce on the object with OBJ_ONEMAPPING flag but ref_count > 1 would not corrupt some other mapping. We need to do vm_object_page_remove(), and in principle that could remove pages which belong to other fragment.

I believe OBJ_ONEMAPPING means, "each page in the object is mapped at most once", so in the case you describe, OBJ_ONEMAPPING should not be set to begin with.

In D53891#1231456, @kib wrote:

In D53891#1231437, @mmel wrote:

After another day of extensive testing, I can confirm that everything is working properly.

I'm happy to perform additional debugging to clarify the problematic condition, should you find it helpful.
Would you like a dump of the affected object for the newly added goto remove_pager case? Or anything else?

Try just this part of the series alone. Do not enable vm_check_pg_zero. Does it help?

diff --git a/sys/vm/vm_object.c b/sys/vm/vm_object.c
index 9d09224e7346..8850a1b8644e 100644
--- a/sys/vm/vm_object.c
+++ b/sys/vm/vm_object.c
@@ -1988,7 +1988,7 @@ vm_object_page_remove(vm_object_t object, vm_pindex_t start, vm_pindex_t end,
       (options & (OBJPR_CLEANONLY | OBJPR_NOTMAPPED)) == OBJPR_NOTMAPPED,
       ("vm_object_page_remove: illegal options for object %p", object));
   if (object->resident_page_count == 0)
-    return;
+     goto remove_pager;
   vm_object_pip_add(object, 1);
   vm_page_iter_limit_init(&pages, object, end);
 again:
@@ -2061,6 +2061,7 @@ vm_object_page_remove(vm_object_t object, vm_pindex_t start, vm_pindex_t end,
   }
   vm_object_pip_wakeup(object);
 
+remove_pager:
   vm_pager_freespace(object, start, (end == 0 ? object->size : end) -
       start);
 }

In D53891#1231619, @mmel wrote:

Confirmed, this part is enough.

Can you elaborate on what you saw when debugging this problem? In particular, whether the map entries had OBJ_ONEMAPPING set? In principle, the ref_count and size shouldn't be a concern if the object has OBJ_ONEMAPPING set. For example, suppose there is an anonymous mapping (with OBJ_ONEMAPPING set) for the range [A, D). Further, suppose we punch a hole in the middle of that mapping, by calling munmap() on the range [B, C) where A < B < C < D. Now, we have two mappings, [A, B) and [C, D), that both reference the original object, and that object should still have OBJ_ONEMAPPING set. Because OBJ_ONEMAPPING is set, the munmap() should have freed any physical pages and swap space from the object that fell within the range [B, C). So, if a new anonymous mapping is created starting at either B or D, we should be able to safely coalesce it.