In D23763#522914, @dougm wrote:

This change makes the second argument to swp_pager_getswapspace redundant. A future change might as well eliminate it.

• No need for DEVOLATILE.
• pip_add may be called with a value of 0, handle this.

In D23782#522571, @tig_freebsdfoundation.org wrote:

It looks like it's just the allocated memory for labels, as most of the memory used by check_labels are given back (freed) according to the picture.
Kernel probably has a lot of assembly files with a lot of labels. Is memory usage a problem?

Rebase, add copyrights.

This is superseded by https://reviews.freebsd.org/rS332974 .

In D23774#522504, @kib wrote:

I very much dislike this. It repeats the mistake of KASSERT() while not eliminating KASSERT.
IMO the big and perhaps the only problem of KASSERT is that it was written before C99 so it cannot use variadic args. If the () around the printing part are dropped, then we can do any additions to the printed line without adding yet another set of macros.
So if you want to spend time on this, doing the convertion of KASSERT to variadic, and dropping the braces, would be a first step IMO. Personally I am fine with the churn.

I am fairly confident that none of the sysctls in this review require Giant, if only because Giant is not referenced at all in ath(9) or the ieee80211 stack. It is just a matter of going through and convincing oneself in each case. I did a few more, but if you prefer to just commit and revisit later, it is ok with me.

• Use func consistently in KASSERT()s.
• Rename "count" to "bc".

I am still open to suggestions regarding the name.

Fix the !INVARIANTS build.

In D23723#521189, @mjg wrote:

Thank you for working on this.
I think this is a good opportunity to introduce struct blockcount to prevent accidental misuse. Then BLOCKCOUNT_COUNT can be blockcount_read or so. Some casts will probably also drop.

• Move blockcount code into distinct headers. I did not add a copyright statement yet.
• Apply mjg's notes: handle Giant, fix assertion message.
• Add a blockcount_t to make the counter opaque. Add blockcount_read() to allow the counter value to be read (masking off the waiter flag).
• Add some barriers: blockcount_release() issues a release fence and _blockcount_wait() issues an acquire fence before returning.
• Avoid re-reading the counter value more than necessary when checking whether to go to sleep.

In D23729#521388, @rlibby wrote:

The issue in general is whether it is okay for objects from these zones to be backed by physical memory that is not contiguous, which UMA multipage slabs may cause unless the UMA_ZONE_CONTIG flag is specified or uma_zone_set_allocf is used.
Note that this is not a revert of the change in r357548, this is adding the UMA_ZONE_CONTIG flag unconditionally.

Can you explain why this makes a difference, at least for slbt_zone? I think I am missing something.

In D23723#521117, @jeff wrote:

I don't mind this in principal but I would rather it was a new api entirely. We need relatively few of the refcount features. Just acquiren/releasen/wait. So I'd rather have it named as some kind of barrier or semaphore.

In D23469#520752, @mjg wrote:

In D23469#515584, @markj wrote:

Some observations/personal feelings:

• I'm not sure about the int/long refcount split. How does a developer know which to choose? There is at least one situation (struct file) where we certainly must handle 32-bit overflows, but in general it seems hard to know whether it's necessary. What about ucred? We would have to look at all kernel resources which may hold a ucred reference. That said, I like the simplification that comes from not having to check fhold()'s return value, but I'm still not 100% convinced that it is really necessary to try and sanely handle the case (i.e., return EBADF) where a file ref count has overflowed _if_ we're also detecting saturation and we wrap all f_count operations using refcount(9).

ucred is an excellent example of something which if in legitimate danger of not fitting in an int should use long. crhold() is called all over with no error checking and trying to add it is probably a futile effort. At the same time the struct comes with void *cr_pspare2[2]; meaning we can convert int to long without growing it, all while not creating unfreeable objects due to saturation limit being reached.

Remove unintended diff.

Something is still needed to fix a "make -C sys/modules/dtrace/systrace". I will commit the patch I suggested earlier if no one has any objections.

So what about my comments in D23469? Why do we have to have 3 refcount APIs?

In D23699#520446, @kib wrote:

In D23699#520445, @markj wrote:

I don't see a problem with excluding the dtrace modules if KDTRACE_HOOKS is not defined, but the patch doesn't completely fix the problem. KDTRACE_HOOKS is a kernel option that means, "the kernel implements the interfaces required by dtrace.ko and friends." It doesn't make sense for the modules themselves to be modulated by this option.

Why ? This is exactly how options work, at least for the coherent build of kernel and modules.

I don't see a problem with excluding the dtrace modules if KDTRACE_HOOKS is not defined, but the patch doesn't completely fix the problem. KDTRACE_HOOKS is a kernel option that means, "the kernel implements the interfaces required by dtrace.ko and friends." It doesn't make sense for the modules themselves to be modulated by this option. Right now a standalone build of systrace.ko will fail because it doesn't define KDTRACE_HOOKS. With this patch it fails also, but for a different reason (SYSDIR is not defined in standalone builds).

Avoid busying valid pages, to avoid a deadlock involving
kernel stack pages.

In D23676#520058, @kib wrote:

In D23676#520034, @markj wrote:

I see that Linux has a non-blocking join, pthread_tryjoin_np(). Is that not sufficient for your purposes?

The tryjoin() method joins, while peekjoin() keeps the thread unjoined, which allows to peek or join again. The use for me was to see whether the thread exited, without disrupting an existing code that joined.
Our timedjoin() can be used as tryjoin() by providing the time in past. I can implement tryjoin() as well, if it is considered worth having explicit interface, to be source-compatible with Linux.

I see that Linux has a non-blocking join, pthread_tryjoin_np(). Is that not sufficient for your purposes?

If the object is dead, wait for pending paging operations
to complete before returning.

In D23666#519989, @kib wrote:

In D23666#519984, @markj wrote:

This change probably can't be MFCed.

Why ?

We could fix the problem in a different way, by changing swaponsomething() to allocate block ranges more carefully. The current algorithm just scans the list of existing swap devices and places the new one after the end of the last one. I think I will just commit the change and make the block range allocation smarter in a follow-up change.

swdevt is kernel-internal, more it is sys/vm internal. The external userspace view on it is provided by struct xswdev.

This change probably can't be MFCed. We could fix the problem in a different way, by changing swaponsomething() to allocate block ranges more carefully. The current algorithm just scans the list of existing swap devices and places the new one after the end of the last one. I think I will just commit the change and make the block range allocation smarter in a follow-up change.

Wake up sleepers when incrementing nsw_wcount_async.

I have no objection to this. I think the checks are cheap enough that they won't hurt, and after spending some time reading libalias I believe they are sufficient.

In D23501#517304, @tig_freebsdfoundation.org wrote:

2. I tested the performance with curlopc and with resetting cu to first cu every time(v2), below is the results:

r=reverse
seq=sequential
rand=random
v1 is addr2line with curlopc
v2 is the version that resets cu to first cu for every translation
10000seq stores 10k sorted addr of first 20k kernel addr.
1000rand stores 1k rand addr in all of kernel addr
r10000seq is 10000seq reversed
Which version should I keep?