Would the results of the atomic swap allow us to avoid vm_page_lock here?

I don't understand the case where we're not dirtying the page but we do need to set PGA_NOSYNC. Do we need to do this if !need_dirty? Can't we wait for the first dirtying fault?

I don't really like the duplication of the atomic macros now that we have set, clear, and swap. I didn't see a clean way to consolidate them.

Could trylock here as well.

Don't we need to recheck PGA_UNSWAPPED after locking the object ? It might be coincidental, but clearing happens only under the object lock, and this prevents unlikely race.

Most of the time, there won't be any allocated swap space for the page. So, I would suggest that you unconditionally call vm_pager_page_unswapped() and let it conditionally set the flag. Essentially, I'm suggesting that we combine D22324 with this change. In D22324, @dougm tries to upgrade the object lock so as to be able to free the swap space. Instead, of attempting to upgrade the lock, we set the flag.

I should have asked: Are you intending to call this function without any lock on the object? vm_pager_page_unswapped() needs a read lock, unless the radix tree used to manage an object's swap space is also made to support lock-free lookups.

I would prefer not to go down the rabbit hole of also making swap radix lockless for now. Although in principal it is very simple. I have a substantial patch queue to work down.

We could just use another bit to indicate that there is allocated swap space. We are not so pressed for page flag space.

How do you feel about a separate paging queue that is drained quickly for pages with both of these bits set? Kib & Mark feel this may create out of swap conditions because the backpressure won't work quickly enough.

Was this not leaking swap space in the !excl && need_dirty case?

Did we not encounter this due to the conditions under which we call vm_fault_soft_fast?

It's not precisely a leak, because the swap space can eventually be reclaimed, e.g., when the vm object is destroyed. However, I do agree that the current behavior is less than ideal, and it probably occurs more often than we would like. In such cases, your patch is already an improvement. Addressing this problem was also the objective of D22324.

If you are comfortable with the 'two bit' solution (HASSWAP & UNSWAPPED) I can code that and push the locking/bit set/bit clear into swap_pager_unswapped().

Given that we were that imprecise about clearing it before do you think we need an additional page queue to lower the latency or should existing paging systems be adequate with the delayed bit detection?

My claim was that there is no backpressure, because we only scan the active queue aggressively when there is a shortage of pages in the inactive and laundry queues. After revisiting the code, I'm not sure that this will be a major problem since the laundry thread moves pages out of PQ_LAUNDRY and into PQ_UNSWAPPABLE when a swap pageout fails because we couldn't allocate blocks. In other words, if we run out of swap space and are under memory pressure we should rapidly run down the laundry queue and trigger an all-out scan of the active queue, which will hopefully reclaim enough swap space if any is available.

Can you describe what synchronization is needed to set and clear this flag?

Don't we need to do this while laundering as well? Suppose a page in the laundry queue is dirty and has UNSWAPPED set. We may page out its contents and mark it clean, but I don't see how PGA_UNSWAPPED gets cleared at that point.

accidental whitespace. I will remove before commit.

I prefer not to trylock here. In my experience if you trylock from a great number of threads you can end up with degenerate behavior. By pushing it into the page daemon we have only a couple of threads that are potentially adding contention to the object lock.

This is a bug. I am running stress2 now.

The page isn't exactly unswappable but the block is dead. On read both are. The comment above "If an error occurs I'd love to..." may be possible now.

Presumably these should not be the same value.

I think you are right. There is also no guarantee that the object pointer is not stale, so that must be rechecked as well.

I see I will fix this. Incidentally with recent changes it would be rendered harmless. We would see that object is not locked and just make sure the bits were right.

Could we put these under vm.stats.swap instead, so there is a little more harmony with vm.stats.page and vm.stats.object?

Assert that either the object is wlocked or the page is xbusied here? I believe that is true among existing callers, or else it is not safe to dereference obj.

This comment isn't true anymore.

Yes that is true, I can add this assert.

Now that I commit this I wonder if this should just be PGA_SWAP_SPACE not SWAP_FREE. It would catch potential leaks before the object was destroyed. replace/rename would remove valid swap but this should only happen with a valid page.

Any comments?