Page MenuHomeFreeBSD

Remove some assertions related to queue state and wiring.
AbandonedPublic

Authored by markj on Aug 26 2019, 11:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 23 2023, 4:39 AM
Unknown Object (File)
Jul 15 2023, 10:16 PM
Unknown Object (File)
Jun 26 2023, 11:14 PM
Subscribers
None

Details

Reviewers
alc
kib
dougm
Summary
  • The virtio balloon driver asserts that a newly allocated page does not belong to a page queue. The page is allocated with _NOOBJ, and the page allocator already ensures that all queue state is cleared before returning a page.
  • When forcing swapins by disabling a swap device, we assert that any resident pages backed by the device are either wired or enqueued.

When the wire count and queue state are updated using atomics, these
invariants are harder to assert. Since they seem somewhat arbitrary,
remove them. The pmap layer does not, for example, assert that newly
allocated PTPs are not enqueued.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 26118
Build 24638: arc lint + arc unit

Event Timeline

I do not object against the removals. But please note that we had systematic bugs where pages were 'forgotten' to be added to a queue after allocation or some uses, essentially making them unswappable. So some way to assert that the page is visible to the pagedaemon is needed.

This revision is now accepted and ready to land.Aug 27 2019, 2:12 PM
In D21424#466489, @kib wrote:

I do not object against the removals. But please note that we had systematic bugs where pages were 'forgotten' to be added to a queue after allocation or some uses, essentially making them unswappable. So some way to assert that the page is visible to the pagedaemon is needed.

I think our KPIs have changed such that it is harder to make such a mistake. For instance, vm_page_unwire() now takes a queue parameter, so it simultaneously releasing a wiring and enqueues the page. With my change for atomic wire_count manipulation, vm_page_unwire(m, PQ_NONE) is not permitted; one must use vm_page_unwire_noq() instead, and there are relatively few callers of this function outside of the pmap code.

I do agree that some replacement assertion for swp_pager_force_dirty() would be useful, but I also do not want any details of the internals of wire_count and queue state handling to leak into unrelated code.

In D21424#466489, @kib wrote:

I do not object against the removals. But please note that we had systematic bugs where pages were 'forgotten' to be added to a queue after allocation or some uses, essentially making them unswappable. So some way to assert that the page is visible to the pagedaemon is needed.

I think our KPIs have changed such that it is harder to make such a mistake. For instance, vm_page_unwire() now takes a queue parameter, so it simultaneously releasing a wiring and enqueues the page. With my change for atomic wire_count manipulation, vm_page_unwire(m, PQ_NONE) is not permitted; one must use vm_page_unwire_noq() instead, and there are relatively few callers of this function outside of the pmap code.

Typical case was alloc/hold, do something, then unhold, forgetting to activate. See r269907 for the prominent example.

In D21424#466489, @kib wrote:

I do not object against the removals. But please note that we had systematic bugs where pages were 'forgotten' to be added to a queue after allocation or some uses, essentially making them unswappable. So some way to assert that the page is visible to the pagedaemon is needed.

+1

In D21424#466539, @kib wrote:
In D21424#466489, @kib wrote:

I do not object against the removals. But please note that we had systematic bugs where pages were 'forgotten' to be added to a queue after allocation or some uses, essentially making them unswappable. So some way to assert that the page is visible to the pagedaemon is needed.

I think our KPIs have changed such that it is harder to make such a mistake. For instance, vm_page_unwire() now takes a queue parameter, so it simultaneously releasing a wiring and enqueues the page. With my change for atomic wire_count manipulation, vm_page_unwire(m, PQ_NONE) is not permitted; one must use vm_page_unwire_noq() instead, and there are relatively few callers of this function outside of the pmap code.

Typical case was alloc/hold, do something, then unhold, forgetting to activate. See r269907 for the prominent example.

So this example is covered by the hold/wire merge, since vm_page_unwire() handles both unwiring and activation.

I understand that in swp_pager_force_pagein() we want to assert that already-resident pages are somehow visible, it just seemed rather arbitrary to do this in one very specific and rarely executed piece of code. I suspect this kind of assertion really belongs in vm_page_lookup(). In attempting to avoid using the page lock to synchronize the wire count and page queue state, it became difficult to express this assertion correctly, and I wanted to remove it since it directly references a vm_page field. I will hold off until I have some code to add a more centralized assertion.