This looks ok, thanks. Next time please upload patches with more context; see the comment about using -U99999 in https://wiki.freebsd.org/Phabricator#Create_a_Revision

Looks ok to me.

In D54338#1243198, @kib wrote:

In D54338#1243197, @markj wrote:

In D54338#1243187, @kib wrote:

Ping? This is a follow-up fix after D54263, and I want to commit them together.

Sorry, I am catching up on reviews today.

Does the patch need to be rebased? I can't apply it to main, the last two hunks in vm_object.c don't apply.

It is on top of D54263

There doesn't seem to be consensus on what to do here. I don't have any strong feelings myself; I don't personally see why it's useful to know the original author date (much like I don't care to see the full, original history from someone's local feature branch), and there are lots of other ways to lose that info (rebasing and commit splitting, as Warner alluded to).

In D54338#1243187, @kib wrote:

Ping? This is a follow-up fix after D54263, and I want to commit them together.

This works well, thanks. It would probably be useful to document/log the fact that if the test uses a execenv jail, then -x will attach the command to the jail.

In D54368#1243161, @markj wrote:

I don't understand. This patch is already applied to the tree, commit 35dd53a9e1326.

I don't understand. This patch is already applied to the tree, commit 35dd53a9e1326.

Add a test

This should be run through the test suite, in case tests are relying on this behaviour.

In D54292#1241041, @kp wrote:

DIOCGETRULENV takes the write lock as well but I believe this is only
required when clearing rule counters. (It might not be required even
then, on platforms where counter increment is done atomically.) Acquire
the read lock if that is not the case.

I believe the intent behind the write lock is to enable a userspace pattern of repeated fetch+clear without missing counts. If we'd take the read lock to retrieve the counters we could be incrementing them while this is going on, before we clear them again. That'd mean we'd miss packets/bytes/evaluations.

Approved. If this doesn't land soon, say in the next day or two, we should revert the original change until it does.

In D54268#1240216, @alc wrote:

Since we are starting to expand the usage of vmem, I want to point out something that I only noticed around the time we added support for nextfit allocation. What we call bestfit is really an implementation of firstfit allocation. (A comment pointing this out was actually deleted from the Netbsd original when vmem was imported.) And, what we call firstfit is what, I believe, Solaris calls instantfit. We have no actual implementation of bestfit allocation. That said, mav's long ago change to "create own free list for each of the first 32 possible allocation sizes" has the effect of bestfit allocation for small sizes.

Here is the comment that got deleted:

	} else { /* VM_BESTFIT */
		/*
		 * we assume that, for space efficiency, it's better to
		 * allocate from a smaller block.  thus we will start searching
		 * from the lower-order list than VM_INSTANTFIT.
		 * however, don't bother to find the smallest block in a free
		 * list because the list can be very long.  we can revisit it
		 * if/when it turns out to be a problem.

Sorry for the noise.

In D54258#1239826, @kib wrote:

This is only relevant for 32bit arches, am I right? (I do not claim that it is not a bug to fix).

In D54259#1239893, @jrtc27 wrote:

So the current code tries 10 times to hit a failure, and will use the last value if all 10 reads succeeded, given the condition after the loop is correct? Importantly, there's no security issue here, because we didn't actually report a successful read when it failed?

This fixes the problem for me, thank you.