sound: Remove CHN_F_SLEEPING
AcceptedPublic
Actions

Authored by christos on Wed, Nov 13, 10:33 PM.

Details

Reviewers

markj
emaste
dev_submerge.ch
pho
dougm

Summary

The KASSERT in chn_sleep() can be triggered if more than one thread
wants to sleep on a given channel at the same time. While this is not
really a common scenario, tools such as stress2, which use fork() and
the child process(es) inherit the parent's FDs as a result, we can end
up triggering such scenarios.

Fix this by removing CHN_F_SLEEPING altogether, which is not very useful
in the first place:

CHN_BROADCAST() checks cv_waiters already, so there is no need to check CHN_F_SLEEPING as well.
We can check whether cv_waiters is 0 in pcm_killchans(), instead of whether CHN_F_SLEEPING is not set.

Reported by: dougm, pho (stress2)
Sponsored by: The FreeBSD Foundation
MFC after: 2 days

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 60627
Build 57511: arc lint + arc unit

Event Timeline

christos created this revision.Wed, Nov 13, 10:33 PM

Herald added a subscriber: imp. · View Herald TranscriptWed, Nov 13, 10:33 PM

christos requested review of this revision.Wed, Nov 13, 10:33 PM

Harbormaster completed remote builds in B60538: Diff 146395.Wed, Nov 13, 10:33 PM

Minor fix in CHN_F_EXCLUSIVE.

Harbormaster completed remote builds in B60539: Diff 146396.Wed, Nov 13, 10:35 PM

christos added a parent revision: D47462: sound: Fix hot-unload panics.Wed, Nov 13, 10:35 PM

@dougm @pho Could you please let me know if the panic persists with this patch?

I do net see any panics with D47559.146396.patch

Great. @markj what do you think?

emaste added inline comments.Fri, Nov 15, 2:30 PM

sys/dev/sound/pcm/channel.h
357	In general we don't renumber flags into now-unused slots. You can instead just leave a comment here indicating that 0x00000020 was CHN_F_SLEEPING

Address Ed's comment.

Harbormaster completed remote builds in B60627: Diff 146585.Sat, Nov 16, 12:41 PM

markj added inline comments.Sat, Nov 16, 4:55 PM

sys/dev/sound/pcm/sound.c
223	Checking the waiters field directly like this is a bit of a layering violation, though the sound driver framework already does this. In fact, `cv_waiters == 0` implies there are no waiters, but the converse is not true: `cv_waiters != 0` does not imply that there are waiters. What happens if the code below is executed and there are no waiters? Is it harmless?

christos added inline comments.Sat, Nov 16, 4:58 PM

sys/dev/sound/pcm/sound.c
223	In order to get past this loop we'll have to eventually execute this check without waiters for all channels, so I'm pretty sure it's fine.

christos added inline comments.Sat, Nov 16, 5:02 PM

sys/dev/sound/pcm/sound.c
223	Ah sorry, you meant what happens if the lines below this check are executed. Still, nothing. `chn_shutdown()` calls `CHN_BROADCAST()` which also checks for `cv_waiters`, and the rest shouldn't concern the value of `cv_waiters` anyway.