My comments are not about this patch, but other issues with memguard.

kernel pmap. Specifically, I would propose clearing the processor's bit in the kernel pmap when it halts. That said, we would still need to have a separate flag indicating when the processor needs to take action when it wakes. (Depending on the documentation of the deeper sleep states maybe that action is a NOP.)

In D17249#367852, @markj wrote:

In D17249#367826, @alc wrote:

Here is some food for thought. We might be better of if we used the per-domain arenas differently. Suppose that the import function allocated the physical memory and mapped it. Further, suppose that we had a release function, and that function actually unmapped and freed physical memory. The benefits being: (1) we wouldn't have to wait for full population to trigger promotion, and (2) uma_reclaim() wouldn't trigger demotions (and hopefully later repromotions).

That's a cool idea. My main concern would be fragmentation, but kmem_* already use M_BESTFIT and we can at least easily measure the amount of physical memory wasted using vmem_size().

Here is some food for thought. We might be better of if we used the per-domain arenas differently. Suppose that the import function allocated the physical memory and mapped it. Further, suppose that we had a release function, and that function actually unmapped and freed physical memory. The benefits being: (1) we wouldn't have to wait for full population to trigger promotion, and (2) uma_reclaim() wouldn't trigger demotions (and hopefully later repromotions).

Do you plan to apply similar changes to the TLB invalidation code?

In D15055#364249, @kib wrote:

In D15055#364225, @alc wrote:

In D15055#324440, @kib wrote:

In D15055#324434, @jeff wrote:

So the consensus is this doesn't actually significantly optimize anything we can find? It certainly isn't worth extra faults in that case.

I am not sure about the consensus. You do have the test where this change helps, right ?

From the links that Alan posted, it seems that Linux also had the resistance to get the similar patch accepted, but ultimately they merged it. Might be we should add a knob to enable zero page mapping. Aside from the required changes to non-x86 pmap, it is rather easy to handle.

In regards to the 4KB zero page, the situation was actually the reverse. People were proposing the removal of the zero-page "optimization", arguing that it was not an effective optimization. Essentially, they had done similar measurements to ours. But, Linux has had this "optimization" from the beginning, before it had multiprocessor support (and the need for TLB shootdown). Linus resisted the proposal citing unspecified Fortran programs as the reason for keeping it.

A 2MB zero page was added to Linux based on claims that it reduced physical memory usage in one particular NAS benchmark (written in Fortran). However, Kostik tried this same program on FreeBSD with this patch and found no evidence that there was any physical memory savings.

I'll add that one of my graduate students has been doing tests with the NAS benchmarks, specifically, the variants for shared-memory multiprocessors using OpenMP. To their credit, the NAS benchmarks parallelize the initialization phase as well as the core computation. (Also, the parallelization of the initialization typically aligns well with the parallelization of the core computation, so a first-touch NUMA policy outperforms interleave.) So, any usage of the zero-page "optimization" where the page is written to and replaced by a physical page is going to require a system-wide TLB shootdown because the initialization is being done in parallel across the entire machine.

Clearly, one can write a program that uses less physical memory with the zero-page "optimization", but I don't think that we're going to come across a non-trivial number of real-world programs where that is the case. And, more often we're going to see cases where implementing the zero page is going to add run-time overhead. I would say that any knob enabling it should be per-process, perhaps an madvise option or mmap flag.

I do not think that it makes sense to add a knob that requires application to explicitly tune it. Both because I do not believe that any app would bother with this specifically for FreeBSD optimization, and because it adds a delicate code to handle never-used mode of operation.

Has anyone looked at armv7, where I believe lld is now the default? My impression is that the default image base there is 64 KB. If so, I would argue for updating it before we roll 12.0.

In (1), we will have zero code segment superpage mappings. The reduction in run time between (1) and (2) suggests that there are some "naturally-occurring" superpage mappings once we have properly aligned the code. This is consistent with what I see on amd64. The effect is just larger. To close the gap between (2) and (3), we'll have to introduce heuristics that will do the extra page-ins, even though the pages are not a part of a normally accessed chunk of the address space.

In D16836#365687, @markj wrote:

In D16836#365616, @gallatin wrote:

In D16836#365423, @markj wrote:

We already have code to renumber NUMA domains to avoid holes in the domain ID space (renumber_domains() in srat.c). Wouldn't it be simpler to apply a similar approach to empty domains, or is there some reason that won't work?

I'm not sure how renumbering helps. At least in my desktop, we have 4 domains. Domains 0 and 2 have both memory and CPU cores. Domains 1 and 3 are empty of memory but have CPU cores. If we renumber, don't we loose information?

Right, but how are we making use of that information now? I'm not sure what the SLIT looks like on the 2990WX (what does "sysctl vm.phys_locality" report?) but in other cases we might want to, based on locality information, treat all CPUs in domain 1 as being in domain 0, and CPUs in domain 3 as being in domain 2.

Anyway, I'm just concerned that this approach is a bit fragile given the number of places where we have to adjust a "domain" parameter. It looks ok to me modulo the comments below.

I think that this change should be abandoned.

In D12635#364226, @pho wrote:

In D12635#363496, @dougm_rice.edu wrote:

The only difference between this and what was posted in January is that that patch discarded code that has recently been found to be buggy, and this patch discards the fixed code.

