From dfe06867bb110777fec5a82066486948b8221766 Mon Sep 17 00:00:00 2001
From: Robert Varga <robert.varga@pantheon.tech>
Date: Sun, 18 Nov 2018 20:15:14 +0100
Subject: [PATCH] Use JvmGlobalLocks in fibmanager

JvmGlobalLocks is giving us all we need to replace String.intern(),
user that.

JIRA: NETVIRT-1510
Change-Id: I50df601bb6c50cbcca72d2741cbbe66938ef5551
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
---
 .../netvirt/fibmanager/FibUtil.java           | 11 ++-
 .../RouterInterfaceVrfEntryHandler.java       | 12 ++-
 .../netvirt/fibmanager/VrfEntryListener.java  | 86 ++++++++++++++++---
 3 files changed, 91 insertions(+), 18 deletions(-)

diff --git a/fibmanager/impl/src/main/java/org/opendaylight/netvirt/fibmanager/FibUtil.java b/fibmanager/impl/src/main/java/org/opendaylight/netvirt/fibmanager/FibUtil.java
index 92c274ee53..c4442e0442 100644
--- a/fibmanager/impl/src/main/java/org/opendaylight/netvirt/fibmanager/FibUtil.java
+++ b/fibmanager/impl/src/main/java/org/opendaylight/netvirt/fibmanager/FibUtil.java
@@ -24,6 +24,7 @@ import java.util.List;
 import java.util.Objects;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
+import java.util.concurrent.locks.ReentrantLock;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.inject.Inject;
@@ -43,6 +44,7 @@ import org.opendaylight.genius.mdsalutil.BucketInfo;
 import org.opendaylight.genius.mdsalutil.MDSALUtil;
 import org.opendaylight.genius.mdsalutil.NWUtil;
 import org.opendaylight.genius.mdsalutil.NwConstants;
+import org.opendaylight.genius.utils.JvmGlobalLocks;
 import org.opendaylight.netvirt.elanmanager.api.IElanService;
 import org.opendaylight.netvirt.fibmanager.NexthopManager.AdjacencyResult;
 import org.opendaylight.netvirt.fibmanager.api.FibHelper;
