From b0f1186c034c4df9eb54ed29944d16ce6d7ade56 Mon Sep 17 00:00:00 2001
From: Hugo Benichi <hugobenichi@google.com>
Date: Tue, 4 Oct 2016 11:24:12 +0900
Subject: [PATCH] Do not synchronize boolean reads/writes

This patch removes the synchronization around the private variable
mRunning inside of IpReachabilityMonitor and instead qualifeis the field
as volatile.

Synchronization is not needed for reads/writes on native fields or
object references because they are already guaranteed to be atomic.

Synchronization here was used for enforcing memory visibility across
concurrent threads indirectly through monitor acquire/release.
The volatile keyword achieves this in a more explicit way.

Also, this patch changes the way that probeAll() copies the
IpReachabilityMonitor's mIpWatchList by temporary holding mIpWatchList
keys into an ArrayList instead of a more expensive HashSet. Since Java
HashSet are just degenerated HashMaps, and that key iteration order is
based on key hash, the iteration order over this temporary collection
will be consistent for the same mIpWatchList.

Test: refactoring CL. Existing unit tests still pass.
Change-Id: I86ca6d54cb41ec78281e224a8d8ffd7155451132
---
 .../java/android/net/ip/IpReachabilityMonitor.java | 33 +++++++++-------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/services/net/java/android/net/ip/IpReachabilityMonitor.java b/services/net/java/android/net/ip/IpReachabilityMonitor.java
index a6bb40c6ac0b..a883e28e96c6 100644
--- a/services/net/java/android/net/ip/IpReachabilityMonitor.java
+++ b/services/net/java/android/net/ip/IpReachabilityMonitor.java
@@ -50,9 +50,9 @@ import java.net.NetworkInterface;
 import java.net.SocketAddress;
 import java.net.SocketException;
 import java.nio.ByteBuffer;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -163,8 +163,7 @@ public class IpReachabilityMonitor {
     private Map<InetAddress, Short> mIpWatchList = new HashMap<>();
     @GuardedBy("mLock")
     private int mIpWatchListVersion;
-    @GuardedBy("mLock")
-    private boolean mRunning;
+    private volatile boolean mRunning;
     // Time in milliseconds of the last forced probe request.
     private volatile long mLastProbeTimeMs;
 
@@ -246,7 +245,7 @@ public class IpReachabilityMonitor {
     }
 
     public void stop() {
-        synchronized (mLock) { mRunning = false; }
+        mRunning = false;
         clearLinkProperties();
         mNetlinkSocketObserver.clearNetlinkSocket();
     }
@@ -281,12 +280,6 @@ public class IpReachabilityMonitor {
         }
     }
 
-    private boolean stillRunning() {
-        synchronized (mLock) {
-            return mRunning;
-        }
-    }
-
     private static boolean isOnLink(List<RouteInfo> routes, InetAddress ip) {
         for (RouteInfo route : routes) {
             if (!route.hasGateway() && route.matches(ip)) {
@@ -390,12 +383,12 @@ public class IpReachabilityMonitor {
     }
 
     public void probeAll() {
-        Set<InetAddress> ipProbeList = new HashSet<InetAddress>();
+        final List<InetAddress> ipProbeList;
         synchronized (mLock) {
-            ipProbeList.addAll(mIpWatchList.keySet());
+            ipProbeList = new ArrayList<>(mIpWatchList.keySet());
         }
 
-        if (!ipProbeList.isEmpty() && stillRunning()) {
+        if (!ipProbeList.isEmpty() && mRunning) {
             // Keep the CPU awake long enough to allow all ARP/ND
             // probes a reasonable chance at success. See b/23197666.
             //
@@ -406,7 +399,7 @@ public class IpReachabilityMonitor {
         }
 
         for (InetAddress target : ipProbeList) {
-            if (!stillRunning()) {
+            if (!mRunning) {
                 break;
             }
             final int returnValue = probeNeighbor(mInterfaceIndex, target);
@@ -451,21 +444,21 @@ public class IpReachabilityMonitor {
         @Override
         public void run() {
             if (VDBG) { Log.d(TAG, "Starting observing thread."); }
-            synchronized (mLock) { mRunning = true; }
+            mRunning = true;
 
             try {
                 setupNetlinkSocket();
             } catch (ErrnoException | SocketException e) {
                 Log.e(TAG, "Failed to suitably initialize a netlink socket", e);
-                synchronized (mLock) { mRunning = false; }
+                mRunning = false;
             }
 
-            ByteBuffer byteBuffer;
-            while (stillRunning()) {
+            while (mRunning) {
+                final ByteBuffer byteBuffer;
                 try {
                     byteBuffer = recvKernelReply();
                 } catch (ErrnoException e) {
-                    if (stillRunning()) { Log.w(TAG, "ErrnoException: ", e); }
+                    if (mRunning) { Log.w(TAG, "ErrnoException: ", e); }
                     break;
                 }
                 final long whenMs = SystemClock.elapsedRealtime();
@@ -477,7 +470,7 @@ public class IpReachabilityMonitor {
 
             clearNetlinkSocket();
 
-            synchronized (mLock) { mRunning = false; }
+            mRunning = false; // Not a no-op when ErrnoException happened.
             if (VDBG) { Log.d(TAG, "Finishing observing thread."); }
         }
 
-- 
2.11.0