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.

It has been a long time since I looked at this man page. In the interest of making sure that these changes were written in an style consistent with the rest of the man page, I decided to take a look, and having done so the following bits concern me:

In D52744#1205856, @andrew wrote:

In D52744#1205716, @alc wrote:

In D52744#1205060, @andrew wrote:

Note that the llsc instructions have some subtly when implementing fully sequentially consistent semantics. As the acquire and release are on different instructions this means memory operations may move into the sequence & be swapped. As we don't implement them in atomic(9) this shouldn't be an issue, however will be if we latter add them.

I vaguely recall Linux having an issue with the aforementioned behavior about 8-10 years ago in the implementation of a synchronization primitive.

I've known about the issue on Linux
This is the fix for Linux issue https://github.com/torvalds/linux/commit/8e86f0b409a44193f1587e87b69c5dcf8f65be67. It's not been a problem on FreeBSD as we don't have atomic operations with a full barrier in atomic(9).

In D52744#1205060, @andrew wrote:

On arm64 atomics with an acquire then later memory operations can move before the store, however the store needs to succeed as if it doesn't then we will execute the load-acquire again.

I don't think the CAS instructions allow this. The pseudo-code has the comment All observers in the shareability domain observe the following load and store atomically before listing the load and store operations. This seems to indicate if the load has been observed the store has too.

Note that the llsc instructions have some subtly when implementing fully sequentially consistent semantics. As the acquire and release are on different instructions this means memory operations may move into the sequence & be swapped. As we don't implement them in atomic(9) this shouldn't be an issue, however will be if we latter add them.

I am happy to see how much simpler this has gotten, particularly the elimination of the range lock.

In principle, I am okay with this change. However, I do have some questions.

I have been following this, but have not given it a careful reading. I should be able to do so over the weekend.

In D52165#1191863, @imp wrote:

Generally I like it, but the name is very long and specific for the logical concept of 'map this into the physical for I/O and keep it there'. But I have no better name, so I'll just grouse because I never could remember the order of the verbs when I was writing the nvme mapping code.

Eliminate unnecessary indirection.

In D49442#1163947, @markj wrote:

In D49442#1163641, @alc wrote:

I'm really on the fence about this. I could make a respectable argument for either approach.

Perhaps it's reasonable to simply implement both approaches? The patch in review makes some sense on its own, independent of the bug which motivated it: I can't see any practical reason for madvise(DONTNEED or FREE) to have any side effects on wired mappings. On the other hand, it seems sensible to modify the demotion path to handle this case, since we have no rule which says that wired mappings must have PG_A set, so this case could arise again in the future.

In D51431#1174792, @markj wrote:

I believe this makes D49442 unnecessary, at least as far as fixing the bug is concerned.

In D51345#1173699, @markj wrote:

In D51345#1173585, @kib wrote:

In D51345#1173169, @markj wrote:

In D51345#1172943, @kib wrote:

I am curious how does the patch build.
For instance, pmap_invalidate_page() calls smp_masked_invlpg(), which is provided by amd64/mp_machdep.c. And mp_machdep.c is 'optional smp' in files.amd64.

Oops, yes. This was from a larger patch series which entirely removes the SMP option for amd64. With just this patch, a nooptions SMP kernel cannot build.

It is enough to #ifdef SMP the calls to smp_masked_*() in each of the SMP invalidation functions.

What do you think of this goal overall? My belief is that !SMP amd64 kernels are not used very much and are not worth the maintenance overhead. With the complete patch series, all of sys/amd64 simply ignores the SMP option. If you think it's the right direction, I'll post the remaining patches.

I stated publicly more than once that I would like to remove UP at all, and not only for x86. But the discussion never ignited. In other words, I fully support removing as much #ifdef SMPs as we can.

Rather than fix this commit, I propose going further: https://reviews.freebsd.org/D51403

I brought up removal of !SMP support for amd64 last year at bsdcan and there was no objection. I am not sure about the importance of !SMP in general, e.g., maybe it is somewhat important to support it for embedded arm devices. But for amd64 I think there is no reasonable objection to its removal. GENERIC with nooptions SMP is broken in multiple ways and has been for a while.

I have two related observations:

Use ADDR_IS_KERNEL.

I also have an upcoming change in this area. AMD Ryzen processors have long supported a subset of the invpcid instruction’s functionality, even though they don’t support PCID. Specifically, they support the functionality to invalidate PG_G mappings, and not surprisingly this is supposed to be faster than toggling the PGE bit in CR4.

This can be abandoned.

In D51220#1170373, @markj wrote:

Do you have some local modifications to test the ADDR_IS_KERNEL(va) code path in pmap_enter_l2()?

See inline comment for an explanation.

Rename remove_pt to demote_kl2e to better reflect what it controls.

In D51180#1168328, @kib wrote:

Should the pmap_demote_pde() in pmap_unwire() get the same treatment? As I understand, wire should saved the pt page in radix.

In D51179#1168325, @alc wrote:

This lookup originated here:

commit 87b646630c4892e21446cd096bea6bcaecea33ac
Author: Mark Johnston <markj@FreeBSD.org>
Date:   Mon Nov 15 11:35:44 2021 -0500

    vm_page: Consolidate page busy sleep mechanisms
    
    - Modify vm_page_busy_sleep() and vm_page_busy_sleep_unlocked() to take
      a VM_ALLOC_* flag indicating whether to sleep on shared-busy, and fix
      up callers.
    - Modify vm_page_busy_sleep() to return a status indicating whether the
      object lock was dropped, and fix up callers.
    - Convert callers of vm_page_sleep_if_busy() to use vm_page_busy_sleep()
      instead.
    - Remove vm_page_sleep_if_(x)busy().
    
    No functional change intended.
    
    Obtained from:  jeff (object_concurrency patches)
    Reviewed by:    kib
    MFC after:      2 weeks
    Differential Revision:  https://reviews.freebsd.org/D32947

This lookup originated here:

commit 87b646630c4892e21446cd096bea6bcaecea33ac
Author: Mark Johnston <markj@FreeBSD.org>
Date:   Mon Nov 15 11:35:44 2021 -0500