If parentp == NULL, node is not linked into the tree. I don't see how it ever gets freed, so I think it's a memory leak.

I see, in this case pctrie_insert_lookup_compound() uses existing nul leaf. Am I right?

I changed the interface to vm_radix_insert_prealloc() to take **node, to signal that the *node was not used.

I think this can be reached via mem_range_attr_set(), and because of the rwlock this will be a non-sleepable context.

BTW, do you have an explanation why it is safe to ignore the demote requests with len > 1G?

It seems that we can change mr_lock to sx. At least all callers are unlocked or in sleepable context.

I looked at the use of it in linuxkpi, and lkpi_arch_phys_wc_del() potentially might be called from the non-sleepable place, the wc_add() sleeps by itself. But I do not see a need in the split if we do MEMRANGE_SET_REMOVE(), since it was already demoted by the SET_UPDATE.

I think I see it myself. If some pdp is participating in the mapping of the region which we demote, then it must fully fit into the region, by the assertion that base is aligned at len.

In other words, this is indeed not a generic demotion.

This means you could fail in a case where an allocation wasn't needed, where currently you would succeed.

I assume that with this change, this line will never be reached.

Why can't

pmap_abort_ptp(pmap, va, pdpg);
return (KERN_RESOURCE_SHORTAGE);

appear here, instead of doing the early node allocation?

You are right.

Why not just call vm_radix_node_free(*node)) here, in the parentp==NULL case?

Yes. But IMO either conditions are very rare, and have the machine continuing running is more important.

We already cleared page table entries in the if/else block above. If we are going to return failure there, we must restore the mappings, at least for the kernel pmap. The rollback is harder.

Caller still needs to call node_free(), so it is slightly less code.
Also, the interface is symmetric this way: caller allocated, so it might need to free.

It's possible to have node == NULL here, but uma_zfree_smr(NULL) is not permitted.

It seems to be ok for both functions to sleep. They are probably only used by drm-kmod, and there the functions are called during driver init and deinit.

Should it be grouped with the other prototypes at the beginning of the file?

Here you are handling the case where *mp is null, but why? It cannot happen today, and in general it is probably better for the caller to handle allocation failures.

if mp == NULL, we allocate with pmap_alloc_pt_page(), which might fail.

I'm talking about the case *mp == NULL, not mp == NULL. If mp != NULL, then I think we must have pdpg != NULL here.

Then I do not understand what do you mean.

I have to handle the possibility of pdpg == NULL, and this is all what I do there. pdpg can be NULL if mp is NULL.

I do not see a place where I have a check for *mp == NULL as is.

I think the code should be structured like this:

if (mp == NULL) {
    pdpg = pmap_alloc_pt_page(...);
    if (pdpg == NULL)
        return (false);
} else {
    pdpg = *mp;
    ...
}

Well, this just tightens up the NULL check, but ok.

The most common use case for this function is pmap_enter_object(), and so only about one in 10,000 calls will actually perform the potentially problematic pmap_insert_pt_page(). The unnecessary allocations can easily be avoided by deferring the allocation until you are inside of the subsequent if statement. Put it right before the comment /* Break the existing mapping(s). */. Moreover, condition it on (oldpde & PG_PS) == 0 && va >= VM_MAXUSER_ADDRESS. Once you do this, then the much later call to pmap_insert_pt_page() that deals with wired mappings can revert back to its original behavior that does not assume a preallocated node. Most of the time, that preallocated node was not going to be used anyway, because the PTP is going to be referenced from an existing trie inode.

I think that we can be even more selective about node allocation than I suggested in my previous comment. If we are removing a kernel mapping here, then we are actually reinstalling the PTP with invalid mappings. In other words, the PTP was already in the trie. Instead, we could install an invalid PDE here, and leave the PTP in the trie. Then, ... (see below)

If we are about to call this function on the kernel pmap, then beforehand we try performing the existing pmap_insert_pt_page() without any of the proposed changes. In other words, I don't see any reason why we can't attempt to insert the kernel PTP before actually destroying the 4KB granularity mappings in the PTE. Concurrent access to the trie by another thread will be blocked by the pmap lock.

I don't see why the final argument can't simply be a vm_page_t, not a vm_page_t *. When a vm_page_t is given the function is guaranteed to succeed. The caller can just set m to NULL itself.

This can also fail. Unlike the regular kernel address space, we don't have preallocated page table pages for the direct map. In other words, we also need to preallocate a page table page for this demotion, but that only needs to happen if len < NBPDR. I will post suggested changes shortly.

I have posted suggested changes in D51091.

Should we perform a WARN_SLEEPOK alongside the KASSERTs?

If you move this if statement up inside the prior if statement, then the m = NULL statement can be eliminated.