Page MenuHomeFreeBSD

witness: Provide facility to print detailed lock tree
ClosedPublic

Authored by jtl on Mon, Jan 19, 7:42 PM.
Tags
None
Referenced Files
F144657984: D54785.diff
Tue, Feb 10, 5:06 PM
Unknown Object (File)
Sun, Feb 8, 1:40 PM
Unknown Object (File)
Sun, Feb 8, 12:41 PM
Unknown Object (File)
Sun, Feb 8, 10:08 AM
Unknown Object (File)
Sun, Feb 8, 1:37 AM
Unknown Object (File)
Tue, Feb 3, 1:05 PM
Unknown Object (File)
Fri, Jan 30, 6:35 PM
Unknown Object (File)
Thu, Jan 29, 5:07 AM

Details

Summary
When witness(4) detects lock order reversals (LORs), it prints
information about the stack trace which caused the LOR. If available,
it can also print information about the first stack trace which
established the other lock ordering. However, it only does this for
"simple" LORs where the two locks in question were directly locked
in the opposite order. When the lock order was established through
a more complex pattern of intermediate locks, WITNESS only prints
the stack trace where it detected the LOR.

This commit provides new functionality to provide more verbose
information about the lock chain(s) which established the lock
ordering. The new functionality can be activated by setting the
debug.witness.trace sysctl/tunable to 2. The new functionality
is also available through the debug.witness.badstacks sysctl,
which has been modified to always show the more verbose
information.

Reviewed by:    <If someone else reviewed your modification.>
Sponsored by:   <If the change was sponsored by an organization.>
Differential Revision:  <https://reviews.freebsd.org/D###>
Test Plan

I wrote some kernel code to create a few LORs. With the changes in this diff, I see the following output on the console and in dmesg:

lock order reversal:
 1st 0xffff0000c29015b0 lle (lle, rw) @ /home/jtl/src/sys/kern/subr_witness.c:3501
 2nd 0xffff0000c2901590 so_rcv (so_rcv, sleep mutex) @ /home/jtl/src/sys/kern/subr_witness.c:3502
All lock orders from so_rcv -> lle:
"so_rcv" -> "tcphash" -> "in6_ifaddr_lock" -> "lle"
"so_rcv" -> "udphash" -> "in6_ifaddr_lock" -> "lle"
"so_rcv" -> "udphash" -> "lle"

Lock order "so_rcv"(sleep mutex) -> "tcphash"(sleep mutex) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004abc8c at __mtx_lock_flags+0x9c
#2 0xffff0000006b5260 at tcp6_usr_listen+0x188
#3 0xffff00000058b4b0 at solisten+0x48
#4 0xffff000000593c78 at kern_listen+0x74
#5 0xffff0000008bb31c at do_el0_sync+0x68c
#6 0xffff00000088e1ac at handle_el0_sync+0x4c

Lock order "tcphash"(sleep mutex) -> "in6_ifaddr_lock"(rm) first seen at:
(No stack trace--hardcoded relationship?)

Lock order "so_rcv"(sleep mutex) -> "udphash"(sleep mutex) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004abc8c at __mtx_lock_flags+0x9c
#2 0xffff000000550064 at sysctl_debug_witness_generate_lor+0xc4
#3 0xffff0000004e5ca0 at sysctl_root_handler_locked+0xe4
#4 0xffff0000004e4fec at sysctl_root+0x1e4
#5 0xffff0000004e5684 at userland_sysctl+0x140
#6 0xffff0000004e5508 at sys___sysctl+0x84
#7 0xffff0000008bb31c at do_el0_sync+0x68c
#8 0xffff00000088e1ac at handle_el0_sync+0x4c

Lock order "udphash"(sleep mutex) -> "in6_ifaddr_lock"(rm) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004ccbb4 at _rm_wlock_debug+0x78
#2 0xffff000000550100 at sysctl_debug_witness_generate_lor+0x160
#3 0xffff0000004e5ca0 at sysctl_root_handler_locked+0xe4
#4 0xffff0000004e4fec at sysctl_root+0x1e4
#5 0xffff0000004e5684 at userland_sysctl+0x140
#6 0xffff0000004e5508 at sys___sysctl+0x84
#7 0xffff0000008bb31c at do_el0_sync+0x68c
#8 0xffff00000088e1ac at handle_el0_sync+0x4c

