Differential D32036

vm_fault: Stop specifying VM_ALLOC_ZERO
ClosedPublic
Actions

Authored by markj on Sep 21 2021, 2:45 AM.

Details

Reviewers

alc
kib

Commits

rGb801c79dda53: vm_fault: Stop specifying VM_ALLOC_ZERO

Summary

Now vm_page_alloc() and friends will unconditionally preserve PG_ZERO,
so there is no point in setting this flag.

Eliminate a local variable and add a comment explaining why we
prioritize the allocation when the process is doomed. I remember that
it took me some time to understand this when I first read the code long
ago.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 41614
Build 38503: arc lint + arc unit

Event Timeline

markj created this revision.Sep 21 2021, 2:45 AM

Herald added a subscriber: imp. · View Herald TranscriptSep 21 2021, 2:45 AM

markj requested review of this revision.Sep 21 2021, 2:45 AM

Harbormaster completed remote builds in B41614: Diff 95417.Sep 21 2021, 2:45 AM

markj added a parent revision: D32035: Rewrite the vm_page_alloc manual page.Sep 21 2021, 2:45 AM

markj added a child revision: D32037: Remove remaining references to VM_ALLOC_NOOBJ.

kib accepted this revision.Sep 21 2021, 12:45 PM

This revision is now accepted and ready to land.Sep 21 2021, 12:45 PM

As an aside, I still think that vm_page_alloc() and other allocators which insert into an object should handle VM_ALLOC_ZERO by zeroing the page. The argument against that is that when used carelessly we end up zeroing pages with the object lock held. But:

Many calls come via vm_page_grab(), which does exactly that anyway. kmem_back_domain() also zeros pages with the kernel object lock held, though I'm not certain about whether this is required.
There are far fewer vm_page_alloc() calls now, making such pessimizations easier to spot.
It seems undesirable to handle VM_ALLOC_ZERO inconsistently between different page allocator interfaces.

Then might be, change the semantic of VM_ALLOC_ZERO to unconditionally zero page, and add e.g. VM_ALLOC_PGZERO which preserves PG_ZERO flag?

In D32036#724558, @kib wrote:

Then might be, change the semantic of VM_ALLOC_ZERO to unconditionally zero page, and add e.g. VM_ALLOC_PGZERO which preserves PG_ZERO flag?

Note that in D32033 and D32034 vm_page_alloc() and vm_page_alloc_contig() were changed to unconditionally preserve PG_ZERO. Alan suggested this in D28805. So there is no need for VM_ALLOC_PGZERO.

In D32036#724560, @markj wrote:

In D32036#724558, @kib wrote:

Then might be, change the semantic of VM_ALLOC_ZERO to unconditionally zero page, and add e.g. VM_ALLOC_PGZERO which preserves PG_ZERO flag?

Note that in D32033 and D32034 vm_page_alloc() and vm_page_alloc_contig() were changed to unconditionally preserve PG_ZERO. Alan suggested this in D28805. So there is no need for VM_ALLOC_PGZERO.

Then everything is already handled, isn't it?

In D32036#724570, @kib wrote:

In D32036#724560, @markj wrote:

In D32036#724558, @kib wrote:

Then might be, change the semantic of VM_ALLOC_ZERO to unconditionally zero page, and add e.g. VM_ALLOC_PGZERO which preserves PG_ZERO flag?

Note that in D32033 and D32034 vm_page_alloc() and vm_page_alloc_contig() were changed to unconditionally preserve PG_ZERO. Alan suggested this in D28805. So there is no need for VM_ALLOC_PGZERO.

Then everything is already handled, isn't it?