Page MenuHomeFreeBSD
Differential D31985

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

Authored by markj on Sep 16 2021, 4:56 PM.
Tags
None
Referenced Files
F81574868: D31985.id95288.diff
Thu, Apr 18, 8:28 AM
Unknown Object (File)
Thu, Mar 28, 9:03 PM
Unknown Object (File)
Mar 13 2024, 2:46 PM
Unknown Object (File)
Mar 4 2024, 4:12 AM
Unknown Object (File)
Mar 4 2024, 4:09 AM
Unknown Object (File)
Mar 4 2024, 4:09 AM
Unknown Object (File)
Mar 4 2024, 4:09 AM
Unknown Object (File)
Mar 4 2024, 4:09 AM
View All 55 Files
Subscribers
imp

Details

Reviewers
alc
kib
Commits
rGb498f71bc56a: vm_page: Add a new page allocator interface for unnamed pages
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
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj created this revision.Sep 16 2021, 4:56 PM
Herald added a subscriber: imp. · View Herald TranscriptSep 16 2021, 4:56 PM
markj requested review of this revision.Sep 16 2021, 4:56 PM
Harbormaster completed remote builds in B41544: Diff 95253.Sep 16 2021, 4:56 PM
markj added a child revision: D31986: Convert vm_page_alloc() callers to use vm_page_alloc_noobj()..Sep 16 2021, 4:56 PM
kib added inline comments.Sep 16 2021, 7:58 PM
sys/vm/vm_page.c
2398

I would allow VM_ALLOC_NOBUSY

2401

!= 0

2439

!= 0

2545

Why moving this function around?

kib added inline comments.Sep 16 2021, 8:02 PM
sys/vm/vm_page.c
2442

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.

kib added inline comments.Sep 16 2021, 8:08 PM
sys/vm/vm_page.h
545

Do you plan to remove the flag?

markj marked 5 inline comments as done.Sep 17 2021, 1:43 PM
markj added inline comments.
sys/vm/vm_page.c
2442

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

2545

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 updated this revision to Diff 95288.Sep 17 2021, 1:44 PM
markj marked 2 inline comments as done.

Style, initialize pindex, permit _NOBUSY.

Harbormaster completed remote builds in B41566: Diff 95288.Sep 17 2021, 1:44 PM
kib accepted this revision.Sep 17 2021, 5:55 PM
kib added inline comments.
sys/vm/vm_page.c
2545

Perhaps commit the move separately?

This revision is now accepted and ready to land.Sep 17 2021, 5:55 PM
alc accepted this revision.Sep 22 2021, 5:54 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().

2404

See my below comment about the _freelist functions.

2547

You can drop the blank line here.

2548–2560

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.

imp added inline comments.Sep 22 2021, 6:02 PM
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.

alc added inline comments.Sep 23 2021, 5:33 PM
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 marked 5 inline comments as done.Sep 24 2021, 3:55 PM
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.

2548–2560

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 updated this revision to Diff 95635.Sep 24 2021, 4:06 PM
markj marked an inline comment as done.

Tidy up vm_page_alloc_check().

This revision now requires review to proceed.Sep 24 2021, 4:06 PM
Harbormaster completed remote builds in B41703: Diff 95635.Sep 24 2021, 4:06 PM
imp added a comment.Sep 25 2021, 2:35 AM

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.

markj added a comment.Oct 16 2021, 2:00 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.

kib accepted this revision.Oct 16 2021, 2:13 PM
In D31985#733674, @markj wrote:

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.Oct 16 2021, 2:13 PM
alc accepted this revision.Oct 16 2021, 6:47 PM
In D31985#733674, @markj wrote:

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.

alc added inline comments.Oct 18 2021, 12:39 AM
sys/vm/vm_page.h
543
markj updated this revision to Diff 97028.Oct 18 2021, 1:14 AM
markj marked an inline comment as done.

Update the VM_ALLOC_ZERO comment.

This revision now requires review to proceed.Oct 18 2021, 1:14 AM
Harbormaster completed remote builds in B42212: Diff 97028.Oct 18 2021, 1:14 AM
alc accepted this revision.Oct 19 2021, 5:23 PM

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.Oct 19 2021, 5:23 PM
markj added a comment.Oct 19 2021, 6:39 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!

Closed by commit rGb498f71bc56a: vm_page: Add a new page allocator interface for unnamed pages (authored by markj). · Explain WhyOct 20 2021, 1:23 AM
This revision was automatically updated to reflect the committed changes.
markj added a commit: rGb498f71bc56a: vm_page: Add a new page allocator interface for unnamed pages.

Revision Contents
Changeset List

PathSize
sys/
vm/
14 lines
141 lines

Diff 97139

View Options

sys/vm/vm_page.h

Loading...
View Options

sys/vm/vm_page.c

Loading...