Lock order "in6_ifaddr_lock"(rm) -> "lle"(rw) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004cf4c8 at __rw_rlock_int+0x8c
#2 0xffff0000006c9f30 at in6_lltable_lookup+0xe0
#3 0xffff0000006e98cc at nd6_lookup+0x74
#4 0xffff0000006f43f8 at find_pfxlist_reachable_router+0x88
#5 0xffff0000006f1e54 at pfxlist_onlink_check+0x4b4
#6 0xffff0000006f1740 at nd6_ra_input+0x116c
#7 0xffff0000006c16a0 at icmp6_input+0x900
#8 0xffff0000006dbf6c at ip6_input+0xf60
#9 0xffff00000063a67c at netisr_dispatch_src+0xd8
#10 0xffff00000061b9cc at ether_demux+0x174
#11 0xffff00000061cf20 at ether_nh_input+0x374
#12 0xffff00000063a67c at netisr_dispatch_src+0xd8
#13 0xffff00000061be14 at ether_input+0xdc
#14 0xffff000000696724 at tcp_lro_flush_all+0xec
#15 0xffff000000635c78 at iflib_rxeof+0xbc4
#16 0xffff00000062ff08 at _task_fn_rx+0xa4
#17 0xffff0000005221b8 at gtaskqueue_run_locked+0x164

Lock order "udphash"(sleep mutex) -> "lle"(rw) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004cf4c8 at __rw_rlock_int+0x8c
#2 0xffff0000005500b0 at sysctl_debug_witness_generate_lor+0x110
#3 0xffff0000004e5ca0 at sysctl_root_handler_locked+0xe4
#4 0xffff0000004e4fec at sysctl_root+0x1e4
#5 0xffff0000004e5684 at userland_sysctl+0x140
#6 0xffff0000004e5508 at sys___sysctl+0x84
#7 0xffff0000008bb31c at do_el0_sync+0x68c
#8 0xffff00000088e1ac at handle_el0_sync+0x4c


lock order lle -> so_rcv attempted at:
#0 0xffff00000054bfa8 at witness_checkorder+0xb60
#1 0xffff0000004abc8c at __mtx_lock_flags+0x9c
#2 0xffff000000550148 at sysctl_debug_witness_generate_lor+0x1a8
#3 0xffff0000004e5ca0 at sysctl_root_handler_locked+0xe4
#4 0xffff0000004e4fec at sysctl_root+0x1e4
#5 0xffff0000004e5684 at userland_sysctl+0x140
#6 0xffff0000004e5508 at sys___sysctl+0x84
#7 0xffff0000008bb31c at do_el0_sync+0x68c
#8 0xffff00000088e1ac at handle_el0_sync+0x4c
lock order reversal:
 1st 0xffff000001118270 jtl2 (jtl2, sleep mutex) @ /home/jtl/src/sys/kern/subr_witness.c:3514
 2nd 0xffff000001118250 jtl1 (jtl1, sleep mutex) @ /home/jtl/src/sys/kern/subr_witness.c:3515
lock order jtl1 -> jtl2 established at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004abc8c at __mtx_lock_flags+0x9c
#2 0xffff0000005501bc at sysctl_debug_witness_generate_lor+0x21c
#3 0xffff0000004e5ca0 at sysctl_root_handler_locked+0xe4
#4 0xffff0000004e4fec at sysctl_root+0x1e4
#5 0xffff0000004e5684 at userland_sysctl+0x140
#6 0xffff0000004e5508 at sys___sysctl+0x84
#7 0xffff0000008bb31c at do_el0_sync+0x68c
#8 0xffff00000088e1ac at handle_el0_sync+0x4c
lock order jtl2 -> jtl1 attempted at:
#0 0xffff00000054bfa8 at witness_checkorder+0xb60
#1 0xffff0000004abc8c at __mtx_lock_flags+0x9c
#2 0xffff00000055020c at sysctl_debug_witness_generate_lor+0x26c
#3 0xffff0000004e5ca0 at sysctl_root_handler_locked+0xe4
#4 0xffff0000004e4fec at sysctl_root+0x1e4
#5 0xffff0000004e5684 at userland_sysctl+0x140
#6 0xffff0000004e5508 at sys___sysctl+0x84
#7 0xffff0000008bb31c at do_el0_sync+0x68c
#8 0xffff00000088e1ac at handle_el0_sync+0x4c

