As was discussed somewhere else, this assert should stil check that the page is busied if PMAP_ENTER_NOSLEEP was specified. It may be just a replacement of VM_OBJECT_ASSERT_LOCKED() by (PMAP_ENTER_NOSLEEP && VM_PAGE_OBJECT_BUSY_ASSERT).

We wrote a value to fs->first_object->busy above, and there we read m->busy. Other thread could write the lock cookie to m->busy, and then rechecks first_object->busy.

What prevents us from seeing non-busies m, simultaneously with other thread seeing unbusy first_object ? In other words, it is typical (x=1; read y;) vs. (y = 1; read x;) race.

I'm not sure I fully follow. We're already asserting that the page is busy in all cases. What is the additional assert?

The important thing is that when the object is busy you can transiently have a busy page but you can not have any modifications permitted by busying a page. So you are left with a harmless race and you skip superpage insertion.

So in your example the first thread may read the page as busy and operate as-if the page had been busy before the call to vm_object_busy(). The second thread will observe the object busy count in tryxbusy and unbusy the page before returning failure to the caller. So you do have this transient condition. I believe it is harmless and according to tests I have done it is quite rare.

The other obvious solution is to abandon this whole approach and simply busy/unbusy 512 pages.

Since the goal of these patches is to permit busying a page without the object lock held, how is the check for the busy state of m below going to work? I presume that at some point we must change this code to xbusy m, but that won't succeed if the object is busy.

Currently vm_page_xbusy() and vm_page_sbusy() panic if their attempts to update the page state fail. Right now we are protected from that by the object lock, but how would this work in the longer term?

I think this deserves a comment. What exactly does the wiring protect us from here?

m doesn't need to be parenthesized here.

I feel that my comments must be failing to describe something fundamental. This is addressed in the patch description.

If the page was busied by another thread before the call to vm_object_busy then busied returns true and we fall back to the fully synchronized version of vm_fault. All of the code in soft_fault_fast is an optional fast path.

We do not busy the first page or any other page in the superpage set. We use the object busy to guarantee that the state remains stable across the operation by denying other threads from acquiring busy.

Those functions are to be eliminated at the end of the patchset. Nothing in the kernel calls them after #2. Only trybusy and busy_acquire() are used.

Yes the notion is if you don't have the object locked you must be holding a reference count on the page otherwise it can disappear or change identity while you sleep and loop to acquire busy again.

Isn't obj != NULL guaranteed by the fact that we hold the object lock?

Same here.

I just confused myself while reading code.

I see, I forgot about the fact that vm_page_object_busy_assert() does check for busy (but not for xbusy, it seems).

But the impossible reads are relevant for the non-superpage case as well. We might map a busy page (4k), and other thread might mark and assume the page busy without noticing object->busy.

I don't understand this example. How will another thread assume page is busy without noticing object busy?

I now understand this to mean I likely need stronger serialization in vm_object_busy().

I'm also considering renaming this to vm_object_barrier() or something of the sort so the exact mechanism and guarantees are not conflated with page busy.

I believe you need the _acq_rel() fence there, because _acq is basically r|r,w barrier. I.e. the write of the new busy counter is allowed to 'move' after the _acq(). _rel is r,w|w.

You may want to note in comment that the _rel fence would sync/with _acq of the page trybusy operation.

On x86 this would still produce only compiler memory barrier, so there should be no loss.

And there, I believe, you need a _rel fence to sync/w the acq part of the fence in vm_object_busy().

We want to stop a load of the page busy field from being re-ordered before the refcount is acquired. _rel would allow the load to precede it.

This acq barrier should mean that we did not speculatively load the object busy until after the atomic completed.

Right, _rel is not enough, but _acq is not enough as well, see above. This is why I said that
_acq_rel is needed (refcount acquire is both load and store in atomic, so it should be not possible to pass _acq_rel fence).

acq only prevents the operations after the fcmpset to appear before it. It does not prevents load before to execute after fcmpset.

I see yes, I agree.

" When an atomic operation has acquire semantics, the operation must have

completed before any subsequent load or store (by program order) is
performed.  Conversely, acquire semantics do not require that prior loads
or stores have completed before the atomic operation is performed. "

This is all that is necessary. The import object busy check is after the atomic. The one above is racy and speculative.

You are right, sorry.

I think we are in agreement now? Ok to commit with the fence above fixed? Thank you for your attention to detail.

Yes, sure.