In order to support NUMA we will want an atomic allocate and sleep to provide tighter synchronization around allocation failures and sleeps. This patch moves most VM_WAIT consumers to use WAITOK/WAITFAIL and consequently eliminates a bunch of redundant code from callers.
Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
All my notes are cosmetics/style.
amd64/amd64/uma_machdep.c | ||
---|---|---|
53 | There is no reason to keep pflags variable, inline the call to malloc2vm_flags() into the arg. | |
arm64/arm64/uma_machdep.c | ||
53 | Same there. | |
powerpc/aim/mmu_oea64.c | ||
1520 | pflags is not needed there as well. | |
1525 | No need for {}. | |
powerpc/aim/slb.c | ||
492 | Same. | |
powerpc/powerpc/uma_machdep.c | ||
67 | Again pflags and {}. | |
vm/swap_pager.c | ||
1823 | It is somewhat pity to allocate the item and free it only to retry the non-wait allocation immediately after. Perhaps I will rewrite this fragment after your commit goes in. | |
vm/vm_fault.c | ||
1675 | I do not think this would work. For busy, we could need to sleep anyway, while hold does not prevent the free. Also, the object might be collapsed and src page copied. Might be the paging in progress count could be used there, but current simple handling of the low mem situation seems to be easiest. | |
vm/vm_page.c | ||
3240 | Please split the line and put spaces around '|'. |
Might be nice to define VM_ALLOC_WAITFLAGS to (VM_ALLOC_WAITOK|VM_ALLOC_NOWAIT|VM_ALLOC_WAITFAIL) and use it in places where you wish to clear all three.
vm_page_alloc(), vm_page_grab_pages() and vm_page_alloc_freelist() document the flags that they accept; those comments should be updated.
mips/mips/uma_machdep.c | ||
---|---|---|
55 | Looks like this is missing parens? Also the constants are misnamed, there's no "PAGE_". |
amd64/amd64/uma_machdep.c | ||
---|---|---|
53 | I prefer sometimes to split and use temporaries for legibility. | |
mips/mips/uma_machdep.c | ||
55 | Thank you. I will also make universe before I commit. | |
vm/swap_pager.c | ||
1823 | You can rewrite if you like. I don't think it's a big deal though since it only happens after failure and it's really just manipulating a stack in the fast path of the allocator. | |
vm/vm_fault.c | ||
1675 | The problem will come when VM_WAIT needs to do something sane for NUMA. Perhaps we could pass in the object that we're trying to allocate for so if it's not round-robin we sleep on the right domain. |
Overall, I think that this change is a good one. It will be a lot easier to perform a NUMA-aware VM_WAIT with this change.
arm64/arm64/uma_machdep.c | ||
---|---|---|
55 | The otherwise identical amd64 version above has dropped the unneeded braces. | |
mips/mips/uma_machdep.c | ||
57 | Please see my comment on vm_page_alloc_freelist() below. Essentially, in this new world order, the call to vm_page_reclaim_contig() should come from vm_page_alloc_freelist(). | |
powerpc/aim/slb.c | ||
493โ494 | There is nothing wrong with your change here, but I wanted to express skepticism about the correctness of the existing code because it does not call vm_page_reclaim_contig(). | |
powerpc/powerpc/uma_machdep.c | ||
62 | Can you please add the _NOOBJ flag here so that this version of uma_small_alloc() doesn't differ from the others in ways that don't reflect necessary machine-specific differences. | |
sparc64/sparc64/vm_machdep.c | ||
406 | Please drop the unnecessary braces so that these functions look the same except for required differences. And, move the _NOOBJ flag. | |
vm/vm_fault.c | ||
1675 | I think that this comment better explains the motivation for this change than the summary. Specifically, that VM_WAIT needs to know the NUMA allocation policy that was used by the failed allocation, and moving the VM_WAIT inside the allocation functions makes that easier. | |
vm/vm_page.c | ||
1814 | In this new world order, it would make sense for this function to call vm_page_reclaim_contig() rather than expecting the caller to do so. Doing so here (versus in the caller) has the advantage that we know whether we failed due a "global" shortage of pages or an absence within the correct range, so we can more selectively use vm_page_reclaim_contig(). | |
1980 | The same comment applies here as in vm_page_alloc_contig(). The difference is that this function would need to call a machine-dependent wrapper around vm_page_reclaim_contig() that understands the mapping from "free lists" to physical address ranges. | |
2573 | The style(9) police will want a blank line here. | |
2582 | It looks like there might be a stray space here. | |
3333โ3334 | Please put spaces around the "|" operator. | |
vm/vm_radix.c | ||
780 | Please insert a blank line here. |