And I see the following output when I run the sysctl:

jtl@freebsd:~ $ sysctl debug.witness.badstacks
debug.witness.badstacks: Number of known direct relationships is 736

Lock order reversal between "so_rcv"(sleep mutex) and "lle"(rw)!
All lock orders from "so_rcv"(sleep mutex) -> "lle"(rw):
"so_rcv" -> "tcphash" -> "in6_ifaddr_lock" -> "lle"
"so_rcv" -> "udphash" -> "in6_ifaddr_lock" -> "lle"
"so_rcv" -> "udphash" -> "lle"

Lock order "so_rcv"(sleep mutex) -> "tcphash"(sleep mutex) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004abc8c at __mtx_lock_flags+0x9c
#2 0xffff0000006b5260 at tcp6_usr_listen+0x188
#3 0xffff00000058b4b0 at solisten+0x48
#4 0xffff000000593c78 at kern_listen+0x74
#5 0xffff0000008bb31c at do_el0_sync+0x68c
#6 0xffff00000088e1ac at handle_el0_sync+0x4c

Lock order "tcphash"(sleep mutex) -> "in6_ifaddr_lock"(rm) first seen at:
(No stack trace--hardcoded relationship?)

Lock order "so_rcv"(sleep mutex) -> "udphash"(sleep mutex) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004abc8c at __mtx_lock_flags+0x9c
#2 0xffff000000550064 at sysctl_debug_witness_generate_lor+0xc4
#3 0xffff0000004e5ca0 at sysctl_root_handler_locked+0xe4
#4 0xffff0000004e4fec at sysctl_root+0x1e4
#5 0xffff0000004e5684 at userland_sysctl+0x140
#6 0xffff0000004e5508 at sys___sysctl+0x84
#7 0xffff0000008bb31c at do_el0_sync+0x68c
#8 0xffff00000088e1ac at handle_el0_sync+0x4c

Lock order "udphash"(sleep mutex) -> "in6_ifaddr_lock"(rm) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004ccbb4 at _rm_wlock_debug+0x78
#2 0xffff000000550100 at sysctl_debug_witness_generate_lor+0x160
#3 0xffff0000004e5ca0 at sysctl_root_handler_locked+0xe4
#4 0xffff0000004e4fec at sysctl_root+0x1e4
#5 0xffff0000004e5684 at userland_sysctl+0x140
#6 0xffff0000004e5508 at sys___sysctl+0x84
#7 0xffff0000008bb31c at do_el0_sync+0x68c
#8 0xffff00000088e1ac at handle_el0_sync+0x4c

Lock order "in6_ifaddr_lock"(rm) -> "lle"(rw) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004cf4c8 at __rw_rlock_int+0x8c
#2 0xffff0000006c9f30 at in6_lltable_lookup+0xe0
#3 0xffff0000006e98cc at nd6_lookup+0x74
#4 0xffff0000006f43f8 at find_pfxlist_reachable_router+0x88
#5 0xffff0000006f1e54 at pfxlist_onlink_check+0x4b4
#6 0xffff0000006f1740 at nd6_ra_input+0x116c
#7 0xffff0000006c16a0 at icmp6_input+0x900
#8 0xffff0000006dbf6c at ip6_input+0xf60
#9 0xffff00000063a67c at netisr_dispatch_src+0xd8
#10 0xffff00000061b9cc at ether_demux+0x174
#11 0xffff00000061cf20 at ether_nh_input+0x374
#12 0xffff00000063a67c at netisr_dispatch_src+0xd8
#13 0xffff00000061be14 at ether_input+0xdc
#14 0xffff000000696724 at tcp_lro_flush_all+0xec
#15 0xffff000000635c78 at iflib_rxeof+0xbc4
#16 0xffff00000062ff08 at _task_fn_rx+0xa4
#17 0xffff0000005221b8 at gtaskqueue_run_locked+0x164

