sound: Do not access cv_waiters
ClosedPublic
Actions

Authored by christos on Nov 27 2024, 3:32 PM.

Details

Reviewers

markj
emaste
jhb
avg
dev_submerge.ch

Commits

rG134a275eea7a: sound: Do not access cv_waiters
rG46a97b9cd6fd: sound: Do not access cv_waiters

Summary

Remove uses of cv_waiters in PCM_RELEASE and CHN_BROADCAST, and also use
a counter around cv_timedwait_sig() in chn_sleep(), which is checked in
pcm_killchans(), as opposed to reading cv_waiters directly, which is a
layering violation.

While here, move CHN_BROADCAST below the channel lock operations.

Reported by: avg, jhb, markj
Sponsored by: The FreeBSD Foundation
MFC after: 2 days

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

christos created this revision.Nov 27 2024, 3:32 PM

Herald added a subscriber: imp. · View Herald TranscriptNov 27 2024, 3:32 PM

christos requested review of this revision.Nov 27 2024, 3:32 PM

Harbormaster completed remote builds in B60814: Diff 147036.Nov 27 2024, 3:32 PM

christos added a parent revision: D47779: condvar: Add cv_waiters() macro.Nov 27 2024, 3:32 PM

christos abandoned this revision.Nov 27 2024, 3:46 PM

christos reclaimed this revision.Nov 27 2024, 4:49 PM

christos retitled this revision from sound: Do not access cv_waiters directly to sound: Do not access cv_waiters.

christos edited the summary of this revision. (Show Details)

christos added reviewers: avg, dev_submerge.ch.

Do not use cv_waiters at all.

Harbormaster completed remote builds in B60840: Diff 147065.Nov 27 2024, 4:50 PM

Looks goof to me.
Thank you!

This revision is now accepted and ready to land.Nov 27 2024, 6:11 PM

avg added inline comments.Nov 27 2024, 6:13 PM

sys/dev/sound/pcm/sound.c
224–225	From a quick look, this appears to be an optimization. But I can be wrong about that. Would anything break or hurt user experience if this fast path was removed?

christos added inline comments.Nov 27 2024, 6:35 PM

sys/dev/sound/pcm/sound.c
224–225	I cannot give you answer in concrete numbers as I haven't benchmarked this specific path. The reason I have added this check is to not issue redundant calls to `chn_shutdown()` and `chn_flush()`/`chn_abort()` if the channel is already completely stopped. With large numbers of channels removing this check could potentially hurt performance I guess. Why do you think it should be removed?

avg added inline comments.Nov 27 2024, 6:38 PM

sys/dev/sound/pcm/sound.c
224–225	I was just wondering.

christos added inline comments.Nov 27 2024, 6:38 PM

sys/dev/sound/pcm/sound.c
224–225	Also this is an easy way to tell if we need to re-try stopping the channels (see how `again` is used), so there has to be some kind of check IMHO anyway.