summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authormukesh agrawal <quiche@google.com>2017-05-09 17:43:21 -0700
committermukesh agrawal <quiche@google.com>2017-06-07 19:11:36 -0700
commit5e6aea460e272ef7c70029abe9f0e5a695ad119e (patch)
treec2333f529cdc96d08345f12ea098d6917f239821
parent9da08e01464d39de3f14fe09f535660635d39c42 (diff)
downloadandroid_frameworks_opt_net_wifi-5e6aea460e272ef7c70029abe9f0e5a695ad119e.tar.gz
android_frameworks_opt_net_wifi-5e6aea460e272ef7c70029abe9f0e5a695ad119e.tar.bz2
android_frameworks_opt_net_wifi-5e6aea460e272ef7c70029abe9f0e5a695ad119e.zip
WifiVendorHal: improve handling of debug events
Move processing of debug events (onRingBufferDataAvailable and onDebugErrorAlert) from a HWBinder thread, to the WifiStateMachine thread. The idea here is to quickly free up shared resources: the thread on which we receive the HIDL event, and the HWBinder async buffer space that is used to queue pending upcalls. The reason we want to free up these resources more quickly is that, in some cases, the debug events need to wait 100-200msec to acquire the WifiDiagnostics object's intrinsic lock. A risk of this change is that we might use a large amount of memory by queuing up large amounts of ring-buffer data callbacks on the Looper's MessageQueue. However, I think that is a better risk to take, than the risk of starving other HALs of access to HWBinder resources. Bug: 38182372 Test: tests/wifitests/runtests.sh # on bullhead Test: adb shell dumpsys wifi | grep TIMEOUT # expect no match Change-Id: I7ec24f8b862cfb8ac100b1c975d732a886ed09fe
-rw-r--r--service/java/com/android/server/wifi/WifiVendorHal.java60
-rw-r--r--tests/wifitests/src/com/android/server/wifi/WifiVendorHalTest.java14
2 files changed, 57 insertions, 17 deletions
diff --git a/service/java/com/android/server/wifi/WifiVendorHal.java b/service/java/com/android/server/wifi/WifiVendorHal.java
index 9c1ae94b5..88f189814 100644
--- a/service/java/com/android/server/wifi/WifiVendorHal.java
+++ b/service/java/com/android/server/wifi/WifiVendorHal.java
@@ -64,6 +64,7 @@ import android.net.wifi.WifiManager;
import android.net.wifi.WifiScanner;
import android.net.wifi.WifiSsid;
import android.net.wifi.WifiWakeReasonAndCounts;
+import android.os.Handler;
import android.os.Looper;
import android.os.RemoteException;
import android.util.MutableBoolean;
@@ -204,16 +205,23 @@ public class WifiVendorHal {
private IWifiRttController mIWifiRttController;
private final HalDeviceManager mHalDeviceManager;
private final HalDeviceManagerStatusListener mHalDeviceManagerStatusCallbacks;
- private final Looper mLooper;
private final IWifiStaIfaceEventCallback mIWifiStaIfaceEventCallback;
private final IWifiChipEventCallback mIWifiChipEventCallback;
private final RttEventCallback mRttEventCallback;
+ // Plumbing for event handling.
+ //
+ // Being final fields, they can be accessed without synchronization under
+ // some reasonable assumptions. See
+ // https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5
+ private final Looper mLooper;
+ private final Handler mHalEventHandler;
public WifiVendorHal(HalDeviceManager halDeviceManager,
Looper looper) {
mHalDeviceManager = halDeviceManager;
mLooper = looper;
+ mHalEventHandler = new Handler(looper);
mHalDeviceManagerStatusCallbacks = new HalDeviceManagerStatusListener();
mIWifiStaIfaceEventCallback = new StaIfaceEventCallback();
mIWifiChipEventCallback = new ChipEventCallback();
@@ -2455,25 +2463,47 @@ public class WifiVendorHal {
WifiDebugRingBufferStatus status, java.util.ArrayList<Byte> data) {
//TODO(b/35875078) Reinstate logging when execessive callbacks are fixed
// mVerboseLog.d("onDebugRingBufferDataAvailable " + status);
- WifiNative.WifiLoggerEventHandler eventHandler;
- synchronized (sLock) {
- if (mLogEventHandler == null || status == null || data == null) return;
- eventHandler = mLogEventHandler;
- }
- eventHandler.onRingBufferData(
- ringBufferStatus(status), NativeUtil.byteArrayFromArrayList(data));
+ mHalEventHandler.post(() -> {
+ WifiNative.WifiLoggerEventHandler eventHandler;
+ synchronized (sLock) {
+ if (mLogEventHandler == null || status == null || data == null) return;
+ eventHandler = mLogEventHandler;
+ }
+ // Because |sLock| has been released, there is a chance that we'll execute
+ // a spurious callback (after someone has called resetLogHandler()).
+ //
+ // However, the alternative risks deadlock. Consider:
+ // [T1.1] WifiDiagnostics.captureBugReport()
+ // [T1.2] -- acquire WifiDiagnostics object's intrinsic lock
+ // [T1.3] -> WifiVendorHal.getRingBufferData()
+ // [T1.4] -- acquire WifiVendorHal.sLock
+ // [T2.1] <lambda>()
+ // [T2.2] -- acquire WifiVendorHal.sLock
+ // [T2.3] -> WifiDiagnostics.onRingBufferData()
+ // [T2.4] -- acquire WifiDiagnostics object's intrinsic lock
+ //
+ // The problem here is that the two threads acquire the locks in opposite order.
+ // If, for example, T2.2 executes between T1.2 and 1.4, then T1 and T2
+ // will be deadlocked.
+ eventHandler.onRingBufferData(
+ ringBufferStatus(status), NativeUtil.byteArrayFromArrayList(data));
+ });
}
@Override
public void onDebugErrorAlert(int errorCode, java.util.ArrayList<Byte> debugData) {
mVerboseLog.d("onDebugErrorAlert " + errorCode);
- WifiNative.WifiLoggerEventHandler eventHandler;
- synchronized (sLock) {
- if (mLogEventHandler == null || debugData == null) return;
- eventHandler = mLogEventHandler;
- }
- eventHandler.onWifiAlert(
- errorCode, NativeUtil.byteArrayFromArrayList(debugData));
+ mHalEventHandler.post(() -> {
+ WifiNative.WifiLoggerEventHandler eventHandler;
+ synchronized (sLock) {
+ if (mLogEventHandler == null || debugData == null) return;
+ eventHandler = mLogEventHandler;
+ }
+ // See comment in onDebugRingBufferDataAvailable(), for an explanation
+ // of why this callback is invoked without |sLock| held.
+ eventHandler.onWifiAlert(
+ errorCode, NativeUtil.byteArrayFromArrayList(debugData));
+ });
}
}
diff --git a/tests/wifitests/src/com/android/server/wifi/WifiVendorHalTest.java b/tests/wifitests/src/com/android/server/wifi/WifiVendorHalTest.java
index 66a55b51a..34ddf2378 100644
--- a/tests/wifitests/src/com/android/server/wifi/WifiVendorHalTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/WifiVendorHalTest.java
@@ -1679,9 +1679,10 @@ public class WifiVendorHalTest {
new Random().nextBytes(errorData);
// Randomly raise the HIDL callback before we register for the log callback.
- // This should be ignored.
+ // This should be safely ignored. (Not trigger NPE.)
mIWifiChipEventCallback.onDebugErrorAlert(
errorCode, NativeUtil.byteArrayToArrayList(errorData));
+ mLooper.dispatchAll();
WifiNative.WifiLoggerEventHandler eventHandler =
mock(WifiNative.WifiLoggerEventHandler.class);
@@ -1691,12 +1692,16 @@ public class WifiVendorHalTest {
// Now raise the HIDL callback, this should be properly handled.
mIWifiChipEventCallback.onDebugErrorAlert(
errorCode, NativeUtil.byteArrayToArrayList(errorData));
+ mLooper.dispatchAll();
verify(eventHandler).onWifiAlert(eq(errorCode), eq(errorData));
// Now stop the logging and invoke the callback. This should be ignored.
+ reset(eventHandler);
assertTrue(mWifiVendorHal.resetLogHandler());
mIWifiChipEventCallback.onDebugErrorAlert(
errorCode, NativeUtil.byteArrayToArrayList(errorData));
+ mLooper.dispatchAll();
+ verify(eventHandler, never()).onWifiAlert(anyInt(), anyObject());
}
/**
@@ -1714,9 +1719,10 @@ public class WifiVendorHalTest {
new Random().nextBytes(errorData);
// Randomly raise the HIDL callback before we register for the log callback.
- // This should be ignored.
+ // This should be safely ignored. (Not trigger NPE.)
mIWifiChipEventCallback.onDebugRingBufferDataAvailable(
new WifiDebugRingBufferStatus(), NativeUtil.byteArrayToArrayList(errorData));
+ mLooper.dispatchAll();
WifiNative.WifiLoggerEventHandler eventHandler =
mock(WifiNative.WifiLoggerEventHandler.class);
@@ -1726,13 +1732,17 @@ public class WifiVendorHalTest {
// Now raise the HIDL callback, this should be properly handled.
mIWifiChipEventCallback.onDebugRingBufferDataAvailable(
new WifiDebugRingBufferStatus(), NativeUtil.byteArrayToArrayList(errorData));
+ mLooper.dispatchAll();
verify(eventHandler).onRingBufferData(
any(WifiNative.RingBufferStatus.class), eq(errorData));
// Now stop the logging and invoke the callback. This should be ignored.
+ reset(eventHandler);
assertTrue(mWifiVendorHal.resetLogHandler());
mIWifiChipEventCallback.onDebugRingBufferDataAvailable(
new WifiDebugRingBufferStatus(), NativeUtil.byteArrayToArrayList(errorData));
+ mLooper.dispatchAll();
+ verify(eventHandler, never()).onRingBufferData(anyObject(), anyObject());
}
/**