AudioFlinger: add specific mutex for client lists
Add a specific mutex to protect access to mClients and
mNotificationClients lists. This avoids locking the main AudioFlinger
mutex from inside thread loops and allows not to worry about
cross deadlocks when sending a config event with status reply while
keeping the ThreadBase or AudioFlinger mutex locked.
As a way of consequence, remove notification client list passed to
processConfigEvents_l() and audioConfigChanged() as the list
can now be accessed by locking client mutex only.
Change-Id: I228022204b6709a8bb60cc96d9514a6ffe59b62e
diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp
index e256f32..11170c2 100644
--- a/services/audioflinger/AudioFlinger.cpp
+++ b/services/audioflinger/AudioFlinger.cpp
@@ -82,6 +82,7 @@
static const char kDeadlockedString[] = "AudioFlinger may be deadlocked\n";
static const char kHardwareLockedString[] = "Hardware lock is taken\n";
+static const char kClientLockedString[] = "Client lock is taken\n";
nsecs_t AudioFlinger::mStandbyTimeInNsecs = kDefaultStandbyTimeInNsecs;
@@ -382,7 +383,16 @@
write(fd, result.string(), result.size());
}
+ bool clientLocked = dumpTryLock(mClientLock);
+ if (!clientLocked) {
+ String8 result(kClientLockedString);
+ write(fd, result.string(), result.size());
+ }
dumpClients(fd, args);
+ if (clientLocked) {
+ mClientLock.unlock();
+ }
+
dumpInternals(fd, args);
// dump playback threads
@@ -426,8 +436,9 @@
return NO_ERROR;
}
-sp<AudioFlinger::Client> AudioFlinger::registerPid_l(pid_t pid)
+sp<AudioFlinger::Client> AudioFlinger::registerPid(pid_t pid)
{
+ Mutex::Autolock _cl(mClientLock);
// If pid is already in the mClients wp<> map, then use that entry
// (for which promote() is always != 0), otherwise create a new entry and Client.
sp<Client> client = mClients.valueFor(pid).promote();
@@ -564,7 +575,7 @@
}
pid_t pid = IPCThreadState::self()->getCallingPid();
- client = registerPid_l(pid);
+ client = registerPid(pid);
PlaybackThread *effectThread = NULL;
if (sessionId != NULL && *sessionId != AUDIO_SESSION_ALLOCATE) {
@@ -623,7 +634,8 @@
if (lStatus != NO_ERROR) {
// remove local strong reference to Client before deleting the Track so that the
- // Client destructor is called by the TrackBase destructor with mLock held
+ // Client destructor is called by the TrackBase destructor with mClientLock held
+ Mutex::Autolock _cl(mClientLock);
client.clear();
track.clear();
goto Exit;
@@ -1140,21 +1152,29 @@
void AudioFlinger::registerClient(const sp<IAudioFlingerClient>& client)
{
-
Mutex::Autolock _l(mLock);
+ bool clientAdded = false;
+ {
+ Mutex::Autolock _cl(mClientLock);
- pid_t pid = IPCThreadState::self()->getCallingPid();
- if (mNotificationClients.indexOfKey(pid) < 0) {
- sp<NotificationClient> notificationClient = new NotificationClient(this,
- client,
- pid);
- ALOGV("registerClient() client %p, pid %d", notificationClient.get(), pid);
+ pid_t pid = IPCThreadState::self()->getCallingPid();
+ if (mNotificationClients.indexOfKey(pid) < 0) {
+ sp<NotificationClient> notificationClient = new NotificationClient(this,
+ client,
+ pid);
+ ALOGV("registerClient() client %p, pid %d", notificationClient.get(), pid);
- mNotificationClients.add(pid, notificationClient);
+ mNotificationClients.add(pid, notificationClient);
- sp<IBinder> binder = client->asBinder();
- binder->linkToDeath(notificationClient);
+ sp<IBinder> binder = client->asBinder();
+ binder->linkToDeath(notificationClient);
+ clientAdded = true;
+ }
+ }
+ // mClientLock should not be held here because ThreadBase::sendIoConfigEvent() will lock the
+ // ThreadBase mutex and teh locknig order is ThreadBase::mLock then AudioFlinger::mClientLock.
+ if (clientAdded) {
// the config change is always sent from playback or record threads to avoid deadlock
// with AudioSystem::gLock
for (size_t i = 0; i < mPlaybackThreads.size(); i++) {
@@ -1170,8 +1190,10 @@
void AudioFlinger::removeNotificationClient(pid_t pid)
{
Mutex::Autolock _l(mLock);
-
- mNotificationClients.removeItem(pid);
+ {
+ Mutex::Autolock _cl(mClientLock);
+ mNotificationClients.removeItem(pid);
+ }
ALOGV("%d died, releasing its sessions", pid);
size_t num = mAudioSessionRefs.size();
@@ -1194,22 +1216,18 @@
}
}
-// audioConfigChanged_l() must be called with AudioFlinger::mLock held
-void AudioFlinger::audioConfigChanged_l(
- const DefaultKeyedVector< pid_t,sp<NotificationClient> >& notificationClients,
- int event,
- audio_io_handle_t ioHandle,
- const void *param2)
+void AudioFlinger::audioConfigChanged(int event, audio_io_handle_t ioHandle, const void *param2)
{
- size_t size = notificationClients.size();
+ Mutex::Autolock _l(mClientLock);
+ size_t size = mNotificationClients.size();
for (size_t i = 0; i < size; i++) {
- notificationClients.valueAt(i)->audioFlingerClient()->ioConfigChanged(event,
+ mNotificationClients.valueAt(i)->audioFlingerClient()->ioConfigChanged(event,
ioHandle,
param2);
}
}
-// removeClient_l() must be called with AudioFlinger::mLock held
+// removeClient_l() must be called with AudioFlinger::mClientLock held
void AudioFlinger::removeClient_l(pid_t pid)
{
ALOGV("removeClient_l() pid %d, calling pid %d", pid,
@@ -1247,7 +1265,7 @@
// 1 MB of address space is good for 32 tracks, 8 buffers each, 4 KB/buffer
}
-// Client destructor must be called with AudioFlinger::mLock held
+// Client destructor must be called with AudioFlinger::mClientLock held
AudioFlinger::Client::~Client()
{
mAudioFlinger->removeClient_l(mPid);
@@ -1377,7 +1395,7 @@
}
pid_t pid = IPCThreadState::self()->getCallingPid();
- client = registerPid_l(pid);
+ client = registerPid(pid);
if (sessionId != NULL && *sessionId != AUDIO_SESSION_ALLOCATE) {
lSessionId = *sessionId;
@@ -1400,7 +1418,8 @@
if (lStatus != NO_ERROR) {
// remove local strong reference to Client before deleting the RecordTrack so that the
- // Client destructor is called by the TrackBase destructor with mLock held
+ // Client destructor is called by the TrackBase destructor with mClientLock held
+ Mutex::Autolock _cl(mClientLock);
client.clear();
recordTrack.clear();
goto Exit;
@@ -1638,7 +1657,7 @@
}
// notify client processes of the new output creation
- thread->audioConfigChanged_l(mNotificationClients, AudioSystem::OUTPUT_OPENED);
+ thread->audioConfigChanged(AudioSystem::OUTPUT_OPENED);
// the first primary output opened designates the primary hw device
if ((mPrimaryHardwareDev == NULL) && (flags & AUDIO_OUTPUT_FLAG_PRIMARY)) {
@@ -1674,7 +1693,7 @@
thread->addOutputTrack(thread2);
mPlaybackThreads.add(id, thread);
// notify client processes of the new output creation
- thread->audioConfigChanged_l(mNotificationClients, AudioSystem::OUTPUT_OPENED);
+ thread->audioConfigChanged(AudioSystem::OUTPUT_OPENED);
return id;
}
@@ -1724,7 +1743,7 @@
}
}
}
- audioConfigChanged_l(mNotificationClients, AudioSystem::OUTPUT_CLOSED, output, NULL);
+ audioConfigChanged(AudioSystem::OUTPUT_CLOSED, output, NULL);
}
thread->exit();
// The thread entity (active unit of execution) is no longer running here,
@@ -1904,7 +1923,7 @@
}
// notify client processes of the new input creation
- thread->audioConfigChanged_l(mNotificationClients, AudioSystem::INPUT_OPENED);
+ thread->audioConfigChanged(AudioSystem::INPUT_OPENED);
return id;
}
@@ -1929,7 +1948,7 @@
}
ALOGV("closeInput() %d", input);
- audioConfigChanged_l(mNotificationClients, AudioSystem::INPUT_CLOSED, input, NULL);
+ audioConfigChanged(AudioSystem::INPUT_CLOSED, input, NULL);
mRecordThreads.removeItem(input);
}
thread->exit();
@@ -1973,13 +1992,16 @@
caller = pid;
}
- // Ignore requests received from processes not known as notification client. The request
- // is likely proxied by mediaserver (e.g CameraService) and releaseAudioSessionId() can be
- // called from a different pid leaving a stale session reference. Also we don't know how
- // to clear this reference if the client process dies.
- if (mNotificationClients.indexOfKey(caller) < 0) {
- ALOGW("acquireAudioSessionId() unknown client %d for session %d", caller, audioSession);
- return;
+ {
+ Mutex::Autolock _cl(mClientLock);
+ // Ignore requests received from processes not known as notification client. The request
+ // is likely proxied by mediaserver (e.g CameraService) and releaseAudioSessionId() can be
+ // called from a different pid leaving a stale session reference. Also we don't know how
+ // to clear this reference if the client process dies.
+ if (mNotificationClients.indexOfKey(caller) < 0) {
+ ALOGW("acquireAudioSessionId() unknown client %d for session %d", caller, audioSession);
+ return;
+ }
}
size_t num = mAudioSessionRefs.size();
@@ -2348,7 +2370,7 @@
}
}
- sp<Client> client = registerPid_l(pid);
+ sp<Client> client = registerPid(pid);
// create effect on selected output thread
handle = thread->createEffect_l(client, effectClient, priority, sessionId,
diff --git a/services/audioflinger/AudioFlinger.h b/services/audioflinger/AudioFlinger.h
index 894bd35..c1d4c08 100644
--- a/services/audioflinger/AudioFlinger.h
+++ b/services/audioflinger/AudioFlinger.h
@@ -453,11 +453,7 @@
// no range check, doesn't check per-thread stream volume, AudioFlinger::mLock held
float streamVolume_l(audio_stream_type_t stream) const
{ return mStreamTypes[stream].volume; }
- void audioConfigChanged_l(const DefaultKeyedVector< pid_t,sp<NotificationClient> >&
- notificationClients,
- int event,
- audio_io_handle_t ioHandle,
- const void *param2);
+ void audioConfigChanged(int event, audio_io_handle_t ioHandle, const void *param2);
// Allocate an audio_io_handle_t, session ID, effect ID, or audio_module_handle_t.
// They all share the same ID space, but the namespaces are actually independent
@@ -482,8 +478,6 @@
void removeClient_l(pid_t pid);
void removeNotificationClient(pid_t pid);
- DefaultKeyedVector< pid_t,sp<NotificationClient> > notificationClients() {
- Mutex::Autolock _l(mLock); return mNotificationClients; }
bool isNonOffloadableGlobalEffectEnabled_l();
void onNonOffloadableGlobalEffectEnable();
@@ -553,7 +547,11 @@
};
mutable Mutex mLock;
-
+ // protects mClients and mNotificationClients.
+ // must be locked after mLock and ThreadBase::mLock if both must be locked
+ // avoids acquiring AudioFlinger::mLock from inside thread loop.
+ mutable Mutex mClientLock;
+ // protected by mClientLock
DefaultKeyedVector< pid_t, wp<Client> > mClients; // see ~Client()
mutable Mutex mHardwareLock;
@@ -602,6 +600,7 @@
DefaultKeyedVector< audio_io_handle_t, sp<RecordThread> > mRecordThreads;
+ // protected by mClientLock
DefaultKeyedVector< pid_t, sp<NotificationClient> > mNotificationClients;
volatile int32_t mNextUniqueId; // updated by android_atomic_inc
@@ -622,7 +621,7 @@
// to be created
private:
- sp<Client> registerPid_l(pid_t pid); // always returns non-0
+ sp<Client> registerPid(pid_t pid); // always returns non-0
// for use from destructor
status_t closeOutput_nonvirtual(audio_io_handle_t output);
diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp
index 29b56db..77aca00 100644
--- a/services/audioflinger/Effects.cpp
+++ b/services/audioflinger/Effects.cpp
@@ -1162,8 +1162,8 @@
mCblk->~effect_param_cblk_t(); // destroy our shared-structure.
}
mCblkMemory.clear(); // free the shared memory before releasing the heap it belongs to
- // Client destructor must run with AudioFlinger mutex locked
- Mutex::Autolock _l(mClient->audioFlinger()->mLock);
+ // Client destructor must run with AudioFlinger client mutex locked
+ Mutex::Autolock _l(mClient->audioFlinger()->mClientLock);
mClient.clear();
}
}
diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp
index 470b018..8243a8b 100644
--- a/services/audioflinger/Threads.cpp
+++ b/services/audioflinger/Threads.cpp
@@ -401,8 +401,7 @@
}
// post condition: mConfigEvents.isEmpty()
-void AudioFlinger::ThreadBase::processConfigEvents_l(
- const DefaultKeyedVector< pid_t,sp<NotificationClient> >& notificationClients)
+void AudioFlinger::ThreadBase::processConfigEvents_l()
{
bool configChanged = false;
@@ -423,7 +422,7 @@
} break;
case CFG_EVENT_IO: {
IoConfigEventData *data = (IoConfigEventData *)event->mData.get();
- audioConfigChanged_l(notificationClients, data->mEvent, data->mParam);
+ audioConfigChanged(data->mEvent, data->mParam);
} break;
case CFG_EVENT_SET_PARAMETER: {
SetParameterConfigEventData *data = (SetParameterConfigEventData *)event->mData.get();
@@ -1638,15 +1637,11 @@
return out_s8;
}
-// audioConfigChanged_l() must be called with AudioFlinger::mLock held
-void AudioFlinger::PlaybackThread::audioConfigChanged_l(
- const DefaultKeyedVector< pid_t,sp<NotificationClient> >& notificationClients,
- int event,
- int param) {
+void AudioFlinger::PlaybackThread::audioConfigChanged(int event, int param) {
AudioSystem::OutputDescriptor desc;
void *param2 = NULL;
- ALOGV("PlaybackThread::audioConfigChanged_l, thread %p, event %d, param %d", this, event,
+ ALOGV("PlaybackThread::audioConfigChanged, thread %p, event %d, param %d", this, event,
param);
switch (event) {
@@ -1667,7 +1662,7 @@
default:
break;
}
- mAudioFlinger->audioConfigChanged_l(notificationClients, event, mId, param2);
+ mAudioFlinger->audioConfigChanged(event, mId, param2);
}
void AudioFlinger::PlaybackThread::writeCallback()
@@ -2317,15 +2312,11 @@
Vector< sp<EffectChain> > effectChains;
- DefaultKeyedVector< pid_t,sp<NotificationClient> > notificationClients =
- mAudioFlinger->notificationClients();
-
{ // scope for mLock
Mutex::Autolock _l(mLock);
- processConfigEvents_l(notificationClients);
- notificationClients.clear();
+ processConfigEvents_l();
if (logString != NULL) {
mNBLogWriter->logTimestamp();
@@ -4695,14 +4686,11 @@
// activeTracks accumulates a copy of a subset of mActiveTracks
Vector< sp<RecordTrack> > activeTracks;
- DefaultKeyedVector< pid_t,sp<NotificationClient> > notificationClients =
- mAudioFlinger->notificationClients();
{ // scope for mLock
Mutex::Autolock _l(mLock);
- processConfigEvents_l(notificationClients);
- notificationClients.clear();
+ processConfigEvents_l();
// check exitPending here because checkForNewParameters_l() and
// checkForNewParameters_l() can temporarily release mLock
@@ -5605,10 +5593,7 @@
return out_s8;
}
-void AudioFlinger::RecordThread::audioConfigChanged_l(
- const DefaultKeyedVector< pid_t,sp<NotificationClient> >& notificationClients,
- int event,
- int param __unused) {
+void AudioFlinger::RecordThread::audioConfigChanged(int event, int param __unused) {
AudioSystem::OutputDescriptor desc;
const void *param2 = NULL;
@@ -5627,7 +5612,7 @@
default:
break;
}
- mAudioFlinger->audioConfigChanged_l(notificationClients, event, mId, param2);
+ mAudioFlinger->audioConfigChanged(event, mId, param2);
}
void AudioFlinger::RecordThread::readInputParameters_l()
diff --git a/services/audioflinger/Threads.h b/services/audioflinger/Threads.h
index 9578993..cc2b246 100644
--- a/services/audioflinger/Threads.h
+++ b/services/audioflinger/Threads.h
@@ -200,10 +200,7 @@
status_t& status) = 0;
virtual status_t setParameters(const String8& keyValuePairs);
virtual String8 getParameters(const String8& keys) = 0;
- virtual void audioConfigChanged_l(
- const DefaultKeyedVector< pid_t,sp<NotificationClient> >& notificationClients,
- int event,
- int param = 0) = 0;
+ virtual void audioConfigChanged(int event, int param = 0) = 0;
// sendConfigEvent_l() must be called with ThreadBase::mLock held
// Can temporarily release the lock if waiting for a reply from
// processConfigEvents_l().
@@ -212,8 +209,7 @@
void sendIoConfigEvent_l(int event, int param = 0);
void sendPrioConfigEvent_l(pid_t pid, pid_t tid, int32_t prio);
status_t sendSetParameterConfigEvent_l(const String8& keyValuePair);
- void processConfigEvents_l(
- const DefaultKeyedVector< pid_t,sp<NotificationClient> >& notificationClients);
+ void processConfigEvents_l();
virtual void cacheParameters_l() = 0;
// see note at declaration of mStandby, mOutDevice and mInDevice
@@ -502,10 +498,7 @@
{ return android_atomic_acquire_load(&mSuspended) > 0; }
virtual String8 getParameters(const String8& keys);
- virtual void audioConfigChanged_l(
- const DefaultKeyedVector< pid_t,sp<NotificationClient> >& notificationClients,
- int event,
- int param = 0);
+ virtual void audioConfigChanged(int event, int param = 0);
status_t getRenderPosition(uint32_t *halFrames, uint32_t *dspFrames);
// FIXME rename mixBuffer() to sinkBuffer() and remove int16_t* dependency.
// Consider also removing and passing an explicit mMainBuffer initialization
@@ -652,7 +645,6 @@
friend class AudioFlinger; // for numerous
- PlaybackThread(const Client&);
PlaybackThread& operator = (const PlaybackThread&);
status_t addTrack_l(const sp<Track>& track);
@@ -1037,10 +1029,7 @@
status_t& status);
virtual void cacheParameters_l() {}
virtual String8 getParameters(const String8& keys);
- virtual void audioConfigChanged_l(
- const DefaultKeyedVector< pid_t,sp<NotificationClient> >& notificationClients,
- int event,
- int param = 0);
+ virtual void audioConfigChanged(int event, int param = 0);
void readInputParameters_l();
virtual uint32_t getInputFramesLost();
diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp
index d8f3423..88ead74 100644
--- a/services/audioflinger/Tracks.cpp
+++ b/services/audioflinger/Tracks.cpp
@@ -198,8 +198,8 @@
}
mCblkMemory.clear(); // free the shared memory before releasing the heap it belongs to
if (mClient != 0) {
- // Client destructor must run with AudioFlinger mutex locked
- Mutex::Autolock _l(mClient->audioFlinger()->mLock);
+ // Client destructor must run with AudioFlinger client mutex locked
+ Mutex::Autolock _l(mClient->audioFlinger()->mClientLock);
// If the client's reference count drops to zero, the associated destructor
// must run with AudioFlinger lock held. Thus the explicit clear() rather than
// relying on the automatic clear() at end of scope.