Fixed deadlock in BMv2 device context service

Also minor refactoring of synchronized blocks. Change-Id: Ifea25208ca4f1839bb3f21ba5b5ecfb2441baa35

Fixed deadlock in BMv2 device context service
Also minor refactoring of synchronized blocks. Change-Id: Ifea25208ca4f1839bb3f21ba5b5ecfb2441baa35
Carmelo Cascone · Gerrit Code Review
Commit 37208805ffc4400ee5bc4de17f21b371a093e9d6 37208805 1 parent 1a27960c
Showing 3 changed files with 35 additions and 26 deletions
drivers/bmv2/src/main/java/org/onosproject/drivers/bmv2/Bmv2FlowRuleProgrammable.java
protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2DeviceContextServiceImpl.java
providers/bmv2/device/src/main/java/org/onosproject/provider/bmv2/device/impl/Bmv2DeviceProvider.java
--- a/drivers/bmv2/src/main/java/org/onosproject/drivers/bmv2/Bmv2FlowRuleProgrammable.java
View file @3720880
+++ b/drivers/bmv2/src/main/java/org/onosproject/drivers/bmv2/Bmv2FlowRuleProgrammable.java
View file @3720880
@@ -49,6 +49,8 @@ import java.util.Collections;
 import java.util.Date;
 import java.util.List;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 import static org.onosproject.bmv2.api.runtime.Bmv2RuntimeException.Code.*;
 import static org.onosproject.net.flow.FlowEntry.FlowEntryState.ADDED;
@@ -61,7 +63,7 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
     private final Logger log = LoggerFactory.getLogger(this.getClass());
     // Needed to synchronize operations over the same table entry.
-    private static final ConcurrentMap<Bmv2TableEntryReference, Boolean> ENTRY_LOCKS = Maps.newConcurrentMap();
+    private static final ConcurrentMap<Bmv2TableEntryReference, Lock> ENTRY_LOCKS = Maps.newConcurrentMap();
     private Bmv2Controller controller;
     private Bmv2TableEntryService tableEntryService;
@@ -131,9 +133,10 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
                 Bmv2TableEntryReference entryRef = new Bmv2TableEntryReference(deviceId, table.name(),
                                                                                parsedEntry.matchKey());
-                ENTRY_LOCKS.putIfAbsent(entryRef, true);
+                Lock lock = ENTRY_LOCKS.computeIfAbsent(entryRef, key -> new ReentrantLock());
-                synchronized (ENTRY_LOCKS.get(entryRef)) {
+                lock.lock();
+                try {
                     Bmv2FlowRuleWrapper frWrapper = tableEntryService.lookup(entryRef);
                     if (frWrapper == null) {
@@ -173,6 +176,9 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
                     FlowEntry entry = new DefaultFlowEntry(frWrapper.rule(), ADDED, frWrapper.lifeInSeconds(),
                                                            packets, bytes);
                     entryList.add(entry);
+
+                } finally {
+                    lock.unlock();
                 }
             }
         }
@@ -232,8 +238,9 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
             String tableName = bmv2Entry.tableName();
             Bmv2TableEntryReference entryRef = new Bmv2TableEntryReference(deviceId, tableName, bmv2Entry.matchKey());
-            ENTRY_LOCKS.putIfAbsent(entryRef, true);
+            Lock lock = ENTRY_LOCKS.computeIfAbsent(entryRef, k -> new ReentrantLock());
-            synchronized (ENTRY_LOCKS.get(entryRef)) {
+            lock.lock();
+            try {
                 // Get from store
                 Bmv2FlowRuleWrapper frWrapper = tableEntryService.lookup(entryRef);
                 try {
@@ -273,6 +280,8 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
                 } else {
                     tableEntryService.unbind(entryRef);
                 }
+            } finally {
+                lock.unlock();
             }
         }
