In D54785#1255050, @markj wrote:

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.

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

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

Addressed review feedback from @glebius.

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.

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.

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

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.

In D54785#1252125, @kib wrote:

This should be extremely useful, I already anticipate it.

Updated to incorporate review feedback.

Updated license and clarified this is only applicable to amd64 in main.

updates to incorporate review feedback

incorporate review feedback; rebase on top of changes to D52946

Reworked patch to use a large-ish buffer allocated from BSS and protected by a lock if called from an interrupt context. As noted in the commit message, this will result in increased lock contention during an interrupt storm which exceeds the capacity of the free list; however, overall lock contention should still be lower than it was when mca_log() was called with the mca_lock held.

In D52946#1210071, @lprylli_netflix.com wrote:

Not sure whether it needs to be addressed or not, but for completeness there is the possibility of some sequence of events occasionally emptying the mca_freelist, and then my understanding is that mca_record_entry() will work in "degraded mode" directly call mca_log() in an interrupt context (next to "MCA: Unable to allocate space for an event.\n"), and as long as that situation persists, the corresponding MCA printf() won't be ratechecked any more.

It could be a very unlikely theoretical case with the mca_postscan() changes, but with previous version of code, a couple of machines were able to trigger the "MCA: Unable to allocate space for an event" situation in netflix fleet.

Updating to apply cleanly on top of D52946, which introduces the use of sbuf to gather the log message.

Remove unneeded check for mca_startup_done

In D52943#1209843, @markj wrote:

Note that I am not 100% sure this can actually happen, since I am not 100% sure we will ever enable interrupts before we run mca_setup().

I don't quite follow, MCEs will be delivered even if interrupts are disabled. _mca_init() enables delivery of MCEs at SI_SUB_CPU time; what's wrong with calling mca_record_entry() after that point?

Fix logic in case (mode != polled && (mca_startup_done || (rec->mr_status & MC_STATUS_UC)))

In D52942#1209601, @jtl wrote:

Updated to hide this code under the DIAGNOSTIC kernel config option.

Updated to hide this code under the DIAGNOSTIC kernel config option.

update diff to account for 8 years of bitrot

AFAIK, no one has bemoaned the lack of this feature in the 8 years this has been sitting here. I think it is safe to abandon this.

Updating the diff to account for 8 years of bit rot.

incorporating review feedback

Thanks for the review! I agree on the documentation comment.

In D52872#1207896, @tuexen wrote:

In D52872#1207881, @jtl wrote:

Is this worthy of a release note?

Yes, the combination of D52871, D52872, and D52873.

My main concern about these three revisions is that they are somewhat susceptible of remote manipulation, and that could make it easier to DoS a server. However, I view that as a tradeoff that the user needs to make, and think an appropriate release note should suffice to warn about these issues.

My main concern about these three revisions is that they are somewhat susceptible of remote manipulation, and that could make it easier to DoS a server. However, I view that as a tradeoff that the user needs to make, and think an appropriate release note should suffice to warn about these issues.

Is this worthy of a release note?

If anyone is planning to review this (or if I should ask more people), let me know. Otherwise, I will probably just commit this soon. AFAICT, this is fairly innocuous (new feature with low risk of breaking existing things). But, there is a reason we get these things reviewed...

Thanks for doing this! LGTM other than one nit.