Shouldn't there be pmap_delayed_invl_page() calls in pmap_remove_pages() where the PV entries are destroyed?

In D5747#131460, @alc wrote:

Shouldn't there be pmap_delayed_invl_page() calls in pmap_remove_pages() where the PV entries are destroyed?

Doesn't this mean that rw_rlock(&pvh_global_lock) in the pmap_remove_pages() is not enough, and we should use rw_wlock there ? And indeed, I do not see immediately why the situation explained in your citation in the revision description is impossible with rlock there.

In D5747#131487, @kib wrote:

In D5747#131460, @alc wrote:

Shouldn't there be pmap_delayed_invl_page() calls in pmap_remove_pages() where the PV entries are destroyed?

Doesn't this mean that rw_rlock(&pvh_global_lock) in the pmap_remove_pages() is not enough, and we should use rw_wlock there ? And indeed, I do not see immediately why the situation explained in your citation in the revision description is impossible with rlock there.

No, I don't think so, but I'll give it a second thought.

In D5747#131505, @alc wrote:

In D5747#131487, @kib wrote:

In D5747#131460, @alc wrote:

Shouldn't there be pmap_delayed_invl_page() calls in pmap_remove_pages() where the PV entries are destroyed?

Doesn't this mean that rw_rlock(&pvh_global_lock) in the pmap_remove_pages() is not enough, and we should use rw_wlock there ? And indeed, I do not see immediately why the situation explained in your citation in the revision description is impossible with rlock there.

No, I don't think so, but I'll give it a second thought.

It is impossible because the current pmap is single-threaded. Then, why would we need pmap_delayed_invl_page() there as well ? There cannot be a third thread still modifying the page with empty pv list but not yet invalidated by us. If e.g. page daemon starts reusing a page for which we cleared pv list, nothing wrong could happen to that page - curthread never returns to usermode with that pmap active.

In D5747#131539, @kib wrote:

In D5747#131505, @alc wrote:

In D5747#131487, @kib wrote:

In D5747#131460, @alc wrote:

Shouldn't there be pmap_delayed_invl_page() calls in pmap_remove_pages() where the PV entries are destroyed?

Doesn't this mean that rw_rlock(&pvh_global_lock) in the pmap_remove_pages() is not enough, and we should use rw_wlock there ? And indeed, I do not see immediately why the situation explained in your citation in the revision description is impossible with rlock there.

No, I don't think so, but I'll give it a second thought.

It is impossible because the current pmap is single-threaded. Then, why would we need pmap_delayed_invl_page() there as well ? There cannot be a third thread still modifying the page with empty pv list but not yet invalidated by us. If e.g. page daemon starts reusing a page for which we cleared pv list, nothing wrong could happen to that page - curthread never returns to usermode with that pmap active.

Yes, I agree. That the current pmap is single-threaded is also the reason why we don't use atomic load-and-clear to destroy page table entries in pmap_remove_pages().

Taken to the limit your argument would also say that the pmap_invalidate_all() at the end of pmap_remove_pages() is unnecessary, but don't we actually return to user-mode with the same pmap after an execve? So, pmap_invalidate_all() is required.

It appears to me that many of the use cases for pmap_delayed_invl_{started,finished}() are to account for the possibility that a failed demotion destroys a managed superpage mapping. However, I claim that in some of these cases, like pmap_unwire(), the calls to pmap_delayed_invl_{started,finished}() are unnecessary because the PV list lock is held until after the TLB invalidate is performed.

In D5747#132129, @alc wrote:

It appears to me that many of the use cases for pmap_delayed_invl_{started,finished}() are to account for the possibility that a failed demotion destroys a managed superpage mapping. However, I claim that in some of these cases, like pmap_unwire(), the calls to pmap_delayed_invl_{started,finished}() are unnecessary because the PV list lock is held until after the TLB invalidate is performed.

Well, the pmap_unwire() does not change the mapping at all, and the herald comment explains it, noting that no TLB invalidation is needed. So it does not really matter that some invalidation might be done under the PV lock (it does not, in fact).

