Permit vm_page_wire() for pages not belonging to an object.

[markj]

Summary

For such pages ref_count is effectively a consumer-managed field, but
there is no harm in calling vm_page_wire() on them.
vm_page_unwire_noq() handles them as well. Relax the vm_page_wire()
assertions to permit this case.

bz and cem both reported this since the assertion breaks some
out-of-tree code.

[dougm]

sys/vm/vm_page.c

3858 ↗

(On Diff #76170)

If there may be no containing object, this comment could be misleading.

[markj]

Clarify the vm_page_wire() header comment.

[kib]

I looked at the disassembly of the GENERIC-NODEBUG vm_page_wire() with your patch applied, and I think that the whole if()/VM_OBJECT_ASSERT_LOCKED statement should be put under #ifdef INVARIANTS:

0xffffffff80ed4c60 <+0>:     push   %rbp
0xffffffff80ed4c61 <+1>:     mov    %rsp,%rbp
0xffffffff80ed4c64 <+4>:     mov    0x20(%rdi),%rax
0xffffffff80ed4c68 <+8>:     test   %rax,%rax
0xffffffff80ed4c6b <+11>:    je     0xffffffff80ed4c7b <vm_page_wire+27>
0xffffffff80ed4c6d <+13>:    mov    0x54(%rdi),%ecx
0xffffffff80ed4c70 <+16>:    cmp    $0x1,%ecx
0xffffffff80ed4c73 <+19>:    jne    0xffffffff80ed4c7b <vm_page_wire+27>
0xffffffff80ed4c75 <+21>:    mov    0x94(%rax),%eax
0xffffffff80ed4c7b <+27>:    mov    $0x1,%eax
0xffffffff80ed4c80 <+32>:    lock xadd %eax,0x50(%rdi)

Unpatched code demostrates similar silliness:

0xffffffff80ed4c60 <+0>:     push   %rbp
0xffffffff80ed4c61 <+1>:     mov    %rsp,%rbp
0xffffffff80ed4c64 <+4>:     mov    0x54(%rdi),%eax
0xffffffff80ed4c67 <+7>:     cmp    $0x1,%eax
0xffffffff80ed4c6a <+10>:    jne    0xffffffff80ed4c76 <vm_page_wire+22>
0xffffffff80ed4c6c <+12>:    mov    0x20(%rdi),%rax
0xffffffff80ed4c70 <+16>:    mov    0x94(%rax),%eax
0xffffffff80ed4c76 <+22>:    mov    $0x1,%eax
0xffffffff80ed4c7b <+27>:    lock xadd %eax,0x50(%rdi)

lock xadd is the atomic_fetchadd_int().

With #ifdef INVARIANTS we get a reasonable code:

0xffffffff80ed4c40 <+0>:     push   %rbp
0xffffffff80ed4c41 <+1>:     mov    %rsp,%rbp
0xffffffff80ed4c44 <+4>:     mov    $0x1,%eax
0xffffffff80ed4c49 <+9>:     lock xadd %eax,0x50(%rdi)

[markj]

Wrap check with an INVARIANTS guard.

[cem]

The only thing missing from https://reviews.freebsd.org/P394 is KASSERT(vm_page_wired(m)) in the m->object == NULL case (maybe better spelled as VPRC_WIRE_COUNT(m->ref_count) >= 1?). Not just for fictitious pages. But I don't know if that matches bz@'s usecase or just ours.

[markj]

In D26173#581443, @kib wrote:

I looked at the disassembly of the GENERIC-NODEBUG vm_page_wire() with your patch applied, and I think that the whole if()/VM_OBJECT_ASSERT_LOCKED statement should be put under #ifdef INVARIANTS:

I guess because the condition uses atomic_load and thus can't be elided?

In D26173#581453, @cem wrote:

The only thing missing from https://reviews.freebsd.org/P394 is KASSERT(vm_page_wired(m)) in the m->object == NULL case (maybe better spelled as VPRC_WIRE_COUNT(m->ref_count) >= 1?). Not just for fictitious pages. But I don't know if that matches bz@'s usecase or just ours.

I thought about asserting that but didn't want to make further assumptions about unmanaged consumers. I believe it would be fine for Bjoern's case since the LinuxKPI's alloc_pages() always returns wired pages (since r348743).

[kib]

In D26173#581473, @markj wrote:

In D26173#581443, @kib wrote:

I looked at the disassembly of the GENERIC-NODEBUG vm_page_wire() with your patch applied, and I think that the whole if()/VM_OBJECT_ASSERT_LOCKED statement should be put under #ifdef INVARIANTS:

I guess because the condition uses atomic_load and thus can't be elided?

It is a consequence of the way clang interprets volatile *, I think.