@@ -486,8 +488,11 @@ public class FibUtil {
         LOG.debug("Updating fib entry for prefix {} with nextHop {} for rd {}.", prefix, nextHop, rd);
 
         InstanceIdentifier<RoutePaths> routePathId = FibHelper.buildRoutePathId(rd, prefix, nextHop);
-        String routePathKey = rd + prefix + nextHop;
-        synchronized (routePathKey.intern()) {
+
+        // FIXME: use routePathId instead?
+        final ReentrantLock lock = JvmGlobalLocks.getLockForString(rd + prefix + nextHop);
+        lock.lock();
+        try {
             if (nextHopAdd) {
                 RoutePaths routePaths = FibHelper.buildRoutePath(nextHop, label);
                 if (writeConfigTxn != null) {
@@ -512,6 +517,8 @@ public class FibUtil {
                 }
                 LOG.info("Removed routepath with nextHop {} for prefix {} and rd {}.", nextHop, prefix, rd);
             }
+        } finally {
+            lock.unlock();
         }
     }
 
diff --git a/fibmanager/impl/src/main/java/org/opendaylight/netvirt/fibmanager/RouterInterfaceVrfEntryHandler.java b/fibmanager/impl/src/main/java/org/opendaylight/netvirt/fibmanager/RouterInterfaceVrfEntryHandler.java
index f33b1315e4..cb8f0b52dc 100644
--- a/fibmanager/impl/src/main/java/org/opendaylight/netvirt/fibmanager/RouterInterfaceVrfEntryHandler.java
+++ b/fibmanager/impl/src/main/java/org/opendaylight/netvirt/fibmanager/RouterInterfaceVrfEntryHandler.java
@@ -12,6 +12,7 @@ import static org.opendaylight.genius.mdsalutil.NWUtil.isIpv4Address;
 import com.google.common.base.Preconditions;
 import java.math.BigInteger;
 import java.util.Collection;
+import java.util.concurrent.locks.ReentrantLock;
 import javax.inject.Inject;
 import javax.inject.Singleton;
 import org.opendaylight.controller.md.sal.binding.api.DataBroker;
@@ -19,6 +20,7 @@ import org.opendaylight.genius.datastoreutils.listeners.DataTreeEventCallbackReg
 import org.opendaylight.genius.mdsalutil.FlowEntity;
 import org.opendaylight.genius.mdsalutil.NwConstants;
 import org.opendaylight.genius.mdsalutil.interfaces.IMdsalApiManager;
+import org.opendaylight.genius.utils.JvmGlobalLocks;
 import org.opendaylight.netvirt.fibmanager.api.FibHelper;
 import org.opendaylight.netvirt.fibmanager.api.RouteOrigin;
 import org.opendaylight.serviceutils.upgrade.UpgradeState;
@@ -69,13 +71,17 @@ public class RouterInterfaceVrfEntryHandler extends BaseVrfEntryHandler implemen
         installRouterFibEntries(vrfEntry, rd, NwConstants.DEL_FLOW, routerInt);
     }
 
-    private Boolean installRouterFibEntries(VrfEntry vrfEntry, String rd, int addOrRemove,
+    private boolean installRouterFibEntries(VrfEntry vrfEntry, String rd, int addOrRemove,
             RouterInterface routerInterface) {
         final VpnInstanceOpDataEntry vpnInstance = getFibUtil().getVpnInstance(rd);
         Preconditions.checkNotNull(vpnInstance, "Vpn Instance not available " + rd);
         Preconditions.checkNotNull(vpnInstance.getVpnId(),
                 "Vpn Instance with rd " + vpnInstance.getVrfId() + " has null vpnId!");
-        synchronized (vpnInstance.getVpnInstanceName().intern()) {
+
+        // FIXME: separate this out somehow?
+        final ReentrantLock lock = JvmGlobalLocks.getLockForString(vpnInstance.getVpnInstanceName());
+        lock.lock();
+        try {
             final Collection<VpnToDpnList> vpnToDpnList;
             if (vrfEntry.getParentVpnRd() != null
                     && FibHelper.isControllerManagedNonSelfImportedRoute(RouteOrigin.value(vrfEntry.getOrigin()))) {
@@ -101,6 +107,8 @@ public class RouterInterfaceVrfEntryHandler extends BaseVrfEntryHandler implemen
                     }
                 }
             }
+        } finally {
+            lock.unlock();
         }
         return true;
     }
diff --git a/fibmanager/impl/src/main/java/org/opendaylight/netvirt/fibmanager/VrfEntryListener.java b/fibmanager/impl/src/main/java/org/opendaylight/netvirt/fibmanager/VrfEntryListener.java
index 75d9beaae7..a309c15158 100755
--- a/fibmanager/impl/src/main/java/org/opendaylight/netvirt/fibmanager/VrfEntryListener.java
+++ b/fibmanager/impl/src/main/java/org/opendaylight/netvirt/fibmanager/VrfEntryListener.java
@@ -32,6 +32,7 @@ import java.util.Objects;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.locks.ReentrantLock;
 import javax.annotation.Nullable;
 import javax.annotation.PostConstruct;
 import javax.inject.Inject;
@@ -68,6 +69,7 @@ import org.opendaylight.genius.mdsalutil.matches.MatchIpv4Destination;
 import org.opendaylight.genius.mdsalutil.matches.MatchMetadata;
 import org.opendaylight.genius.mdsalutil.matches.MatchMplsLabel;
 import org.opendaylight.genius.mdsalutil.matches.MatchTunnelId;
+import org.opendaylight.genius.utils.JvmGlobalLocks;
 import org.opendaylight.genius.utils.ServiceIndex;
 import org.opendaylight.genius.utils.batching.SubTransaction;
 import org.opendaylight.infrautils.jobcoordinator.JobCoordinator;
@@ -434,7 +436,9 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
         if (!localDpnIdList.isEmpty() && vpnToDpnList != null) {
             jobCoordinator.enqueueJob(FibUtil.getJobKeyForRdPrefix(rd, vrfEntry.getDestPrefix()),
                 () -> Collections.singletonList(txRunner.callWithNewWriteOnlyTransactionAndSubmit(CONFIGURATION, tx -> {
-                    synchronized (vpnInstance.getVpnInstanceName().intern()) {
+                    final ReentrantLock lock = lockFor(vpnInstance);
+                    lock.lock();
+                    try {
                         for (VpnToDpnList vpnDpn : vpnToDpnList) {
                             if (!localDpnIdList.contains(vpnDpn.getDpnId())) {
                                 if (vpnDpn.getDpnState() == VpnToDpnList.DpnState.Active) {
@@ -455,6 +459,8 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                                 }
                             }
                         }
+                    } finally {
+                        lock.unlock();
                     }
                 })), MAX_RETRIES);
         }
@@ -526,7 +532,10 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
         }
         FibUtil.getLabelFromRoutePaths(vrfEntry).ifPresent(label -> {
             List<String> nextHopAddressList = FibHelper.getNextHopListFromRoutePaths(vrfEntry);
-            synchronized (label.toString().intern()) {
+
+            final ReentrantLock lock = lockFor(label);
+            lock.lock();
+            try {
                 LabelRouteInfo lri = getLabelRouteInfo(label);
                 if (isPrefixAndNextHopPresentInLri(vrfEntry.getDestPrefix(), nextHopAddressList, lri)) {
 
@@ -543,6 +552,8 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                     LOG.debug("SUBNETROUTE: installSubnetRouteInFib: Fetched labelRouteInfo for label {} interface {}"
                             + " and got dpn {}", label, lri.getVpnInterfaceName(), lri.getDpnId());
                 }
+            } finally {
+                lock.unlock();
             }
         });
         final List<InstructionInfo> instructions = new ArrayList<>();
@@ -756,10 +767,12 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
         if (localNextHopInfo == null) {
             boolean localNextHopSeen = false;
             List<Routes> vpnExtraRoutes = null;
-            String rdPrefixKey = localNextHopIP + FibConstants.SEPARATOR + rd;
             //Synchronized to prevent missing bucket action due to race condition between refreshFib and
             // add/updateFib threads on missing nexthop in VpnToExtraroutes
-            synchronized (rdPrefixKey.intern()) {
+            // FIXME: use an Identifier structure?
+            final ReentrantLock lock = JvmGlobalLocks.getLockForString(localNextHopIP + FibConstants.SEPARATOR + rd);
+            lock.lock();
+            try {
                 List<String> usedRds = VpnExtraRouteHelper.getUsedRds(dataBroker, vpnId, localNextHopIP);
                 vpnExtraRoutes = VpnExtraRouteHelper.getAllVpnExtraRoutes(dataBroker,
                         vpnName, usedRds, localNextHopIP);
@@ -786,13 +799,17 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                         returnLocalDpnId.add(dpnId);
                     }
                 }
+            } finally {
+                lock.unlock();
             }
             if (!localNextHopSeen && RouteOrigin.value(vrfEntry.getOrigin()) == RouteOrigin.SELF_IMPORTED) {
                 java.util.Optional<Long> optionalLabel = FibUtil.getLabelFromRoutePaths(vrfEntry);
                 if (optionalLabel.isPresent()) {
                     Long label = optionalLabel.get();
                     List<String> nextHopAddressList = FibHelper.getNextHopListFromRoutePaths(vrfEntry);
-                    synchronized (label.toString().intern()) {
+                    final ReentrantLock labelLock = lockFor(label);
+                    labelLock.lock();
+                    try {
                         LabelRouteInfo lri = getLabelRouteInfo(label);
                         if (isPrefixAndNextHopPresentInLri(localNextHopIP, nextHopAddressList, lri)) {
                             Optional<VpnInstanceOpDataEntry> vpnInstanceOpDataEntryOptional =
@@ -823,6 +840,8 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                                 }
                             }
                         }
+                    } finally {
+                        labelLock.unlock();
                     }
                 }
             }
@@ -1188,7 +1207,7 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                     .stream()
                     .filter(route -> {
                         Prefixes prefixToInterface = fibUtil.getPrefixToInterface(vpnId,
-                                fibUtil.getIpPrefix(route.getNexthopIpList().get(0)));
+                                FibUtil.getIpPrefix(route.getNexthopIpList().get(0)));
                         if (prefixToInterface == null) {
                             return false;
                         }
@@ -1318,7 +1337,9 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                 if (VrfEntry.EncapType.Mplsgre.equals(vrfEntry.getEncapType())) {
                     FibUtil.getLabelFromRoutePaths(vrfEntry).ifPresent(label -> {
                         List<String> nextHopAddressList = FibHelper.getNextHopListFromRoutePaths(vrfEntry);
-                        synchronized (label.toString().intern()) {
+                        final ReentrantLock lock = lockFor(label);
+                        lock.lock();
+                        try {
                             LabelRouteInfo lri = getLabelRouteInfo(label);
                             if (lri != null && Objects.equals(lri.getPrefix(), vrfEntry.getDestPrefix())
                                     && nextHopAddressList.contains(lri.getNextHopIpList().get(0))) {
@@ -1338,6 +1359,8 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                                 fibUtil.releaseId(FibConstants.VPN_IDPOOL_NAME, FibUtil.getNextHopLabelKey(
                                         rd, vrfEntry.getDestPrefix()));
                             }
+                        } finally {
+                            lock.unlock();
                         }
                     });
                 }
@@ -1478,7 +1501,9 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                         })));
             }
             optionalLabel.ifPresent(label -> {
-                synchronized (label.toString().intern()) {
+                final ReentrantLock lock = lockFor(label);
+                lock.lock();
+                try {
                     LabelRouteInfo lri = getLabelRouteInfo(label);
                     if (isPrefixAndNextHopPresentInLri(vrfEntry.getDestPrefix(), nextHopAddressList, lri)) {
                         Optional<VpnInstanceOpDataEntry> vpnInstanceOpDataEntryOptional =
@@ -1501,6 +1526,8 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                         LOG.trace("SUBNETROUTE: deleteFibEntries: Released subnetroute label {} for rd {} prefix {}",
                                 label, rd, vrfEntry.getDestPrefix());
                     }
+                } finally {
+                    lock.unlock();
                 }
             });
             return;
@@ -1637,7 +1664,10 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                     }
                     return futures;
                 }
