Catch illegal M_WAITOK immediately.

[glebius]

Summary

Right now illegal M_WAITOK can easily leak into sources because
INVARIANTS would catch it only when it really would sleep(9). So
most test runs will succeed and only stress testing may catch it.

Fix that with assertion right in malloc.

[markj]

sys/kern/kern_malloc.c

622 ↗

(On Diff #75681)

Why not check it in UMA as well?

[kib]

Why it is not WITNESS_WARN() ? Perhaps witness_warn should take td_no_sleeping as arbitrary non-sleepable lock as well.

[glebius]

In D26027#577390, @kib wrote:

Why it is not WITNESS_WARN() ? Perhaps witness_warn should take td_no_sleeping as arbitrary non-sleepable lock as well.

Better to keep this check under INVARIANTS rather than under WITNESS.

First, WITNESS is slow and expensive, so many developers skip testing with WITNESS unless they introduce new locks or new locking schemes. Second, WITNESS is designed for testing lock orders mostly, but non-sleepability can be induced not only by locks, e.g. epoch.

[mjg]

I remember UMA warning about relevant allocations without even attempting to sleep, but maybe my memory is off here.

I think the key problem is that there are many places which opencode a variant of "must be able to go off cpu". Something like ASSERT_CAN_SLEEP() provided by INVARIANTS and possibly extended when WITNESS is present would be best in my opinion.

Then this patch would be:

if (flags & M_WAITOK)
    ASSERT_CAN_SLEEP();

[glebius]

Add same check to UMA.

[glebius]

In D26027#577448, @mjg wrote:

I remember UMA warning about relevant allocations without even attempting to sleep, but maybe my memory is off here.

It has this check in uma_zalloc_domain() with WITNESS and in general path with WITNESS & INVARIANTS.

[mjg]

So this indeed catches more stuff but panicking is too harsh. This should convert to just a warning for the foreseeable future.

[glebius]

I'm going to check in this patch again, but in warning mode rather than panic. Any objections? @markj @mjg

[mjg]

So I had a look and THREAD_CAN_SLEEP does not do the trick. For example it does not account for critical enter/exit. I did not check if it accounts for holding a mutex.

I definitely support making this into a warning.

[markj]

In D26027#798541, @glebius wrote:

I'm going to check in this patch again, but in warning mode rather than panic. Any objections? @markj @mjg

No objection from me.

Maybe add a debug knob to enable panics upon violation, so that it's easier to catch problems in automated testing.

[glebius]

This legacy revision has some metadata legacy from subversion times. The arc tool fails on it. Will reopen this revision with git-arc.