Page MenuHomeFreeBSD

vm_page: Add a new page allocator interface for anonymous pages
ClosedPublic

Authored by markj on Sep 16 2021, 4:56 PM.

Details

Summary

The diff adds vm_page_alloc_noobj() and vm_page_alloc_noobj_domain().
These mostly correspond to vm_page_alloc() and vm_page_alloc_domain()
when no VM object is specified, with the exception that they handle
VM_ALLOC_ZERO by zeroing the page, rather than by preserving PG_ZERO.

This simplifies callers and makes it harder to accidentally omit a
pmap_zero_page() call. It also permits simplification of the
vm_page_alloc_domain() implementation.

Since the new interface is similar to vm_page_alloc_freelist(),
implement both of them using a common backend allocator function.
No functional change intended.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Sep 16 2021, 4:56 PM
sys/vm/vm_page.c
2397

I would allow VM_ALLOC_NOBUSY

2400

!= 0

2438

!= 0

2544

Why moving this function around?

sys/vm/vm_page.c
2441

I wonder if it would be useful to assign pindex some value? E.g. 0 or -1, or even some pattern like deadc0de, so that consumers that need pindex would note it.

sys/vm/vm_page.h
545

Do you plan to remove the flag?

markj added inline comments.
sys/vm/vm_page.c
2441

I like that idea. Maybe it should be INVARIANTS-only but I don't think there is any harm in having it be unconditional.

2544

I just wanted to group all of the allocator entry points together.

sys/vm/vm_page.h
545

Eventually, yes. For now it is kept since vm_page_alloc(NULL, 0, VM_ALLOC_NOOBJ) is still permitted. I would like to MFC as much as possible to stable/13.

markj marked 2 inline comments as done.

Style, initialize pindex, permit _NOBUSY.

kib added inline comments.
sys/vm/vm_page.c
2544

Perhaps commit the move separately?

This revision is now accepted and ready to land.Sep 17 2021, 5:55 PM
alc added inline comments.
sys/vm/vm_page.c
2401–2406

As an aside, I am going to argue that these functions should be jettisoned. They are only used by MIPS, and those uses would be handled efficiently, i.e., without excessive iteration, by vm_page_alloc_noobj_contig().

2403

See my below comment about the _freelist functions.

2546

You can drop the blank line here.

2547–2559

If I am allowed to be truly pedantic for a moment, I would suggest reordering these asserts to match the order of the fields in the struct. Then, a reader can more easily figure out which fields are being checked and which are not.

sys/vm/vm_page.c
2401–2406

One could just mark them #ifdef mips. then when the coming mips retirement happens, they will be removed.

sys/vm/vm_page.c
2401–2406

How soon will that be? I brought up the elimination of the _freelist functions here because eliminating them would simplify the implementation of the helper function, _vm_page_alloc_noobj_domain(), that is introduced in this change.

markj added inline comments.
sys/vm/vm_page.c
2401–2406

It looks like Warner started the discussion, so I guess it would happen sometime before 14.x. We could in the meantime implement a MIPS-private vm_page_alloc_freelist() like this (together with a per-domain variant)?

vm_page_t
pmap_alloc_direct_page(vm_pindex_t index, int req)
{
    vm_page_t m;

    req |= VM_ALLOC_WIRE | VM_ALLOC_ZERO;
#if VM_FREELIST_DIRECT == VM_FREELIST_DEFAULT
    m = vm_page_alloc_noobj(req);
#elif VM_FREELIST_DIRECT == VM_FREELIST_LOWMEM
    m = vm_page_alloc_noobj_contig(req, 1, 0, VM_LOWMEM_BOUNDARY, PAGE_SIZE, 0, VM_MEMATTR_DEFAULT);
#else
#error unhandled
#endif
    m->pindex = index;
    return (m);
}

Alternately, just eliminate the _vm_page_alloc_noobj_domain() helper once MIPS is removed. Or, keep the vm_page_alloc_freelist() implementation separate from the vm_page_alloc_noobj() implementation so that the eventual removal of vm_page_alloc_freelist() needn't touch anything else.

2547–2559

I split the vm_page_alloc_check() move into a separate commit already. This diff shows the modifications you suggested, but I will merge them with the separate commit before pushing.

markj marked an inline comment as done.

Tidy up vm_page_alloc_check().

This revision now requires review to proceed.Fri, Sep 24, 4:06 PM

Mips will be in the tree through end of year. If all the stars align, maybe it will be gone by US thanksgiving.
There's no push back so far in the discussions.

Is there any objection to committing this and the rest of the series (the reviews listed under the "stack" tab)? Especially with regards to naming since many consumers are being updated.

Is there any objection to committing this and the rest of the series (the reviews listed under the "stack" tab)? Especially with regards to naming since many consumers are being updated.

Not from me; I believe I clicked "accept" at least once on each review.

This revision is now accepted and ready to land.Sat, Oct 16, 2:13 PM

Is there any objection to committing this and the rest of the series (the reviews listed under the "stack" tab)? Especially with regards to naming since many consumers are being updated.

I don't think so. I have some free time on my hands today, so I will take one last look at the series.

sys/vm/vm_page.h
542
markj marked an inline comment as done.

Update the VM_ALLOC_ZERO comment.

This revision now requires review to proceed.Mon, Oct 18, 1:14 AM

Just to be clear, I believe that I've looked at all of the patches, and unless there is a recent change that you want me to look at, I'm done.

This revision is now accepted and ready to land.Tue, Oct 19, 5:23 PM
In D31985#734942, @alc wrote:

Just to be clear, I believe that I've looked at all of the patches, and unless there is a recent change that you want me to look at, I'm done.

Thank you very much for the reviews!