Module Name: src
Committed By: isaki
Date: Thu May 23 12:20:27 UTC 2019
Modified Files:
src/sys/dev/audio: audio.c audiodef.h
Log Message:
Remove unnecessary file lock.
It has been introduced to prevent multiple syscalls entering
simultaneously. But it's completely unnecessary.
It fixes firefox problem in PR kern/54177.
To generate a diff of this commit:
cvs rdiff -u -r1.8 -r1.9 src/sys/dev/audio/audio.c
cvs rdiff -u -r1.2 -r1.3 src/sys/dev/audio/audiodef.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/dev/audio/audio.c
diff -u src/sys/dev/audio/audio.c:1.8 src/sys/dev/audio/audio.c:1.9
--- src/sys/dev/audio/audio.c:1.8 Tue May 21 12:52:57 2019
+++ src/sys/dev/audio/audio.c Thu May 23 12:20:27 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: audio.c,v 1.8 2019/05/21 12:52:57 isaki Exp $ */
+/* $NetBSD: audio.c,v 1.9 2019/05/23 12:20:27 isaki Exp $ */
/*-
* Copyright (c) 2008 The NetBSD Foundation, Inc.
@@ -131,14 +131,7 @@
* neither lock were necessary. Currently, on the other hand, since
* these may be also called after attach, the thread lock is required.
*
- * In addition, there are two additional locks.
- *
- * - file->lock. This is a variable protected by sc_lock and is similar
- * to the "thread lock". This is one for each file. If any thread
- * context and software interrupt context who want to access the file
- * structure, they must acquire this lock before. It protects
- * descriptor's consistency among multithreaded accesses. Since this
- * lock uses sc_lock, don't acquire from hardware interrupt context.
+ * In addition, there is an additional lock.
*
* - track->lock. This is an atomic variable and is similar to the
* "interrupt lock". This is one for each track. If any thread context
@@ -149,7 +142,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: audio.c,v 1.8 2019/05/21 12:52:57 isaki Exp $");
+__KERNEL_RCSID(0, "$NetBSD: audio.c,v 1.9 2019/05/23 12:20:27 isaki Exp $");
#ifdef _KERNEL_OPT
#include "audio.h"
@@ -507,8 +500,6 @@ static void audio_softintr_wr(void *);
static int audio_enter_exclusive(struct audio_softc *);
static void audio_exit_exclusive(struct audio_softc *);
static int audio_track_waitio(struct audio_softc *, audio_track_t *);
-static int audio_file_acquire(struct audio_softc *, audio_file_t *);
-static void audio_file_release(struct audio_softc *, audio_file_t *);
static int audioclose(struct file *);
static int audioread(struct file *, off_t *, struct uio *, kauth_cred_t, int);
@@ -1431,57 +1422,6 @@ audio_track_waitio(struct audio_softc *s
}
/*
- * Acquire the file lock.
- * If file is acquired successfully, returns 0. Otherwise returns errno.
- * In both case, sc_lock is released.
- */
-static int
-audio_file_acquire(struct audio_softc *sc, audio_file_t *file)
-{
- int error;
-
- KASSERT(!mutex_owned(sc->sc_lock));
-
- mutex_enter(sc->sc_lock);
- if (sc->sc_dying) {
- mutex_exit(sc->sc_lock);
- return EIO;
- }
-
- while (__predict_false(file->lock != 0)) {
- error = cv_wait_sig(&sc->sc_exlockcv, sc->sc_lock);
- if (sc->sc_dying)
- error = EIO;
- if (error) {
- mutex_exit(sc->sc_lock);
- return error;
- }
- }
-
- /* Mark this file locked */
- file->lock = 1;
- mutex_exit(sc->sc_lock);
-
- return 0;
-}
-
-/*
- * Release the file lock.
- */
-static void
-audio_file_release(struct audio_softc *sc, audio_file_t *file)
-{
-
- KASSERT(!mutex_owned(sc->sc_lock));
-
- mutex_enter(sc->sc_lock);
- KASSERT(file->lock);
- file->lock = 0;
- cv_broadcast(&sc->sc_exlockcv);
- mutex_exit(sc->sc_lock);
-}
-
-/*
* Try to acquire track lock.
* It doesn't block if the track lock is already aquired.
* Returns true if the track lock was acquired, or false if the track
@@ -1563,11 +1503,7 @@ audioclose(struct file *fp)
sc = file->sc;
dev = file->dev;
- /* Acquire file lock and exlock */
- /* XXX what should I do when an error occurs? */
- error = audio_file_acquire(sc, file);
- if (error)
- return error;
+ /* audio_{enter,exit}_exclusive() is called by lower audio_close() */
device_active(sc->sc_dev, DVA_SYSTEM);
switch (AUDIODEV(dev)) {
@@ -1590,11 +1526,6 @@ audioclose(struct file *fp)
fp->f_audioctx = NULL;
}
- /*
- * Since file has already been destructed,
- * audio_file_release() is not necessary.
- */
-
return error;
}
@@ -1612,10 +1543,6 @@ audioread(struct file *fp, off_t *offp,
sc = file->sc;
dev = file->dev;
- error = audio_file_acquire(sc, file);
- if (error)
- return error;
-
if (fp->f_flag & O_NONBLOCK)
ioflag |= IO_NDELAY;
@@ -1632,7 +1559,6 @@ audioread(struct file *fp, off_t *offp,
error = ENXIO;
break;
}
- audio_file_release(sc, file);
return error;
}
@@ -1651,10 +1577,6 @@ audiowrite(struct file *fp, off_t *offp,
sc = file->sc;
dev = file->dev;
- error = audio_file_acquire(sc, file);
- if (error)
- return error;
-
if (fp->f_flag & O_NONBLOCK)
ioflag |= IO_NDELAY;
@@ -1671,7 +1593,6 @@ audiowrite(struct file *fp, off_t *offp,
error = ENXIO;
break;
}
- audio_file_release(sc, file);
return error;
}
@@ -1690,10 +1611,6 @@ audioioctl(struct file *fp, u_long cmd,
sc = file->sc;
dev = file->dev;
- error = audio_file_acquire(sc, file);
- if (error)
- return error;
-
switch (AUDIODEV(dev)) {
case SOUND_DEVICE:
case AUDIO_DEVICE:
@@ -1714,7 +1631,6 @@ audioioctl(struct file *fp, u_long cmd,
error = ENXIO;
break;
}
- audio_file_release(sc, file);
return error;
}
@@ -1750,9 +1666,6 @@ audiopoll(struct file *fp, int events)
sc = file->sc;
dev = file->dev;
- if (audio_file_acquire(sc, file) != 0)
- return 0;
-
switch (AUDIODEV(dev)) {
case SOUND_DEVICE:
case AUDIO_DEVICE:
@@ -1766,7 +1679,6 @@ audiopoll(struct file *fp, int events)
revents = POLLERR;
break;
}
- audio_file_release(sc, file);
return revents;
}
@@ -1784,10 +1696,6 @@ audiokqfilter(struct file *fp, struct kn
sc = file->sc;
dev = file->dev;
- error = audio_file_acquire(sc, file);
- if (error)
- return error;
-
switch (AUDIODEV(dev)) {
case SOUND_DEVICE:
case AUDIO_DEVICE:
@@ -1801,7 +1709,6 @@ audiokqfilter(struct file *fp, struct kn
error = ENXIO;
break;
}
- audio_file_release(sc, file);
return error;
}
@@ -1820,10 +1727,6 @@ audiommap(struct file *fp, off_t *offp,
sc = file->sc;
dev = file->dev;
- error = audio_file_acquire(sc, file);
- if (error)
- return error;
-
mutex_enter(sc->sc_lock);
device_active(sc->sc_dev, DVA_SYSTEM); /* XXXJDM */
mutex_exit(sc->sc_lock);
@@ -1840,7 +1743,6 @@ audiommap(struct file *fp, off_t *offp,
error = ENOTSUP;
break;
}
- audio_file_release(sc, file);
return error;
}
@@ -1887,11 +1789,6 @@ audiobellclose(audio_file_t *file)
sc = file->sc;
- /* XXX what should I do when an error occurs? */
- error = audio_file_acquire(sc, file);
- if (error)
- return error;
-
device_active(sc->sc_dev, DVA_SYSTEM);
error = audio_close(sc, file);
@@ -1911,13 +1808,7 @@ audiobellwrite(audio_file_t *file, struc
int error;
sc = file->sc;
- error = audio_file_acquire(sc, file);
- if (error)
- return error;
-
error = audio_write(sc, uio, 0, file);
-
- audio_file_release(sc, file);
return error;
}
@@ -2179,6 +2070,9 @@ bad1:
return error;
}
+/*
+ * Must NOT called with sc_lock nor sc_exlock held.
+ */
int
audio_close(struct audio_softc *sc, audio_file_t *file)
{
@@ -2186,7 +2080,6 @@ audio_close(struct audio_softc *sc, audi
int error;
KASSERT(!mutex_owned(sc->sc_lock));
- KASSERT(file->lock);
TRACEF(1, file, "%spid=%d.%d po=%d ro=%d",
(audiodebug >= 3) ? "start " : "",
@@ -2209,10 +2102,8 @@ audio_close(struct audio_softc *sc, audi
/* Then, acquire exclusive lock to protect counters. */
/* XXX what should I do when an error occurs? */
error = audio_enter_exclusive(sc);
- if (error) {
- audio_file_release(sc, file);
+ if (error)
return error;
- }
if (file->ptrack) {
/* Call hw halt_output if this is the last playback track. */
@@ -2294,7 +2185,6 @@ audio_read(struct audio_softc *sc, struc
TRACET(2, track, "resid=%zd", uio->uio_resid);
KASSERT(!mutex_owned(sc->sc_lock));
- KASSERT(file->lock);
/* I think it's better than EINVAL. */
if (track->mmapped)
@@ -2367,7 +2257,6 @@ audio_read(struct audio_softc *sc, struc
audio_track_lock_enter(track);
audio_track_record(track);
- audio_track_lock_exit(track);
/* uiomove from usrbuf as much as possible. */
bytes = uimin(usrbuf->used, uio->uio_resid);
@@ -2377,6 +2266,7 @@ audio_read(struct audio_softc *sc, struc
error = uiomove((uint8_t *)usrbuf->mem + head, len,
uio);
if (error) {
+ audio_track_lock_exit(track);
device_printf(sc->sc_dev,
"uiomove(len=%d) failed with %d\n",
len, error);
@@ -2389,6 +2279,8 @@ audio_read(struct audio_softc *sc, struc
usrbuf->head, usrbuf->used, usrbuf->capacity);
bytes -= len;
}
+
+ audio_track_lock_exit(track);
}
abort:
@@ -2425,7 +2317,6 @@ audio_write(struct audio_softc *sc, stru
uio->uio_resid, (int)curproc->p_pid, (int)curlwp->l_lid, ioflag);
KASSERT(!mutex_owned(sc->sc_lock));
- KASSERT(file->lock);
/* I think it's better than EINVAL. */
if (track->mmapped)
@@ -2492,6 +2383,8 @@ audio_write(struct audio_softc *sc, stru
}
mutex_exit(sc->sc_lock);
+ audio_track_lock_enter(track);
+
/* uiomove to usrbuf as much as possible. */
bytes = uimin(track->usrbuf_usedhigh - usrbuf->used,
uio->uio_resid);
@@ -2501,6 +2394,7 @@ audio_write(struct audio_softc *sc, stru
error = uiomove((uint8_t *)usrbuf->mem + tail, len,
uio);
if (error) {
+ audio_track_lock_exit(track);
device_printf(sc->sc_dev,
"uiomove(len=%d) failed with %d\n",
len, error);
@@ -2515,11 +2409,11 @@ audio_write(struct audio_softc *sc, stru
}
/* Convert them as much as possible. */
- audio_track_lock_enter(track);
while (usrbuf->used >= track->usrbuf_blksize &&
outbuf->used < outbuf->capacity) {
audio_track_play(track);
}
+
audio_track_lock_exit(track);
}
@@ -2544,7 +2438,6 @@ audio_ioctl(dev_t dev, struct audio_soft
int error;
KASSERT(!mutex_owned(sc->sc_lock));
- KASSERT(file->lock);
#if defined(AUDIO_DEBUG)
const char *ioctlnames[] = {
@@ -2849,7 +2742,6 @@ audio_poll(struct audio_softc *sc, int e
bool out_is_valid;
KASSERT(!mutex_owned(sc->sc_lock));
- KASSERT(file->lock);
#if defined(AUDIO_DEBUG)
#define POLLEV_BITMAP "\177\020" \
@@ -3004,7 +2896,6 @@ audio_kqfilter(struct audio_softc *sc, a
struct klist *klist;
KASSERT(!mutex_owned(sc->sc_lock));
- KASSERT(file->lock);
TRACEF(3, file, "kn=%p kn_filter=%x", kn, (int)kn->kn_filter);
@@ -3042,7 +2933,6 @@ audio_mmap(struct audio_softc *sc, off_t
int error;
KASSERT(!mutex_owned(sc->sc_lock));
- KASSERT(file->lock);
TRACEF(2, file, "off=%lld, prot=%d", (long long)(*offp), prot);
Index: src/sys/dev/audio/audiodef.h
diff -u src/sys/dev/audio/audiodef.h:1.2 src/sys/dev/audio/audiodef.h:1.3
--- src/sys/dev/audio/audiodef.h:1.2 Wed May 8 13:40:17 2019
+++ src/sys/dev/audio/audiodef.h Thu May 23 12:20:27 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: audiodef.h,v 1.2 2019/05/08 13:40:17 isaki Exp $ */
+/* $NetBSD: audiodef.h,v 1.3 2019/05/23 12:20:27 isaki Exp $ */
/*
* Copyright (C) 2017 Tetsuya Isaki. All rights reserved.
@@ -182,13 +182,6 @@ struct audio_file {
/* process who wants audio SIGIO. */
pid_t async_audio;
- /*
- * Non-zero if some thread context is using this file structure
- * (including ptrack and rtrack) now.
- * Must be protected by sc_lock.
- */
- volatile int lock;
-
SLIST_ENTRY(audio_file) entry;
};