Lock order "udphash"(sleep mutex) -> "lle"(rw) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004cf4c8 at __rw_rlock_int+0x8c
#2 0xffff0000005500b0 at sysctl_debug_witness_generate_lor+0x110
#3 0xffff0000004e5ca0 at sysctl_root_handler_locked+0xe4
#4 0xffff0000004e4fec at sysctl_root+0x1e4
#5 0xffff0000004e5684 at userland_sysctl+0x140
#6 0xffff0000004e5508 at sys___sysctl+0x84
#7 0xffff0000008bb31c at do_el0_sync+0x68c
#8 0xffff00000088e1ac at handle_el0_sync+0x4c


All lock orders from "lle"(rw) -> "so_rcv"(sleep mutex):
** "lle" -> "so_rcv"

Lock order "lle"(rw) -> "so_rcv"(sleep mutex) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004abc8c at __mtx_lock_flags+0x9c
#2 0xffff000000550148 at sysctl_debug_witness_generate_lor+0x1a8
#3 0xffff0000004e5ca0 at sysctl_root_handler_locked+0xe4
#4 0xffff0000004e4fec at sysctl_root+0x1e4
#5 0xffff0000004e5684 at userland_sysctl+0x140
#6 0xffff0000004e5508 at sys___sysctl+0x84
#7 0xffff0000008bb31c at do_el0_sync+0x68c
#8 0xffff00000088e1ac at handle_el0_sync+0x4c



Lock order reversal between "jtl1"(sleep mutex) and "jtl2"(sleep mutex)!
All lock orders from "jtl1"(sleep mutex) -> "jtl2"(sleep mutex):
"jtl1" -> "jtl2"

Lock order "jtl1"(sleep mutex) -> "jtl2"(sleep mutex) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004abc8c at __mtx_lock_flags+0x9c
#2 0xffff0000005501bc at sysctl_debug_witness_generate_lor+0x21c
#3 0xffff0000004e5ca0 at sysctl_root_handler_locked+0xe4
#4 0xffff0000004e4fec at sysctl_root+0x1e4
#5 0xffff0000004e5684 at userland_sysctl+0x140
#6 0xffff0000004e5508 at sys___sysctl+0x84
#7 0xffff0000008bb31c at do_el0_sync+0x68c
#8 0xffff00000088e1ac at handle_el0_sync+0x4c


All lock orders from "jtl2"(sleep mutex) -> "jtl1"(sleep mutex):
** "jtl2" -> "jtl1"

Lock order "jtl2"(sleep mutex) -> "jtl1"(sleep mutex) first seen at:
#0 0xffff00000054b774 at witness_checkorder+0x32c
#1 0xffff0000004abc8c at __mtx_lock_flags+0x9c
#2 0xffff00000055020c at sysctl_debug_witness_generate_lor+0x26c
#3 0xffff0000004e5ca0 at sysctl_root_handler_locked+0xe4
#4 0xffff0000004e4fec at sysctl_root+0x1e4
#5 0xffff0000004e5684 at userland_sysctl+0x140
#6 0xffff0000004e5508 at sys___sysctl+0x84
#7 0xffff0000008bb31c at do_el0_sync+0x68c
#8 0xffff00000088e1ac at handle_el0_sync+0x4c

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

jtl requested review of this revision.Mon, Jan 19, 7:42 PM
jtl added a subscriber: gallatin.

This should be extremely useful, I already anticipate it. But, why not make this information available by default? I am not even sure that the knob is needed to hide it.

In D54785#1252125, @kib wrote:

This should be extremely useful, I already anticipate it.

Great! Glad to hear it. I knew I would find it useful. I wasn't sure if others would.

But, why not make this information available by default? I am not even sure that the knob is needed to hide it.

