Page MenuHomeFreeBSD

Catch illegal M_WAITOK immediately.
AbandonedPublic

Authored by glebius on Aug 11 2020, 3:49 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 29, 5:27 AM
Unknown Object (File)
Fri, Nov 8, 7:39 AM
Unknown Object (File)
Oct 6 2024, 7:43 PM
Unknown Object (File)
Oct 3 2024, 11:34 AM
Unknown Object (File)
Sep 17 2024, 12:40 PM
Unknown Object (File)
Sep 7 2024, 7:42 AM
Unknown Object (File)
Sep 4 2024, 11:33 PM
Unknown Object (File)
Sep 1 2024, 12:35 PM
Subscribers

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 32915
Build 30313: arc lint + arc unit

Event Timeline

sys/kern/kern_malloc.c
622

Why not check it in UMA as well?

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

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.

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();
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.

This revision is now accepted and ready to land.Aug 12 2020, 2:38 PM

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

I'm going to check in this patch again, but in warning mode rather than panic. Any objections? @markj @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.

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.

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