If not braced with pmap_delayed_invl_started(), pmap_delayed_invl_page() becomes nop. It was very long time ago when I thought about this decision, but I have some memory that I did put assert initially there, but then changed it to just do nothing.

I reverted the addition of braces to pmap_remove_pages() and removed them from pmap_unwire() as well, with a comment.

In D5747#132130, @kib wrote:

In D5747#132129, @alc wrote:

It appears to me that many of the use cases for pmap_delayed_invl_{started,finished}() are to account for the possibility that a failed demotion destroys a managed superpage mapping. However, I claim that in some of these cases, like pmap_unwire(), the calls to pmap_delayed_invl_{started,finished}() are unnecessary because the PV list lock is held until after the TLB invalidate is performed.

Well, the pmap_unwire() does not change the mapping at all, and the herald comment explains it, noting that no TLB invalidation is needed. So it does not really matter that some invalidation might be done under the PV lock (it does not, in fact).

pmap_unwire() may not be the best example, but for a different reason than what you cite. A demotion can fail because it cannot find an existing, reserved page table page or allocate a new one. In such cases, it destroys the mapping, including a call to pmap_invalidate_page(). However, the reason that pmap_unwire() is not the best example is that demotion of wired mappings should never fail in this way.

If not braced with pmap_delayed_invl_started(), pmap_delayed_invl_page() becomes nop. It was very long time ago when I thought about this decision, but I have some memory that I did put assert initially there, but then changed it to just do nothing.

I reverted the addition of braces to pmap_remove_pages() and removed them from pmap_unwire() as well, with a comment.

In D5747#132133, @alc wrote:

pmap_unwire() may not be the best example, but for a different reason than what you cite. A demotion can fail because it cannot find an existing, reserved page table page or allocate a new one. In such cases, it destroys the mapping, including a call to pmap_invalidate_page(). However, the reason that pmap_unwire() is not the best example is that demotion of wired mappings should never fail in this way.

I changed the comment to

*	Since pmap_demote_pde() for the wired entry must never fail,
*	pmap_delayed_invl_started()/finished() calls around the
*	function are not needed.

Take a look at pmap_protect(). I claim that it doesn't need the calls to pmap_delayed_invl_{started,finished}() for the aforementioned reason.

In D5747#132135, @alc wrote:

Take a look at pmap_protect(). I claim that it doesn't need the calls to pmap_delayed_invl_{started,finished}() for the aforementioned reason.

Yes, it does not need the braces, but IMO it is because invalidation is done under pmap lock. Would the lock dropped, we would have a similar three-way race with pmap_remove_write(), and BTW this is the reason for pmap_delayed_invl_wait() in pmap_remove_write().

I removed braces in my repo.

In D5747#132136, @kib wrote:

In D5747#132135, @alc wrote:

Take a look at pmap_protect(). I claim that it doesn't need the calls to pmap_delayed_invl_{started,finished}() for the aforementioned reason.

Yes, it does not need the braces, but IMO it is because invalidation is done under pmap lock. Would the lock dropped, we would have a similar three-way race with pmap_remove_write(), and BTW this is the reason for pmap_delayed_invl_wait() in pmap_remove_write().

Where managed mappings are involved, I would never even think of performing the TLB invalidation after releasing the pmap lock. It would make the already difficult reasoning about the pmap layer even harder. :-)

In regards to pmap_remove_write(), I agree with what you've done. Concurrent calls to pmap_remove() and pmap_remove_write() are potentially problematic without the call to pmap_delayed_invl_wait() in pmap_remove_write().

In D5747#133072, @kib wrote:

Right patch.

That was fast!

My hypothesis is that the use cases for this new delayed invalidation synchronization primitive are now infrequent enough that spurious waits are rare.

Can we get pho@ to test this hypothesis?

In D5747#133075, @alc wrote:

My hypothesis is that the use cases for this new delayed invalidation synchronization primitive are now infrequent enough that spurious waits are rare.