If you are talking about changing the behavior of the debug.witness.badstacks sysctl, the main reason I didn't change the default behavior is that I don't know how people use that sysctl; therefore, I was hesitant to change it. A second reason is that I wasn't sure other people would appreciate the extra information as much as I would. :-) But, I am happy to simply change the behavior of the sysctl if the consensus is that we should do that.

If you are talking about changing the information printed to the console when a LOR is detected at run-time, there are a few reasons I was concerned about changing this: a) This new code adds a decent bit of extra processing which will make a WITNESS violation even more expensive, b) This code allows (bounded) recursion, which I am hesitant to add in unconstrained contexts, c) Printing extra information could mean a lot of the information scrolls off the console for people using graphical consoles, and d) I wasn't sure people would appreciate the extra information. :-)

FWIW, the same function (sbuf_print_witness_badstacks()) is called from a DDB command. Because of the recursion in the current version of the patch, I am hesitant to call it from anything but the sysctl context (where the current stack should be predictable and short). I can unroll the recursion into a loop, but I think that would hurt readability.

If we want to add this same information to the run-time checks printed on the console, I would likely switch those to defer most output to a taskqueue. But, modifying the code to print the extra information should be a minimal change.

I do not think that trying to schedule a task is very robust with a LOR detected. We are already in the situation risking the deadlock.
Also I do not think that the doubts of the stack overflow is very grounded, since WITNESS typically has spinlock recording disabled II believe there are not solved issues with console spinlock which in reality cause hard hang). So there is definitely the space for interrupt frame, and mode, when WITNESS running.

If you refuse to make this a default, I am asking to add a simple knob which would make this additional info to be printed in all situations.

In D54785#1252666, @kib wrote:

I do not think that trying to schedule a task is very robust with a LOR detected. We are already in the situation risking the deadlock.

Fair point!

If you refuse to make this a default, I am asking to add a simple knob which would make this additional info to be printed in all situations.

I don't "refuse"; I was just giving my thinking.

I'm currently working on changing the recursion into a loop; once I do that, I'm happy to add it everywhere.

In D54785#1252692, @jtl wrote:
In D54785#1252666, @kib wrote:

I do not think that trying to schedule a task is very robust with a LOR detected. We are already in the situation risking the deadlock.

Fair point!

If you refuse to make this a default, I am asking to add a simple knob which would make this additional info to be printed in all situations.

I don't "refuse"; I was just giving my thinking.

Ok, refuse was probably too strong word, sorry about this.

I'm currently working on changing the recursion into a loop; once I do that, I'm happy to add it everywhere.

Thank you.

Updates:

  • Switched the recursion into a loop
  • Added the ability to execute the new functionality when a run-time check fails
  • Merged the functionality into the existing debug.witness.badstacks sysctl (and identical show badstacks DDB command), removing the separate "verbose" functionality I had previously proposed adding
  • Updated the man page

Note that I left the default runtime functionality in its current form and made the new functionality optional (by setting debug.witness.trace to 2). I thought it would be best to give people a chance to try it out for a month or two before switching the default. As usual, I'm open to alternate opinions.

jtl edited the test plan for this revision. (Show Details)
sys/kern/subr_witness.c
3091

Why such large buffer? There is a drain routine used by sbuf, so can the buffer be much smaller?

This revision is now accepted and ready to land.Wed, Jan 21, 6:28 PM
sys/kern/subr_witness.c
3091

