Date: Tue, 26 Nov 2024 13:32:14 -0500 From: John Baldwin <jhb@FreeBSD.org> To: Christos Margiolis <christos@FreeBSD.org>, src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 5317480967bf - main - sound: Remove CHN_F_SLEEPING Message-ID: <77c4113c-c023-4db3-826a-83be4e9baa42@FreeBSD.org> In-Reply-To: <202411261448.4AQEmw7Y084208@gitrepo.freebsd.org> References: <202411261448.4AQEmw7Y084208@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?77c4113c-c023-4db3-826a-83be4e9baa42>