Page MenuHomeFreeBSD

uma_zalloc: assert M_NOWAIT ^ M_WAITOK
ClosedPublic

Authored by vangyzen on Mar 5 2022, 12:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 5, 9:36 AM
Unknown Object (File)
Fri, Nov 15, 6:10 AM
Unknown Object (File)
Oct 29 2024, 3:23 PM
Unknown Object (File)
Sep 24 2024, 8:38 AM
Unknown Object (File)
Sep 24 2024, 2:08 AM
Unknown Object (File)
Sep 23 2024, 7:22 PM
Unknown Object (File)
Sep 23 2024, 4:04 PM
Unknown Object (File)
Sep 20 2024, 10:08 AM
Subscribers

Details

Summary

The uma_zalloc functions expect exactly one of [M_NOWAIT, M_WAITOK].
If neither or both are passed, print an error and a stack dump.
Only do this ten times, to prevent livelock. In the future, after
this exposes enough bad callers, this will be changed to a KASSERT().

MFC after: 1 month
Sponsored by: Dell EMC Isilon

Test Plan

The whole kyua suite completed without a panic or stack-dump.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 44670
Build 41558: arc lint + arc unit

Event Timeline

sys/vm/uma_core.c
3665

I love the irony of hard-coding M_WAITOK here.

sys/vm/uma_core.c
3665

Do we actually have to allocate one here? I thought that struct stack was small enough to be safe to be allocated directly from the stack.

vangyzen added inline comments.
sys/vm/uma_core.c
3665

No, we don't. Contrary to the little voice in my head, I tried using this KPI by reading _only the man page_, which clearly says stack_create is required. The src tree tells a different story, where only 1 out of 24 instances uses stack_create. From now on, I'll listen to the voices in my head. No...wait...

D34461 updates the man page.

This revision is now accepted and ready to land.Mar 7 2022, 2:39 PM
vangyzen marked an inline comment as done.

Thanks for the review, Ryan.

Mark, if you have time, I'd appreciate one more set of eyes.

This should go in uma_zalloc_debug() instead. Otherwise uma_zalloc_smr() isn't checked.

This should go in uma_zalloc_debug() instead. Otherwise uma_zalloc_smr() isn't checked.

Thank you! I also noticed that uma_zalloc_domain() does not call uma_zalloc_debug() in the multi-domain path.

This should go in uma_zalloc_debug() instead. Otherwise uma_zalloc_smr() isn't checked.

Thank you! I also noticed that uma_zalloc_domain() does not call uma_zalloc_debug() in the multi-domain path.

Yet another bug. :(

  • move to uma_zalloc_debug()
This revision now requires review to proceed.Mar 7 2022, 6:55 PM

I also noticed that uma_zalloc_domain() does not call uma_zalloc_debug() in the multi-domain path.

Fixed in D34472.

D34450 and D34451 fix the WAIT flags in two callers.

This revision is now accepted and ready to land.Mar 10 2022, 8:38 PM

I think the _Static_assert is overkill: yes, if one of the flags is defined to be zero then the assertion is always true, but that would be a legitimate implementation choice (similar to how O_RDONLY is 0) and the assertion would be harmless. In any case it's very unlikely to change. I don't really object though.

This revision was automatically updated to reflect the committed changes.