Re: git: 5317480967bf - main - sound: Remove CHN_F_SLEEPING

On 11/26/24 06:48, Christos Margiolis wrote:
> The branch main has been updated by christos:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=5317480967bfc8bf678e4da3fce81bcb3f5b7836
> 
> commit 5317480967bfc8bf678e4da3fce81bcb3f5b7836
> Author:     Christos Margiolis <christos@FreeBSD.org>
> AuthorDate: 2024-11-26 14:48:36 +0000
> Commit:     Christos Margiolis <christos@FreeBSD.org>
> CommitDate: 2024-11-26 14:48:36 +0000
> 
>      sound: Remove CHN_F_SLEEPING
>      
>      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
>      Reviewed by:    dev_submerge.ch, markj
>      Differential Revision:  https://reviews.freebsd.org/D47559
> ---
>   sys/dev/sound/pcm/channel.c | 13 +------------
>   sys/dev/sound/pcm/channel.h |  4 ++--
>   sys/dev/sound/pcm/sound.c   |  4 ++--
>   3 files changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/sys/dev/sound/pcm/channel.h b/sys/dev/sound/pcm/channel.h
> index 79a8d35b22f7..d226adfba06b 100644
> --- a/sys/dev/sound/pcm/channel.h
> +++ b/sys/dev/sound/pcm/channel.h
> @@ -354,7 +354,7 @@ enum {
>   #define CHN_F_RUNNING		0x00000004  /* dma is running */
>   #define CHN_F_TRIGGERED		0x00000008
>   #define CHN_F_NOTRIGGER		0x00000010
> -#define CHN_F_SLEEPING		0x00000020
> +/* unused			0x00000020 */
>   
>   #define CHN_F_NBIO              0x00000040  /* do non-blocking i/o */
>   #define CHN_F_MMAP		0x00000080  /* has been mmap()ed */
> @@ -381,8 +381,8 @@ enum {
>   				"\002ABORTING"				\
>   				"\003RUNNING"				\
>   				"\004TRIGGERED"				\
> +				/* \006 */				\
>   				"\005NOTRIGGER"				\
> -				"\006SLEEPING"				\
>   				"\007NBIO"				\

Hmm, new comment is mis-sorted?

>   				"\010MMAP"				\
>   				"\011BUSY"				\
> diff --git a/sys/dev/sound/pcm/sound.c b/sys/dev/sound/pcm/sound.c
> index 4b7bcc64397f..218147b73db0 100644
> --- a/sys/dev/sound/pcm/sound.c
> +++ b/sys/dev/sound/pcm/sound.c
> @@ -221,8 +221,8 @@ pcm_killchans(struct snddev_info *d)
>   		/* Make sure all channels are stopped. */
>   		CHN_FOREACH(ch, d, channels.pcm) {
>   			CHN_LOCK(ch);
> -			if ((ch->flags & CHN_F_SLEEPING) == 0 &&
> -			    CHN_STOPPED(ch) && ch->inprog == 0) {
> +			if (ch->intr_cv.cv_waiters == 0 && CHN_STOPPED(ch) &&
> +			    ch->inprog == 0) {

I'm not super excited about reading cv_waiters directly.  Generally speaking
'struct cv' is opaque to the rest of the kernel.  Maybe add a little inline
routine or macro cv_waiters() that returns this value instead?  Then it can
be documented in condvar.9 along with the caveats about when it is safe to
use.

-- 
John Baldwin