Page MenuHomeFreeBSD
Differential D49391

vm_page: update legend documenting allocation
ClosedPublic
Actions

Authored by alc on Mar 16 2025, 6:15 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 20, 2:35 PM
Unknown Object (File)
Fri, Dec 12, 8:06 PM
Unknown Object (File)
Fri, Dec 12, 8:06 PM
Unknown Object (File)
Fri, Nov 28, 6:10 PM
Unknown Object (File)
Fri, Nov 28, 6:08 PM
Unknown Object (File)
Thu, Nov 27, 8:59 PM
Unknown Object (File)
Wed, Nov 26, 4:45 PM
Unknown Object (File)
Sun, Nov 23, 11:14 PM
View All Files
Subscribers
imp

Details

Reviewers
dougm
kib
markj
Commits
rG07297aee35f2: vm_page: update comments and KASSERT()s concerning page allocation
Summary

Update the legend describing the arguments to the most commonly used page allocation functions. Notably, eliminate a reference to a function that no longer exists; and update to reflect the elimination of VM_ALLOC_NOOBJ, specifically, VM_ALLOC_WAITOK is no longer a legal option to vm_page_alloc{,_contig}().

Eliminate a nonsensical KASSERT(). VM_ALLOC_SBUSY is forbidden as an argument to vm_page_alloc_noobj{,_contig,}_domain() by a KASSERT(), so having a different KASSERT() that tests for it is nonsensical.

Strengthen other KASSERT()s involving VM_ALLOC_NOBUSY and VM_ALLOC_NOFREE.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

dougm requested review of this revision.Mar 16 2025, 6:15 AM
dougm created this revision.
kib added inline comments.Mar 16 2025, 7:11 AM
sys/vm/vm_page.c
4764

I think that noting that the object is locked on entry and exit, with possible drop in between, is at least in line with other functions descriptions.

4800

Or you could write MPASS((allocflags & VM_ALLOC_WAITFAIL) == 0);

dougm updated this revision to Diff 152332.Mar 16 2025, 8:05 PM
dougm marked 2 inline comments as done.

Address @kib comments.

kib accepted this revision.Mar 17 2025, 12:52 AM
This revision is now accepted and ready to land.Mar 17 2025, 12:52 AM
alc added inline comments.Mar 17 2025, 6:12 AM
sys/vm/vm_page.c
4800

You could pass __func__ to this function and use %s here. Constant propagation with inlining will make it the generated code equivalent to the callers performing the KASSERTs directly.

dougm updated this revision to Diff 152336.Mar 17 2025, 6:18 AM
dougm marked an inline comment as done.

Apply @alc suggestion for func in KASSERTS in vm_page_grab_check().

This revision now requires review to proceed.Mar 17 2025, 6:18 AM
alc added a comment.Mar 17 2025, 6:48 AM

The vm_page.h comment incorrectly says vm_page_alloc accepts VM_ALLOC_WAITOK.

alc added a comment.Mar 17 2025, 7:05 AM
In D49391#1126158, @alc wrote:

The vm_page.h comment incorrectly says vm_page_alloc accepts VM_ALLOC_WAITOK.

VM_ALLOC_NODUMP is also incorrectly annotated.

dougm updated this revision to Diff 152337.Mar 17 2025, 7:19 AM

Correct a couple of vm allocflags annotations in readcli.h.

alc added inline comments.Mar 17 2025, 7:42 PM
sys/vm/vm_page.h
537
alc added inline comments.Mar 17 2025, 7:45 PM
sys/vm/vm_page.h
515–516

vm_page_alloc_freelist() no longer exists.

528
dougm updated this revision to Diff 152359.Mar 17 2025, 7:58 PM
dougm marked 3 inline comments as done.

Further correct the page allocation parameter comments of vm_page.h, guided by @alc. Add a func parameter to vm_page_grab_check.

dougm mentioned this in D49371: vm_grab: use iterator for grab lookup.Mar 17 2025, 11:20 PM
kib accepted this revision.Mar 18 2025, 1:42 AM
This revision is now accepted and ready to land.Mar 18 2025, 1:42 AM
markj added a comment.Mar 23 2025, 10:16 AM

