Fixed deadlock in BMv2 device context service
Also minor refactoring of synchronized blocks.
Change-Id: Ifea25208ca4f1839bb3f21ba5b5ecfb2441baa35
diff --git a/drivers/bmv2/src/main/java/org/onosproject/drivers/bmv2/Bmv2FlowRuleProgrammable.java b/drivers/bmv2/src/main/java/org/onosproject/drivers/bmv2/Bmv2FlowRuleProgrammable.java
index 403f68c..ce93422 100644
--- a/drivers/bmv2/src/main/java/org/onosproject/drivers/bmv2/Bmv2FlowRuleProgrammable.java
+++ b/drivers/bmv2/src/main/java/org/onosproject/drivers/bmv2/Bmv2FlowRuleProgrammable.java
@@ -49,6 +49,8 @@
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 @@
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 @@
Bmv2TableEntryReference entryRef = new Bmv2TableEntryReference(deviceId, table.name(),
parsedEntry.matchKey());
- ENTRY_LOCKS.putIfAbsent(entryRef, true);
- synchronized (ENTRY_LOCKS.get(entryRef)) {
+ Lock lock = ENTRY_LOCKS.computeIfAbsent(entryRef, key -> new ReentrantLock());
+ lock.lock();
+ try {
Bmv2FlowRuleWrapper frWrapper = tableEntryService.lookup(entryRef);
if (frWrapper == null) {
@@ -173,6 +176,9 @@
FlowEntry entry = new DefaultFlowEntry(frWrapper.rule(), ADDED, frWrapper.lifeInSeconds(),
packets, bytes);
entryList.add(entry);
+
+ } finally {
+ lock.unlock();
}
}
}
@@ -232,8 +238,9 @@
String tableName = bmv2Entry.tableName();
Bmv2TableEntryReference entryRef = new Bmv2TableEntryReference(deviceId, tableName, bmv2Entry.matchKey());
- ENTRY_LOCKS.putIfAbsent(entryRef, true);
- synchronized (ENTRY_LOCKS.get(entryRef)) {
+ Lock lock = ENTRY_LOCKS.computeIfAbsent(entryRef, k -> new ReentrantLock());
+ lock.lock();
+ try {
// Get from store
Bmv2FlowRuleWrapper frWrapper = tableEntryService.lookup(entryRef);
try {
@@ -273,6 +280,8 @@
} else {
tableEntryService.unbind(entryRef);
}
+ } finally {
+ lock.unlock();
}
}
diff --git a/protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2DeviceContextServiceImpl.java b/protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2DeviceContextServiceImpl.java
index 4727913..c6f3d04 100644
--- a/protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2DeviceContextServiceImpl.java
+++ b/protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2DeviceContextServiceImpl.java
@@ -60,6 +60,8 @@
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 @@
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 @@
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 @@
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));
- synchronized (deviceLocks.get(deviceId)) {
-
- Bmv2DeviceContext storedContext = getContext(deviceId);
- if (storedContext == null) {
- return;
- }
-
+ Lock lock = deviceLocks.computeIfAbsent(deviceId, did -> new ReentrantLock());
+ lock.lock();
+ try {
log.trace("Executing configuration check on {}...", deviceId);
try {
@@ -200,18 +200,20 @@
} 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 @@
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());
}
}
}
diff --git a/providers/bmv2/device/src/main/java/org/onosproject/provider/bmv2/device/impl/Bmv2DeviceProvider.java b/providers/bmv2/device/src/main/java/org/onosproject/provider/bmv2/device/impl/Bmv2DeviceProvider.java
index 1a9d4a0..2f30f06 100644
--- a/providers/bmv2/device/src/main/java/org/onosproject/provider/bmv2/device/impl/Bmv2DeviceProvider.java
+++ b/providers/bmv2/device/src/main/java/org/onosproject/provider/bmv2/device/impl/Bmv2DeviceProvider.java
@@ -269,12 +269,10 @@
private void disconnectDevice(DeviceId did) {
log.debug("Trying to disconnect device from core... deviceId={}", did);
- activeDevices.compute(did, (k, v) -> {
- if (deviceService.isAvailable(did)) {
- providerService.deviceDisconnected(did);
- }
- return null;
- });
+ if (deviceService.isAvailable(did)) {
+ providerService.deviceDisconnected(did);
+ }
+ activeDevices.put(did, null);
}
/**