As currently implemented, the sbuf_print_witness_badstacks() function restarts (and clears the sbuf) anytime the generation changes. (This was the pre-existing behavior and I didn't change it.) With the small buffer, this was likely to cause very odd output when a generation change occurred. Because I increased the amount of output the sbuf_print_witness_badstacks() function produces, I increased this static buffer. But, the underlying issue is still there and may need to be addressed at some point.

Updated the DDB "show badstacks" code to not check for generation changes while printing. We shouldn't generally see generation changes when running DDB commands. Even if we do (for example, because of the DDB code itself acquiring locks), we should be able to safely ignore the changes.

This revision now requires review to proceed.Thu, Jan 22, 3:33 PM

Other than updating the manpage date, I *think* this patch is now in its final form and ready to land. I am happy to wait for more reviews if anyone wants to review it. If I don't hear otherwise, I'll probably commit this late tomorrow.

sys/kern/subr_witness.c
3091

I decided to fix this behavior while I was here anyway. I have restored the previous (small) buffer.

This revision is now accepted and ready to land.Thu, Jan 22, 3:49 PM
glebius added inline comments.
sys/kern/subr_witness.c
1110

I'd suggest to inline this function together with free_verbose_tracker(). It is called just two in two places and malloc flags are different. The inlined version won't need obvious KASSERT and won't need a check for NULL in one of the places. A new reader of the code would expect that get_verbose_tracker/free_verbose_tracker is doing something more complicated than malloc/free, they would jump to the function to check. When malloc/free is inlined into witness_checkorder() a reader won't need to jump to implementations.

Also, it hides M_TEMP allocation. Our normal style of using M_TEMP is that malloc(M_TEMP) and free(M_TEMP) happen in the same function.

sys/kern/subr_witness.c
1130–1131

If I checked everything correct you can assert it is not NULL.

2896

Is always assigned down below.

I agree with Kostik that this nice feature should be enabled by default.

jtl marked 3 inline comments as done.Sat, Jan 24, 6:07 PM
jtl added inline comments.
sys/kern/subr_witness.c
1110

I moved the allocations/frees in line. I kept the initialization portions of this.

1130–1131

Yes, you are correct. I changed this.

2896

That is true. But it is also true of the other variables. So, I've deleted all of these NULL initializers.

jtl marked 3 inline comments as done.Sat, Jan 24, 6:07 PM

Addressed review feedback from @glebius.

This revision now requires review to proceed.Sat, Jan 24, 6:17 PM
This revision is now accepted and ready to land.Sun, Jan 25, 3:11 AM
sys/kern/subr_witness.c
1501

There is a case where witness prints an LOR warning because one acquired a sleepable lock (e.g., sx or lockmgr lock) after a non-sleepable lock. In that case there may not be a lock order in the opposite direction, so the new code doesn't print anything here except the "All lock orders from ..." header, which seems a bit confusing.

Can we detect this case (e.g., by setting a flag in the "sleepable after non-sleepable" case above) and avoid printing anything here?

3053

Extra semicolon here.

sys/kern/subr_witness.c
1507

This ends up printing two consecutive newlines. Is that intentional?

jtl marked 3 inline comments as done.Mon, Jan 26, 3:43 PM

@markj Thanks for the review! Do you have any thoughts on enabling this by default right away vs. waiting a month or so?

sys/kern/subr_witness.c
1501

Good catch! I have both added a test case to the tests I've been running and fixed this.

FWIW, this same problem also impacted the sysctl output, so I've also fixed that.

1507

Yes, it was. It provided better visual separation between multiple events. However, I can see why that is not as valuable here as in the sysctl output. I've removed it.

jtl marked 2 inline comments as done.

Addressed @markj's review feedback. Also, fixed the check for witness_trace > 1 which had gotten lost somewhere along the way.

This revision now requires review to proceed.Mon, Jan 26, 3:57 PM
In D54785#1254929, @jtl wrote:

@markj Thanks for the review! Do you have any thoughts on enabling this by default right away vs. waiting a month or so?

I think you should just go for it, maybe with a long MFC period.

Thanks for doing this, it's quite useful.

sys/kern/subr_witness.c
1312

I don't think it should block landing the diff, but it should be pretty easy to determine whether the order is in fact encoded in order_lists[]. I saw a couple of warnings where that's the case. IMO it'd be good to have a separate message for that case so that it's easier to spot truly anomalous cases.

This revision is now accepted and ready to land.Mon, Jan 26, 7:15 PM
In D54785#1254929, @jtl wrote:

@markj Thanks for the review! Do you have any thoughts on enabling this by default right away vs. waiting a month or so?

I think you should just go for it, maybe with a long MFC period.

OK. Given you all seem to agree, I'll change the default and send a note to freebsd-current as a heads up.

Thanks for doing this, it's quite useful.

NP; I'm glad it seems useful.

sys/kern/subr_witness.c
1312

ACK. I'll plan that as a follow-up commit.