The functional portion of the change seems fine, but I'm not sure why it's a problem for vm_page_grab* to support VM_ALLOC_WAITFAIL?

sys/vm/vm_page.c
4762
4791

You might need to annotate func with __diagused to dampen build warnings when KASSERT() is compiled out, i.e., the GENERIC-NODEBUG kernel config.

Rather than forcing callers to explicitly pass __func__, it'd be a bit nicer to rename this function to _vm_page_grab_check() and have #define vm_page_grab_check(...) _vm_page_grab_check(__VA_ARGS__, __func__).

dougm marked 2 inline comments as done.Mar 23 2025, 5:14 PM
In D49391#1127890, @markj wrote:

The functional portion of the change seems fine, but I'm not sure why it's a problem for vm_page_grab* to support VM_ALLOC_WAITFAIL?

Because in comments on D49371, @kib suggested that VM_ALLOC_WAITFAIL was invalid here. So, to remove that obstacle to getting D49371 approved, I investigated and found that he might be right. Now, @alc has verbally placed a hold on this change because, when he is freed from another task, he plans to look for other illegal allocflag settings.

dougm updated this revision to Diff 152556.Mar 23 2025, 5:16 PM

Apply @markj comments.

This revision now requires review to proceed.Mar 23 2025, 5:16 PM
markj added a comment.Mar 23 2025, 10:32 PM
In D49391#1127942, @dougm wrote:
In D49391#1127890, @markj wrote:

The functional portion of the change seems fine, but I'm not sure why it's a problem for vm_page_grab* to support VM_ALLOC_WAITFAIL?

Because in comments on D49371, @kib suggested that VM_ALLOC_WAITFAIL was invalid here. So, to remove that obstacle to getting D49371 approved, I investigated and found that he might be right. Now, @alc has verbally placed a hold on this change because, when he is freed from another task, he plans to look for other illegal allocflag settings.

My reading of the comments is that kib is pointing out a bug in the new code.

dougm added a comment.Mar 23 2025, 11:36 PM
In D49391#1128121, @markj wrote:
In D49391#1127942, @dougm wrote:
In D49391#1127890, @markj wrote:

The functional portion of the change seems fine, but I'm not sure why it's a problem for vm_page_grab* to support VM_ALLOC_WAITFAIL?

Because in comments on D49371, @kib suggested that VM_ALLOC_WAITFAIL was invalid here. So, to remove that obstacle to getting D49371 approved, I investigated and found that he might be right. Now, @alc has verbally placed a hold on this change because, when he is freed from another task, he plans to look for other illegal allocflag settings.

My reading of the comments is that kib is pointing out a bug in the new code.

He wrote, at D49371:
I am not sure about the semantic of VM_ALLOC_WAITFAIL. If we sleep inside vm_page_grab_lookup(), should we also return NULL for VM_ALLOC_WAITFAIL? (This is the pre-existing bug, if indeed a bug).

So I disagree that he is calling out a bug in new code.

markj added a comment.EditedMar 23 2025, 11:42 PM
In D49391#1128151, @dougm wrote:
In D49391#1128121, @markj wrote:
In D49391#1127942, @dougm wrote:
In D49391#1127890, @markj wrote:

The functional portion of the change seems fine, but I'm not sure why it's a problem for vm_page_grab* to support VM_ALLOC_WAITFAIL?

Because in comments on D49371, @kib suggested that VM_ALLOC_WAITFAIL was invalid here. So, to remove that obstacle to getting D49371 approved, I investigated and found that he might be right. Now, @alc has verbally placed a hold on this change because, when he is freed from another task, he plans to look for other illegal allocflag settings.

My reading of the comments is that kib is pointing out a bug in the new code.

He wrote, at D49371:
I am not sure about the semantic of VM_ALLOC_WAITFAIL. If we sleep inside vm_page_grab_lookup(), should we also return NULL for VM_ALLOC_WAITFAIL? (This is the pre-existing bug, if indeed a bug).

So I disagree that he is calling out a bug in new code.

Then I don't follow: in the old code, if we sleep inside vm_page_grab_sleep(WAITFAIL), then vm_page_grab_sleep() returns false, in which case vm_page_grab() indeed returns NULL. That's consistent with vm_page_alloc*(). Where is the bug?

