I did some more testing and now I think that the change is wrong. In particular, note that the RACCT_PCPU resource has RACCT_IN_MILLIONS attribute, which explains why this multiplication occurs.

In D30778#692214, @domagoj.stolfa_gmail.com wrote:

In D30778#691982, @markj wrote:

If there are any tags, e.g., sponsored by, please add them to the review description and I'll commit.

You can add a Signed-off-by: domagoj.stolfa@gmail.com if you'd like

If there are any tags, e.g., sponsored by, please add them to the review description and I'll commit.

In D30764#691796, @ae wrote:

I think we can set *p.m unconditionally, like we do in ipfw_check_packet().

I think checking for TAILQ_EMPTY(&vsi->sysctl_ctx) is probably a hack. sysctl_ctx_free() handles an empty queue perfectly well. The problem seems to be that the list is not initialized (with sysctl_ctx_init()) in some paths where sysctl_ctx_free() may be called.

• Compare initiator and target LDTs, and only reload if they match.
• Use an acquire load in the comparison.

In D30685#689474, @kib wrote:

In D30685#689462, @markj wrote:

In D30685#689430, @kib wrote:

So what specifically the problem is? That the process might be forced to execute set_user_ldt() while md_ldt is actually still being set up?

Sorry for not explaining further. The problem is that set_user_ldt() may populate the LDT entry of the GDT with zeroes, and the subsequent lldt instruction raises #GP because the segment descriptor is not valid (I think because the descriptor type is wrong).

Ah, md_ldt_sd is zero, right. So yes I think the solution is to read md_ldt and only do something if md_ldt != NULL. But the read of md_ldt should have acq semantic to pair with fence_rel() in user_ldt_alloc().

I don't see any problems with the change, but note that this adds something like 30KB of data per thread. Probably not the end of the world but not ideal either.

In D30685#689430, @kib wrote:

So what specifically the problem is? That the process might be forced to execute set_user_ldt() while md_ldt is actually still being set up?

In D30644#688675, @arichardson wrote:

Can we also commit the test program from https://reviews.freebsd.org/D30550?

Add comments to pmap_promote_l2() noting that we do not invalidate the TLB.

Preserve AF, DBM and RW flags when copying 2MB pages.

In D30643#688707, @alc wrote:

You wrote, "Modify pmap_copy() to make new 2MB mappings read-only, like we do on amd64. I am not sure though why we shouldn't simply copy the dirty bit over to the child."

I'm confused. :-) I'm looking at the amd64 source code right now, and for 2MB page mappings it copies PG_A and PG_M. Moreover, it copies PG_RW. However, for 4KB page mappings, it does clear PG_A and PG_M. In other words, we don't handle different page sizes uniformly.

@whu ping. Do you plan to review this?

In D30649#688475, @jhb wrote:

Not sure if there any others for which suppressing the warning makes sense.

Try to better explain why it's ok to defer sfence.vma in pmap_promote_l2().

In D30643#688425, @kib wrote:

Don't we mark the copied mapping as clean to avoid unnecessary writes? Suppose that the source mapping is destroyed, and backing pages are marked dirty and written to the storage. Now, if the copied mapping is destroyed without ever being written to, we would re-dirty and write them again.

BTW, I did some buildkernel tests on a graviton instance and found no change in the number of promotions during a buildkernel. I can't think of any non-synthetic workloads that are likely to be affected in light of the fact that arm64 implements pmap_enter(psind=1), so faults on a shared mapping skip the promotion step, assuming that some entity triggered the initial promotion through writes.

Add a comment explaining why we downgrade protections and why we don't
execute sfence.vma.

In D30644#688379, @jrtc27 wrote:

I agree with your assessment about not needing sfence.vma, but that's probably worth a comment in the source.

In D30645#688383, @jrtc27 wrote:

I guess it got copied from arm's pmap-v4 (you can still find references that function for it in the tree)?

In D30550#687729, @freebsdphab-AX9_cmx.ietfng.org wrote:

Thanks to @markj for the explanation; this is an incorrect fix.

I would suggest noting in the description/commit log message that this estimate is used only for short-lived processes.