I think I've brought this up before, but I would like it if the VM had a generic per-2MB page structure. We already have several in vm_reserv and the pmap, and IMO it would be a good place to maintain a "compound" busy state, rather than in the object. I worry that a mechanism to block the busying of all pages in an object will inhibit concurrency and lead to transient latency spikes. I don't object to the current approach though.

In D21669#473159, @cem wrote:

Both remarks seem reasonable to me. I wasn’t going to drop this in the tree without circulating the idea past net@ at a minimum 😀. But I wanted some feedback from a smaller audience first— thank you both.
One additional possibility would be to define mtod in such a way that either 1 or 2 argument versions continue to compile, as an easier onramp for out of tree consumers. (I believe this is possible but haven’t yet confirmed so.) It would be an ugly macro hack, but importantly the ugliness would just be in one place. If that is possible, do either of you have an opinion on whether that seems worse or better than requiring a flag day for out of tree consumers?

I have no objections to the change in principle, it's a nice cleanup. It would cause a massive amount of churn: we have about 2000 uses of mtod() in the tree, and downstream trees will have many thousands more. I think such a change deserves a wider discussion on -arch or so.

I don't see why this is sufficient. If the unlocked translation attempt fails, e.g., because the mapping is being promoted or demoted a second time, we may still attempt to acquire a mutex in a scenario where that is not allowed.

In D21642#472503, @alc wrote:

Yes, I put the const in the wrong place. So, code within vm_page.c will avoid the dereference, but code outside vm_page.c that uses the declaration from vm_page.h will still perform the dereference. Declaring vm_page_array as an array would deal with both cases.

In D21658#472502, @mjg wrote:

I already noted pmcstat needs serious help. I think adding this with (the optional key swap) is solves the immediate problem of not having the feature while not having to spend a significant amount of time writing a new tool from scratch.

In D21642#472496, @alc wrote:

In D21642#472480, @markj wrote:

In D21642#472450, @alc wrote:

We could go further and make vm_page_array a const pointer on amd64, ...

If you start down this path, I would go a step further: Declare vm_page_array as an array, instead of a pointer, and define the symbol's value to VM_MIN_KERNEL_ADDRESS. This would eliminate the memory dereference to retrieve the start of the array.

Sorry, I don't quite see what you are proposing. My suggestion also eliminated the memory dereference, but I don't see how declaring vm_page_array as an array would help anything.

I'm afraid not. Consider this simplified example.

const int *ptr = (int *)0x10000000;
...

In D21642#472450, @alc wrote:

We could go further and make vm_page_array a const pointer on amd64, ...

If you start down this path, I would go a step further: Declare vm_page_array as an array, instead of a pointer, and define the symbol's value to VM_MIN_KERNEL_ADDRESS. This would eliminate the memory dereference to retrieve the start of the array.

In D21658#472473, @mjg wrote:

I think pmcstat needs serious help and the compatibility change (which I highly doubt hurts anyone) is a step towards that goal. I want a usable tool without the need to rewrite current one, if it can be helped. Should someone want to write something new from scratch I'm happy to see it, but I don't want to the work myself.

In D21658#472446, @mjg wrote:

The option was added at my request and highly doubt anyone else is using it (including the big user). I highly doubt any user will get upset over this change, but worst case I can drop the swap.

In D21658#472440, @mjg wrote:

I think it fits better. 'A' for address, 'I' for instruction. If there is a time to change it, it is now. I would not do it if pmcstat was actually in widespread use and people had the opt ingrained in their fingers.

Restore lost comment.

Why did you change the meaning of -I instead of putting the new functionality under a new option?

I have no objection to this with a separate header and man page. It does seem convenient to have a map structure that can be copied with memcpy() or copyout() or so.

• Remove PMAP_PA_ADDRESS.

In D21617#471544, @mjg wrote:

I don't care for the name for the most part, but perhaps something not sounding so refcounty will make it easier to not accidentally use the wrong API. Alternatively, perhaps it's time we de-u_int all consumers and have something of this sort:

struct refcount {
    u_int count;
};

with open-coded access prohibited.

In D21617#471527, @mjg wrote:

So I had a closer look and think that the introduction of support for waiting was an abuse of this API and should either be moved to another one or get dedicated routines in this one. Most consumers (including very frequently used ones like struct file) don't care for it and avoidably pay the price for its existence.

1. there is a refcount_wake unnecessary scattered, but hopefully most of the time it is only at the end of the func and has to be jumped to
2. the range is halved because of the bit needed to know if there are waiters. this means various consumers will have to switch 32 -> 64 bit sooner or abandon the API in the first place

That said, I suggest a different namespace (refcountw?) for this and reverting refcount.h to pre-r351188.

Address most of the feedback from Jeff.

Any reason not to commit this?

Does this fix an observed bug? The waiter bit should only be set if the counter value is non-zero, so the comparisons with 0 should work correctly regardless. I agree with the change though.

In D21460#470271, @cem wrote:

@markj I've moved over discussion of netdump_configure() / CURVNET / etc from D21482 to here. I'll admit I don't understand VNETs at all, so I'm pretty naive about what is reasonable where. Given you have some idea of what the right thing to do is, I'm totally happy to do just do that.

In D21566#470372, @kib wrote:

In D21566#470363, @markj wrote:

The change LGTM. I would consider keeping the vm_fault() wrapper for vm_fault_hold(), to avoid churn if vm_fault_hold() ever needs to grow additional arguments.

There is only one caller of vm_fault() outside of the trap_pfault()s. Note that vm_map_wire() use of vm_fault() caused ktrace recording the wired pages, which IMO is not right.

The change LGTM. I would consider keeping the vm_fault() wrapper for vm_fault_hold(), to avoid churn if vm_fault_hold() ever needs to grow additional arguments.

The comment I made in D21482 about configuration from outside vnet0 really belongs here, but the change otherwise LGTM.

In D21510#470181, @cem wrote:

Any objection in principle? Planning to review or ENOTIME? Suggestions for reviewers with free time and interest in this layer of the network stack?

Approved.

Sorry for the delay.