I ran a full test with D12635.id47723.diff. No problems seen.

In D13644#363010, @markj wrote:

I believe this revision can be closed.

In D15055#324440, @kib wrote:

In D15055#324434, @jeff wrote:

So the consensus is this doesn't actually significantly optimize anything we can find? It certainly isn't worth extra faults in that case.

I am not sure about the consensus. You do have the test where this change helps, right ?

From the links that Alan posted, it seems that Linux also had the resistance to get the similar patch accepted, but ultimately they merged it. Might be we should add a knob to enable zero page mapping. Aside from the required changes to non-x86 pmap, it is rather easy to handle.

My policy question aside, I see no reason not to commit this change ASAP, once the two comments by Kostik about replacing the word "zone" with "domain" are addressed.

In D16976#362278, @kib wrote:

Why not coalesce phys_avail[] ranges instead ? I believe that we handle such range if it should be split between proximity domains, looking at the code in vm_phys_create_seg().

This revision can now be closed. (Do you have any idea why revisions, like this one, are no longer closing automatically?)

This change was committed.

Aside from my question about whether imin and imax should be reset, I think that this is change is correct and complete.

Update to reflect the reinstatement of drm{,2}.

Jeff, can you please close this review. This change was committed 6 weeks ago, but the commit didn't cite this review, so it wasn't automatically closed.

I would love to see this committed before we branch for 12.0.

Adding jtl@ because this change replaces the per-slab executable flag that he introduced by a per-page flag.

Eliminate the arena parameter to kmem_free().

I'm not sure why this revision wasn't closed automatically by the commit. The commit logs show that I cited this review.

Functionally, this change looks good to me. Do as you see fit with my earlier comments, and commit the change. l don't feel the need to review it again.

In D16799#357568, @jeff wrote:

In D16799#357567, @alc wrote:

In D16799#357566, @jeff wrote:

In D16799#357563, @alc wrote:

In D16799#357540, @jeff wrote:

I did not do this before because I felt the ROI was low compared to the churn in ports. Have we contacted ports maintainers? The nvidia and virtualbox port are both going to need #ifdefs.

I will followup with the ports maintainers. I had two reasons for this. Primarily, I want to implement stronger segregation for physical memory allocations that are permanent, e.g., physical pages backing UMA_ZONE_NOFREE. Specifically, I don't want to have to modify the kmem callers to specify a different arena if that arena is simply a placeholder. Secondarily, we use the arenas somewhat differently in older branches. So, mechanical MFCs may not do the correct thing anyway.

Do you intend to do kmem_malloc as well?

Yes, and kmem_free().

At this point in the release we should possibly also consult re@

Sure, and I'll make it clear that this is not a functional change.

If you'd like I can write an awk script to convert these and we can fix it up by hand if necessary. There are quite a lot of those locations.

In D16799#357566, @jeff wrote:

In D16799#357563, @alc wrote:

In D16799#357540, @jeff wrote:

I did not do this before because I felt the ROI was low compared to the churn in ports. Have we contacted ports maintainers? The nvidia and virtualbox port are both going to need #ifdefs.

I will followup with the ports maintainers. I had two reasons for this. Primarily, I want to implement stronger segregation for physical memory allocations that are permanent, e.g., physical pages backing UMA_ZONE_NOFREE. Specifically, I don't want to have to modify the kmem callers to specify a different arena if that arena is simply a placeholder. Secondarily, we use the arenas somewhat differently in older branches. So, mechanical MFCs may not do the correct thing anyway.

Do you intend to do kmem_malloc as well?

In D16799#357563, @alc wrote:

In D16799#357540, @jeff wrote:

I did not do this before because I felt the ROI was low compared to the churn in ports. Have we contacted ports maintainers? The nvidia and virtualbox port are both going to need #ifdefs.

I will followup with the ports maintainers. I had two reasons for this. Primarily, I want to implement stronger segregation for physical memory allocations that are permanent, e.g., physical pages backing UMA_ZONE_NOFREE. Specifically, I don't want to have to modify the kmem callers to specify a different arena if that arena is simply a placeholder.

In D16799#357540, @jeff wrote:

I did not do this before because I felt the ROI was low compared to the churn in ports. Have we contacted ports maintainers? The nvidia and virtualbox port are both going to need #ifdefs.

Increase the number of lines of context.

In D16793#357204, @kib wrote:

In D16793#357203, @alc wrote:

Should I bump __FreeBSD_version?

I would not.

In D16793#357198, @kib wrote:

kmem_malloc() needs sys_machdep.c update. I think it is better to keep them in separate changes.

I'm happy to either add kmem_malloc() and kmem_alloc_contig() to this patch or do them separately.

This should probably include an increment to __FreeBSD_version.

In D16713#355903, @markj wrote:

In D16713#355892, @alc wrote:

In D16713#355831, @markj wrote:

Do you plan to update the kmem_malloc() prototype at some point?

I would happily see the arena argument removed from all of these functions before we branch 12.x. kmem_free() is the only one where the arena isn't entirely redundant. Essentially, kmem_free() uses the arena as a flag to determine whether the addresses should be returned to the domain's kernel arena or rwx arena.

Do we have a consensus?

Are you proposing that we keep the arena parameter to kmem_free()?