--- a/protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2DeviceContextServiceImpl.java
View file @3720880
+++ b/protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2DeviceContextServiceImpl.java
View file @3720880
@@ -60,6 +60,8 @@ import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static org.onosproject.bmv2.api.context.Bmv2DefaultConfiguration.parse;
@@ -87,7 +89,7 @@ public class Bmv2DeviceContextServiceImpl implements Bmv2DeviceContextService {
     private final ScheduledExecutorService scheduledExecutor = SharedScheduledExecutors.getPoolThreadExecutor();
     private final MapEventListener<DeviceId, Bmv2DeviceContext> contextListener = new ContextMapEventListener();
-    private final ConcurrentMap<DeviceId, Boolean> deviceLocks = Maps.newConcurrentMap();
+    private final ConcurrentMap<DeviceId, Lock> deviceLocks = Maps.newConcurrentMap();
     private ConsistentMap<DeviceId, Bmv2DeviceContext> contexts;
     private Map<String, ClassLoader> interpreterClassLoaders;
@@ -115,7 +117,7 @@ public class Bmv2DeviceContextServiceImpl implements Bmv2DeviceContextService {
         interpreterClassLoaders = Maps.newConcurrentMap();
         registerInterpreterClassLoader(defaultInterpreter.getClass(), this.getClass().getClassLoader());
-        contexts.addListener(contextListener);
+        contexts.addListener(contextListener, scheduledExecutor);
         if (configChecker != null &&  configChecker.isCancelled()) {
             configChecker.cancel(false);
@@ -169,16 +171,14 @@ public class Bmv2DeviceContextServiceImpl implements Bmv2DeviceContextService {
         return defaultContext;
     }
-    private void configCheck(DeviceId deviceId) {
+    private void configCheck(DeviceId deviceId, Bmv2DeviceContext storedContext) {
+        if (storedContext == null) {
+            return;
+        }
         // Synchronize executions over the same deviceId.
-        deviceLocks.putIfAbsent(deviceId, new Boolean(true));
+        Lock lock = deviceLocks.computeIfAbsent(deviceId, did -> new ReentrantLock());
-        synchronized (deviceLocks.get(deviceId)) {
+        lock.lock();
-
+        try {
-            Bmv2DeviceContext storedContext = getContext(deviceId);
-            if (storedContext == null) {
-                return;
-            }
-
             log.trace("Executing configuration check on {}...", deviceId);
             try {
@@ -200,18 +200,20 @@ public class Bmv2DeviceContextServiceImpl implements Bmv2DeviceContextService {
             } catch (Bmv2RuntimeException e) {
                 log.warn("Unable to dump JSON configuration from {}: {}", deviceId, e.explain());
             }
+        } finally {
+            lock.unlock();
         }
     }
-    private void triggerConfigCheck(DeviceId deviceId) {
+    private void triggerConfigCheck(DeviceId deviceId, Bmv2DeviceContext context) {
         if (mastershipService.isLocalMaster(deviceId)) {
-            scheduledExecutor.schedule(() -> configCheck(deviceId), 0, TimeUnit.SECONDS);
+            scheduledExecutor.schedule(() -> configCheck(deviceId, context), 0, TimeUnit.SECONDS);
         }
     }
     private void checkDevices() {
         deviceService.getAvailableDevices().forEach(device -> {
-            triggerConfigCheck(device.id());
+            triggerConfigCheck(device.id(), getContext(device.id()));
         });
     }
@@ -234,7 +236,7 @@ public class Bmv2DeviceContextServiceImpl implements Bmv2DeviceContextService {
             DeviceId deviceId = event.key();
             if (event.type().equals(INSERT) || event.type().equals(UPDATE)) {
                 log.trace("Context {} for {}", event.type().name(), deviceId);
-                triggerConfigCheck(deviceId);
+                triggerConfigCheck(deviceId, event.newValue().value());
             }
         }
     }
--- a/providers/bmv2/device/src/main/java/org/onosproject/provider/bmv2/device/impl/Bmv2DeviceProvider.java
View file @3720880
+++ b/providers/bmv2/device/src/main/java/org/onosproject/provider/bmv2/device/impl/Bmv2DeviceProvider.java
View file @3720880
@@ -269,12 +269,10 @@ public class Bmv2DeviceProvider extends AbstractDeviceProvider {
     private void disconnectDevice(DeviceId did) {
         log.debug("Trying to disconnect device from core... deviceId={}", did);
-        activeDevices.compute(did, (k, v) -> {
+        if (deviceService.isAvailable(did)) {
-            if (deviceService.isAvailable(did)) {
+            providerService.deviceDisconnected(did);
-                providerService.deviceDisconnected(did);
+        }
-            }
+        activeDevices.put(did, null);
-            return null;
-        });
     }
     /**