Can we get pho@ to test this hypothesis?

How could this be arranged ? Restore md_page.invl_gen in addition to the pv_invl_gen[], and increment a counter on pv_invl_gen[i] being greater than md_page.invl_gen, in invl_wait() ?

In D5747#133076, @kib wrote:

In D5747#133075, @alc wrote:

My hypothesis is that the use cases for this new delayed invalidation synchronization primitive are now infrequent enough that spurious waits are rare.

Can we get pho@ to test this hypothesis?

How could this be arranged ? Restore md_page.invl_gen in addition to the pv_invl_gen[], and increment a counter on pv_invl_gen[i] being greater than md_page.invl_gen, in invl_wait() ?

With the following patch applied

diff --git a/sys/amd64/amd64/pmap.c b/sys/amd64/amd64/pmap.c
index d25c6c3..69d5ee5 100644
--- a/sys/amd64/amd64/pmap.c
+++ b/sys/amd64/amd64/pmap.c
@@ -484,6 +484,12 @@ pmap_delayed_invl_finished(void)
 static long invl_wait;
 SYSCTL_LONG(_vm_pmap, OID_AUTO, invl_wait, CTLFLAG_RD, &invl_wait, 0,
     "");
+static long invl_wait_coll1;
+SYSCTL_LONG(_vm_pmap, OID_AUTO, invl_wait_coll1, CTLFLAG_RD, &invl_wait_coll1, 0,
+    "");
+static long invl_wait_coll2;
+SYSCTL_LONG(_vm_pmap, OID_AUTO, invl_wait_coll2, CTLFLAG_RD, &invl_wait_coll2, 0,
+    "");
 #endif
 
 static u_long *
@@ -522,6 +528,9 @@ pmap_delayed_invl_wait(vm_page_t m)
 			accounted = TRUE;
 		}
 #endif
+		if (m->md.invl_gen <= pmap_invl_gen)
+			atomic_add_long(&invl_wait_coll1, 1);
+		atomic_add_long(&invl_wait_coll2, 1);
 		kern_yield(PRI_USER); /* XXX */
 	}
 }
@@ -548,6 +557,8 @@ pmap_delayed_invl_page(vm_page_t m)
 	m_gen = pmap_delayed_invl_genp(m);
 	if (*m_gen < gen)
 		atomic_store_rel_long(m_gen, gen);
+	if (m->md.invl_gen < gen)
+		m->md.invl_gen = gen;
 }
 
 /*
diff --git a/sys/amd64/include/pmap.h b/sys/amd64/include/pmap.h
index 47d81ca..e96c635 100644
--- a/sys/amd64/include/pmap.h
+++ b/sys/amd64/include/pmap.h
@@ -292,6 +292,7 @@ struct md_page {
  	TAILQ_HEAD(, pv_entry)	pv_list;  /* (p) */
 	int			pv_gen;   /* (p) */
 	int			pat_mode;
+	u_long			invl_gen; /* (p) long to avoid wraparounds. */
 };
 
 enum pmap_type {

Peter' testing shows

>> vm.pmap.invl_wait_coll2: 5228265
>> vm.pmap.invl_wait_coll1: 5156965
>> vm.pmap.invl_wait: 705

which is convincing enough that per-PV list DI generation counter gives similar wait times as the per-page DI generation.

I'm done with all of the comments.

The turnstile KPI use looks ok to me. My only request as I noted to Konstantin on IRC is having an overview comment explaining why this is done.

Peter did extensive testing of the latest version of the patch, including specialized checks to see if there is a blocked thread left after the DI situation due to lost wakeups. No issues were seen.

WRT John request to explain the issue which is solved by DI. First, the introductory comment before pmap_delayed_invl_started() explains what is it. Second, I will cite the letter from Alan, which was used as the introduction for the revision, in the commit log. Do you consider any more details are needed ?

Overall, this looks like a concluding stage.

Yes, I think Alan's original mail is the thing I'm after. Mainly I want to document why this change is being made (and why to use a more complex construct in place of a rwlock).