vm_page: update legend documenting allocation

[alc]

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.

[kib]

sys/vm/vm_page.c

4660

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.

4706

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

[dougm]

Address @kib comments.

[alc]

sys/vm/vm_page.c

4706

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]

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

[alc]

The vm_page.h comment incorrectly says vm_page_alloc accepts VM_ALLOC_WAITOK.

[alc]

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]

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

[alc]

sys/vm/vm_page.h

489–490

[alc]

sys/vm/vm_page.h

464–465

vm_page_alloc_freelist() no longer exists.

481

[dougm]

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

[markj]

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

4664

4694

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]

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]

Apply @markj comments.

[markj]

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]

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]

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]

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.

[alc]

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

[dougm]

Drop all but the header file changes.

[kib]

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]

Introduce VM_ALLOC_COMMON.

Some wording changes.