-                synchronized (vpnInstance.getVpnInstanceName().intern()) {
+
+                final ReentrantLock lock = lockFor(vpnInstance);
+                lock.lock();
+                try {
                     futures.add(retryingTxRunner.callWithNewReadWriteTransactionAndSubmit(CONFIGURATION, tx -> {
                         for (final VrfEntry vrfEntry : nullToEmpty(vrfTable.get().getVrfEntry())) {
                             SubnetRoute subnetRoute = vrfEntry.augmentation(SubnetRoute.class);
@@ -1698,6 +1728,8 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                         ListenableFuture<List<Void>> listenableFuture = Futures.allAsList(futures);
                         Futures.addCallback(listenableFuture, callback, MoreExecutors.directExecutor());
                     }
+                } finally {
+                    lock.unlock();
                 }
                 return futures;
             });
@@ -1714,11 +1746,15 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
         if (vrfTable.isPresent()) {
             jobCoordinator.enqueueJob(FibUtil.getJobKeyForVpnIdDpnId(vpnId, dpnId),
                 () -> Collections.singletonList(txRunner.callWithNewWriteOnlyTransactionAndSubmit(CONFIGURATION, tx -> {
-                    synchronized (vpnInstance.getVpnInstanceName().intern()) {
+                    final ReentrantLock lock = lockFor(vpnInstance);
+                    lock.lock();
+                    try {
                         nullToEmpty(vrfTable.get().getVrfEntry()).stream()
                             .filter(vrfEntry -> RouteOrigin.BGP == RouteOrigin.value(vrfEntry.getOrigin()))
                             .forEach(bgpRouteVrfEntryHandler.getConsumerForCreatingRemoteFib(dpnId, vpnId,
                                 rd, remoteNextHopIp, vrfTable, TransactionAdapter.toWriteTransaction(tx), txnObjects));
+                    } finally {
+                        lock.unlock();
                     }
                 })));
         }
@@ -1740,7 +1776,9 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
 
         jobCoordinator.enqueueJob(FibUtil.getJobKeyForVpnIdDpnId(vpnId, localDpnId),
             () -> Collections.singletonList(txRunner.callWithNewWriteOnlyTransactionAndSubmit(CONFIGURATION, tx -> {
-                synchronized (vpnInstance.getVpnInstanceName().intern()) {
+                final ReentrantLock lock = lockFor(vpnInstance);
+                lock.lock();
+                try {
                     VrfTablesKey vrfTablesKey = new VrfTablesKey(rd);
                     VrfEntry vrfEntry = getVrfEntry(dataBroker, rd, destPrefix);
                     if (vrfEntry == null) {
@@ -1781,6 +1819,8 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                         baseVrfEntryHandler.deleteRemoteRoute(null, localDpnId, vpnId, vrfTablesKey, modVrfEntry,
                                 extraRouteOptional, TransactionAdapter.toWriteTransaction(tx));
                     }
+                } finally {
+                    lock.unlock();
                 }
             })));
     }
@@ -1797,7 +1837,9 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                         LogicalDatastoreType.CONFIGURATION, id);
                 List<ListenableFuture<Void>> futures = new ArrayList<>();
                 if (vrfTable.isPresent()) {
-                    synchronized (vpnInstance.getVpnInstanceName().intern()) {
+                    final ReentrantLock lock = lockFor(vpnInstance);
+                    lock.lock();
+                    try {
                         futures.add(retryingTxRunner.callWithNewWriteOnlyTransactionAndSubmit(CONFIGURATION, tx -> {
                             String vpnName = fibUtil.getVpnNameFromId(vpnInstance.getVpnId());
                             for (final VrfEntry vrfEntry : nullToEmpty(vrfTable.get().getVrfEntry())) {
@@ -1888,6 +1930,8 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                                 }
                             }
                         }));
+                    } finally {
+                        lock.unlock();
                     }
                     if (callback != null) {
                         ListenableFuture<List<Void>> listenableFuture = Futures.allAsList(futures);
@@ -1913,7 +1957,9 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
         if (vrfTable.isPresent()) {
             jobCoordinator.enqueueJob(FibUtil.getJobKeyForVpnIdDpnId(vpnId, dpnId),
                 () -> {
-                    synchronized (vpnInstance.getVpnInstanceName().intern()) {
+                    final ReentrantLock lock = lockFor(vpnInstance);
+                    lock.lock();
+                    try {
                         return Collections.singletonList(
                             txRunner.callWithNewWriteOnlyTransactionAndSubmit(CONFIGURATION,
                                 tx -> nullToEmpty(vrfTable.get().getVrfEntry()).stream()
@@ -1921,6 +1967,8 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
                                     .forEach(bgpRouteVrfEntryHandler.getConsumerForDeletingRemoteFib(dpnId, vpnId,
                                         remoteNextHopIp, vrfTable, TransactionAdapter.toWriteTransaction(tx),
                                         txnObjects))));
+                    } finally {
+                        lock.unlock();
                     }
                 });
         }
@@ -2003,7 +2051,7 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
         });
     }
 
-    private boolean isPrefixAndNextHopPresentInLri(String prefix,
+    private static boolean isPrefixAndNextHopPresentInLri(String prefix,
             List<String> nextHopAddressList, LabelRouteInfo lri) {
         return lri != null && Objects.equals(lri.getPrefix(), prefix)
                 && nextHopAddressList.contains(lri.getNextHopIpList().get(0));
@@ -2025,4 +2073,14 @@ public class VrfEntryListener extends AsyncDataTreeChangeListenerBase<VrfEntry,
         }
         return true;
     }
+
+    private static ReentrantLock lockFor(final VpnInstanceOpDataEntry vpnInstance) {
+        // FIXME: use vpnInstance.key() instead?
+        return JvmGlobalLocks.getLockForString(vpnInstance.getVpnInstanceName());
+    }
+
+    private static ReentrantLock lockFor(Long label) {
+        // FIXME: use LabelRouteInfoKey instead?
+        return JvmGlobalLocks.getLockForString(label.toString());
+    }
 }
-- 
2.36.6