kib added a comment.Mar 24 2025, 1:40 AM
In D49391#1128152, @markj wrote:
In D49391#1128151, @dougm wrote:
In D49391#1128121, @markj wrote:
In D49391#1127942, @dougm wrote:
In D49391#1127890, @markj wrote:

The functional portion of the change seems fine, but I'm not sure why it's a problem for vm_page_grab* to support VM_ALLOC_WAITFAIL?

Because in comments on D49371, @kib suggested that VM_ALLOC_WAITFAIL was invalid here. So, to remove that obstacle to getting D49371 approved, I investigated and found that he might be right. Now, @alc has verbally placed a hold on this change because, when he is freed from another task, he plans to look for other illegal allocflag settings.

My reading of the comments is that kib is pointing out a bug in the new code.

He wrote, at D49371:
I am not sure about the semantic of VM_ALLOC_WAITFAIL. If we sleep inside vm_page_grab_lookup(), should we also return NULL for VM_ALLOC_WAITFAIL? (This is the pre-existing bug, if indeed a bug).

So I disagree that he is calling out a bug in new code.

Then I don't follow: in the old code, if we sleep inside vm_page_grab_sleep(WAITFAIL), then vm_page_grab_sleep() returns false, in which case vm_page_grab() indeed returns NULL. That's consistent with vm_page_alloc*(). Where is the bug?

I did pointed out that the new code handled NOWAIT improperly. I have no reason to request that the flag was disallowed for the function, but Doug' analysis demonstrated that no callers needed it, so I did not objected against the route of disallowing it.

dougm removed reviewers: kib, markj.Mar 28 2025, 10:02 PM
dougm abandoned this revision.Apr 26 2025, 6:43 PM
alc added a comment.Apr 26 2025, 8:15 PM

Aren't all of the changes to vm_page.h valid? Can we have a standalone patch with just those changes?

dougm updated this revision to Diff 154340.Apr 26 2025, 8:47 PM

Drop all but the header file changes.

dougm accepted this revision.Apr 26 2025, 8:48 PM
dougm foisted this revision upon alc.
dougm edited reviewers, added: dougm; removed: alc.
This revision is now accepted and ready to land.Apr 26 2025, 8:48 PM
alc updated this revision to Diff 157384.Jun 20 2025, 5:14 PM
alc added reviewers: kib, markj.
This revision now requires review to proceed.Jun 20 2025, 5:14 PM
alc retitled this revision from vm_grab: assert no VM_ALLOC_WAITFAIL to vm_page: update legend documenting allocation.Jun 20 2025, 5:16 PM
alc edited the summary of this revision. (Show Details)
alc updated this revision to Diff 157390.Jun 20 2025, 5:18 PM
kib added inline comments.Jun 20 2025, 5:35 PM
sys/vm/vm_page.c
2330

Wouldn't it be better to have e.g. VPA_COMMON flags without VM_ALLOC_NOFREE, and then use this to define VPA/VPAN/VPAC/VPANC without the ~ hack?

sys/vm/vm_page.h
499

s/flag/state/ ?

alc edited the summary of this revision. (Show Details)Jun 20 2025, 5:43 PM
alc marked 2 inline comments as done.Jun 20 2025, 11:24 PM
alc updated this revision to Diff 157400.Jun 20 2025, 11:27 PM

Introduce VM_ALLOC_COMMON.

Some wording changes.

kib accepted this revision.Jun 21 2025, 11:54 AM
This revision is now accepted and ready to land.Jun 21 2025, 11:54 AM
markj accepted this revision.Jun 23 2025, 1:24 PM
Closed by commit rG07297aee35f2: vm_page: update comments and KASSERT()s concerning page allocation (authored by alc). · Explain WhyJun 25 2025, 7:10 PM
This revision was automatically updated to reflect the committed changes.
alc added a commit: rG07297aee35f2: vm_page: update comments and KASSERT()s concerning page allocation.
Herald added a subscriber: imp. · View Herald TranscriptJun 25 2025, 7:10 PM

Revision Contents
Changeset List

PathSize
sys/
vm/
32 lines
36 lines

Diff 157384

View Options

sys/vm/vm_page.h

Loading...
View Options

sys/vm/vm_page.c

Loading...