Improves rules population

- offloading redoRoutingEdgePairs to predictable callables
- offloading redoRoutingIndDests to predictable callables

Change-Id: Ia08e2cdbd03c513ec15900fd5117cc055cd95d4e
diff --git a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
index 6890f63..e4cfc0a 100644
--- a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
+++ b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
@@ -28,6 +28,8 @@
 import org.onlab.packet.IpPrefix;
 import org.onlab.packet.MacAddress;
 import org.onlab.packet.VlanId;
+import org.onlab.util.PredictableExecutor;
+import org.onlab.util.PredictableExecutor.PickyCallable;
 import org.onosproject.cluster.NodeId;
 import org.onosproject.mastership.MastershipEvent;
 import org.onosproject.net.ConnectPoint;
@@ -54,6 +56,10 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.locks.Lock;
@@ -91,6 +97,9 @@
         = newScheduledThreadPool(1, groupedThreads("masterChg", "mstch-%d", log));
     private ScheduledExecutorService executorServiceFRR
         = newScheduledThreadPool(1, groupedThreads("fullRR", "fullRR-%d", log));
+    // Route populators - 0 will leverage available processors
+    private static final int DEFAULT_THREADS = 0;
+    private ExecutorService routePopulators;
 
     private Instant lastRoutingChange = Instant.EPOCH;
     private Instant lastFullReroute = Instant.EPOCH;
@@ -111,14 +120,10 @@
     public enum Status {
         // population process is not started yet.
         IDLE,
-
         // population process started.
         STARTED,
-
-        // population process was aborted due to errors, mostly for groups not
-        // found.
+        // population process was aborted due to errors, mostly for groups not found.
         ABORTED,
-
         // population process was finished successfully.
         SUCCEEDED
     }
@@ -136,6 +141,8 @@
                 .build().asJavaMap();
         this.shouldProgramCache = Maps.newConcurrentMap();
         update(srManager);
+        this.routePopulators = new PredictableExecutor(DEFAULT_THREADS,
+                                                      groupedThreads("onos/sr", "r-populator-%d", log));
     }
 
     /**
@@ -204,6 +211,7 @@
         executorService.shutdown();
         executorServiceMstChg.shutdown();
         executorServiceFRR.shutdown();
+        routePopulators.shutdown();
     }
 
     //////////////////////////////////////
@@ -647,9 +655,8 @@
      *                     the path.
      * @return true if successful
      */
-    private boolean redoRoutingEdgePairs(Set<EdgePair> edgePairs,
-                                      Set<IpPrefix> subnets,
-                                      Set<ArrayList<DeviceId>> changedRoutes) {
+    private boolean redoRoutingEdgePairs(Set<EdgePair> edgePairs, Set<IpPrefix> subnets,
+                                         Set<ArrayList<DeviceId>> changedRoutes) {
         for (EdgePair ep : edgePairs) {
             // temp store for a target's changedRoutes to this edge-pair
             Map<DeviceId, Set<ArrayList<DeviceId>>> targetRoutes = new HashMap<>();
@@ -682,102 +689,135 @@
             }
             // so now for this edgepair we have a per target set of routechanges
             // process target->edgePair route
+            List<Future<Boolean>> futures = Lists.newArrayList();
             for (Map.Entry<DeviceId, Set<ArrayList<DeviceId>>> entry :
                             targetRoutes.entrySet()) {
                 log.debug("* redoRoutingDstPair Target:{} -> edge-pair {}",
                           entry.getKey(), ep);
-                DeviceId targetSw = entry.getKey();
-                Map<DeviceId, Set<DeviceId>> perDstNextHops = new HashMap<>();
-                entry.getValue().forEach(route -> {
-                    Set<DeviceId> nhops = getNextHops(route.get(0), route.get(1));
-                    log.debug("route: target {} -> dst {} found with next-hops {}",
-                              route.get(0), route.get(1), nhops);
-                    perDstNextHops.put(route.get(1), nhops);
-                });
-
-                List<Set<IpPrefix>> batchedSubnetDev1, batchedSubnetDev2;
-                if (subnets != null) {
-                    batchedSubnetDev1 = Lists.<Set<IpPrefix>>newArrayList(Sets.newHashSet(subnets));
-                    batchedSubnetDev2 = Lists.<Set<IpPrefix>>newArrayList(Sets.newHashSet(subnets));
-                } else {
-                    batchedSubnetDev1 = config.getBatchedSubnets(ep.dev1);
-                    batchedSubnetDev2 = config.getBatchedSubnets(ep.dev2);
-                }
-                List<Set<IpPrefix>> batchedSubnetBoth = Streams
-                        .zip(batchedSubnetDev1.stream(), batchedSubnetDev2.stream(), (a, b) -> Sets.intersection(a, b))
-                        .filter(set -> !set.isEmpty())
-                        .collect(Collectors.toList());
-                List<Set<IpPrefix>> batchedSubnetDev1Only = Streams
-                        .zip(batchedSubnetDev1.stream(), batchedSubnetDev2.stream(), (a, b) -> Sets.difference(a, b))
-                        .filter(set -> !set.isEmpty())
-                        .collect(Collectors.toList());
-                List<Set<IpPrefix>> batchedSubnetDev2Only = Streams
-                        .zip(batchedSubnetDev1.stream(), batchedSubnetDev2.stream(), (a, b) -> Sets.difference(b, a))
-                        .filter(set -> !set.isEmpty())
-                        .collect(Collectors.toList());
-
-                Set<DeviceId> nhDev1 = perDstNextHops.get(ep.dev1);
-                Set<DeviceId> nhDev2 = perDstNextHops.get(ep.dev2);
-
-                // handle routing to subnets common to edge-pair
-                // only if the targetSw is not part of the edge-pair and there
-                // exists a next hop to at least one of the devices in the edge-pair
-                if (!ep.includes(targetSw)
-                        && ((nhDev1 != null && !nhDev1.isEmpty()) || (nhDev2 != null && !nhDev2.isEmpty()))) {
-                    log.trace("getSubnets on both {} and {}: {}", ep.dev1, ep.dev2, batchedSubnetBoth);
-                    for (Set<IpPrefix> prefixes : batchedSubnetBoth) {
-                        if (!populateEcmpRoutingRulePartial(
-                                targetSw,
-                                ep.dev1, ep.dev2,
-                                perDstNextHops,
-                                prefixes)) {
-                            return false; // abort everything and fail fast
-                        }
-                    }
-
-                }
-                // handle routing to subnets that only belong to dev1 only if
-                // a next-hop exists from the target to dev1
-                if (!batchedSubnetDev1Only.isEmpty() &&
-                        batchedSubnetDev1Only.stream().anyMatch(subnet -> !subnet.isEmpty()) &&
-                        nhDev1 != null  && !nhDev1.isEmpty()) {
-                    Map<DeviceId, Set<DeviceId>> onlyDev1NextHops = new HashMap<>();
-                    onlyDev1NextHops.put(ep.dev1, nhDev1);
-                    log.trace("getSubnets on {} only: {}", ep.dev1, batchedSubnetDev1Only);
-                    for (Set<IpPrefix> prefixes : batchedSubnetDev1Only) {
-                        if (!populateEcmpRoutingRulePartial(
-                                targetSw,
-                                ep.dev1, null,
-                                onlyDev1NextHops,
-                                prefixes)) {
-                            return false; // abort everything and fail fast
-                        }
-                    }
-                }
-                // handle routing to subnets that only belong to dev2 only if
-                // a next-hop exists from the target to dev2
-                if (!batchedSubnetDev2Only.isEmpty() &&
-                        batchedSubnetDev2Only.stream().anyMatch(subnet -> !subnet.isEmpty()) &&
-                        nhDev2 != null && !nhDev2.isEmpty()) {
-                    Map<DeviceId, Set<DeviceId>> onlyDev2NextHops = new HashMap<>();
-                    onlyDev2NextHops.put(ep.dev2, nhDev2);
-                    log.trace("getSubnets on {} only: {}", ep.dev2, batchedSubnetDev2Only);
-                    for (Set<IpPrefix> prefixes : batchedSubnetDev2Only) {
-                        if (!populateEcmpRoutingRulePartial(
-                                targetSw,
-                                ep.dev2, null,
-                                onlyDev2NextHops,
-                                prefixes)) {
-                            return false; // abort everything and fail fast
-                        }
-                    }
-                }
+                futures.add(routePopulators.submit(new RedoRoutingEdgePair(entry.getKey(), entry.getValue(),
+                                                                           subnets, ep)));
+            }
+            if (!checkJobs(futures)) {
+                return false;
             }
             // if it gets here it has succeeded for all targets to this edge-pair
         }
         return true;
     }
 
+    private final class RedoRoutingEdgePair implements PickyCallable<Boolean> {
+        private DeviceId targetSw;
+        private Set<ArrayList<DeviceId>> routes;
+        private Set<IpPrefix> subnets;
+        private EdgePair ep;
+
+        /**
+         * Builds a RedoRoutingEdgePair task which provides a result.
+         *
+         * @param targetSw the target switch
+         * @param routes the changed routes
+         * @param subnets the subnets
+         * @param ep the edge pair
+         */
+        RedoRoutingEdgePair(DeviceId targetSw, Set<ArrayList<DeviceId>> routes,
+                            Set<IpPrefix> subnets, EdgePair ep) {
+            this.targetSw = targetSw;
+            this.routes = routes;
+            this.subnets = subnets;
+            this.ep = ep;
+        }
+
+        @Override
+        public Boolean call() throws Exception {
+            return redoRoutingEdgePair();
+        }
+
+        @Override
+        public int hint() {
+            return targetSw.hashCode();
+        }
+
+        private boolean redoRoutingEdgePair() {
+            Map<DeviceId, Set<DeviceId>> perDstNextHops = new HashMap<>();
+            routes.forEach(route -> {
+                Set<DeviceId> nhops = getNextHops(route.get(0), route.get(1));
+                log.debug("route: target {} -> dst {} found with next-hops {}",
+                          route.get(0), route.get(1), nhops);
+                perDstNextHops.put(route.get(1), nhops);
+            });
+
+            List<Set<IpPrefix>> batchedSubnetDev1, batchedSubnetDev2;
+            if (subnets != null) {
+                batchedSubnetDev1 = Lists.<Set<IpPrefix>>newArrayList(Sets.newHashSet(subnets));
+                batchedSubnetDev2 = Lists.<Set<IpPrefix>>newArrayList(Sets.newHashSet(subnets));
+            } else {
+                batchedSubnetDev1 = config.getBatchedSubnets(ep.dev1);
+                batchedSubnetDev2 = config.getBatchedSubnets(ep.dev2);
+            }
+            List<Set<IpPrefix>> batchedSubnetBoth = Streams
+                    .zip(batchedSubnetDev1.stream(), batchedSubnetDev2.stream(), (a, b) -> Sets.intersection(a, b))
+                    .filter(set -> !set.isEmpty())
+                    .collect(Collectors.toList());
+            List<Set<IpPrefix>> batchedSubnetDev1Only = Streams
+                    .zip(batchedSubnetDev1.stream(), batchedSubnetDev2.stream(), (a, b) -> Sets.difference(a, b))
+                    .filter(set -> !set.isEmpty())
+                    .collect(Collectors.toList());
+            List<Set<IpPrefix>> batchedSubnetDev2Only = Streams
+                    .zip(batchedSubnetDev1.stream(), batchedSubnetDev2.stream(), (a, b) -> Sets.difference(b, a))
+                    .filter(set -> !set.isEmpty())
+                    .collect(Collectors.toList());
+
+            Set<DeviceId> nhDev1 = perDstNextHops.get(ep.dev1);
+            Set<DeviceId> nhDev2 = perDstNextHops.get(ep.dev2);
+
+            // handle routing to subnets common to edge-pair
+            // only if the targetSw is not part of the edge-pair and there
+            // exists a next hop to at least one of the devices in the edge-pair
+            if (!ep.includes(targetSw)
+                    && ((nhDev1 != null && !nhDev1.isEmpty()) || (nhDev2 != null && !nhDev2.isEmpty()))) {
+                log.trace("getSubnets on both {} and {}: {}", ep.dev1, ep.dev2, batchedSubnetBoth);
+                for (Set<IpPrefix> prefixes : batchedSubnetBoth) {
+                    if (!populateEcmpRoutingRulePartial(targetSw, ep.dev1, ep.dev2,
+                                                        perDstNextHops, prefixes)) {
+                        return false; // abort everything and fail fast
+                    }
+                }
+
+            }
+            // handle routing to subnets that only belong to dev1 only if
+            // a next-hop exists from the target to dev1
+            if (!batchedSubnetDev1Only.isEmpty() &&
+                    batchedSubnetDev1Only.stream().anyMatch(subnet -> !subnet.isEmpty()) &&
+                    nhDev1 != null  && !nhDev1.isEmpty()) {
+                Map<DeviceId, Set<DeviceId>> onlyDev1NextHops = new HashMap<>();
+                onlyDev1NextHops.put(ep.dev1, nhDev1);
+                log.trace("getSubnets on {} only: {}", ep.dev1, batchedSubnetDev1Only);
+                for (Set<IpPrefix> prefixes : batchedSubnetDev1Only) {
+                    if (!populateEcmpRoutingRulePartial(targetSw, ep.dev1, null,
+                                                        onlyDev1NextHops, prefixes)) {
+                        return false; // abort everything and fail fast
+                    }
+                }
+            }
+            // handle routing to subnets that only belong to dev2 only if
+            // a next-hop exists from the target to dev2
+            if (!batchedSubnetDev2Only.isEmpty() &&
+                    batchedSubnetDev2Only.stream().anyMatch(subnet -> !subnet.isEmpty()) &&
+                    nhDev2 != null && !nhDev2.isEmpty()) {
+                Map<DeviceId, Set<DeviceId>> onlyDev2NextHops = new HashMap<>();
+                onlyDev2NextHops.put(ep.dev2, nhDev2);
+                log.trace("getSubnets on {} only: {}", ep.dev2, batchedSubnetDev2Only);
+                for (Set<IpPrefix> prefixes : batchedSubnetDev2Only) {
+                    if (!populateEcmpRoutingRulePartial(targetSw, ep.dev2, null,
+                                                        onlyDev2NextHops, prefixes)) {
+                        return false; // abort everything and fail fast
+                    }
+                }
+            }
+            return true;
+        }
+    }
+
     /**
      * Programs targetSw in the changedRoutes for given prefixes reachable by
      * a destination switch that is not part of an edge-pair.
@@ -793,8 +833,7 @@
      *                     the path.
      * @return true if successful
      */
-    private boolean redoRoutingIndividualDests(Set<IpPrefix> subnets,
-                                               Set<ArrayList<DeviceId>> changedRoutes,
+    private boolean redoRoutingIndividualDests(Set<IpPrefix> subnets, Set<ArrayList<DeviceId>> changedRoutes,
                                                Set<DeviceId> updatedDevices) {
         // aggregate route-path changes for each dst device
         HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>> routesBydevice =
@@ -809,28 +848,19 @@
             }
             deviceRoutes.add(route);
         }
+        // iterate over the impacted devices
         for (DeviceId impactedDstDevice : routesBydevice.keySet()) {
             ArrayList<ArrayList<DeviceId>> deviceRoutes =
                     routesBydevice.get(impactedDstDevice);
+            List<Future<Boolean>> futures = Lists.newArrayList();
             for (ArrayList<DeviceId> route: deviceRoutes) {
                 log.debug("* redoRoutingIndiDst Target: {} -> dst: {}",
                           route.get(0), route.get(1));
-                DeviceId targetSw = route.get(0);
-                DeviceId dstSw = route.get(1); // same as impactedDstDevice
-                Set<DeviceId> nextHops = getNextHops(targetSw, dstSw);
-                if (nextHops.isEmpty()) {
-                    log.debug("Could not find next hop from target:{} --> dst {} "
-                            + "skipping this route", targetSw, dstSw);
-                    continue;
-                }
-                Map<DeviceId, Set<DeviceId>> nhops = new HashMap<>();
-                nhops.put(dstSw, nextHops);
-                if (!populateEcmpRoutingRulePartial(targetSw, dstSw, null, nhops,
-                         (subnets == null) ? Sets.newHashSet() : subnets)) {
-                    return false; // abort routing and fail fast
-                }
-                log.debug("Populating flow rules from target: {} to dst: {}"
-                        + " is successful", targetSw, dstSw);
+                futures.add(routePopulators.submit(new RedoRoutingIndividualDest(subnets, route)));
+            }
+            // check the execution of each job
+            if (!checkJobs(futures)) {
+                return false;
             }
             //Only if all the flows for all impacted routes to a
             //specific target are pushed successfully, update the
@@ -847,6 +877,49 @@
         return true;
     }
 
+    private final class RedoRoutingIndividualDest implements PickyCallable<Boolean> {
+        private DeviceId targetSw;
+        private ArrayList<DeviceId> route;
+        private Set<IpPrefix> subnets;
+
+        /**
+         * Builds a RedoRoutingIndividualDest task, which provides a result.
+         *
+         * @param subnets a set of prefixes
+         * @param route a route-path change
+         */
+        RedoRoutingIndividualDest(Set<IpPrefix> subnets, ArrayList<DeviceId> route) {
+            this.targetSw = route.get(0);
+            this.route = route;
+            this.subnets = subnets;
+        }
+
+        @Override
+        public Boolean call() throws Exception {
+            DeviceId dstSw = route.get(1); // same as impactedDstDevice
+            Set<DeviceId> nextHops = getNextHops(targetSw, dstSw);
+            if (nextHops.isEmpty()) {
+                log.debug("Could not find next hop from target:{} --> dst {} "
+                                  + "skipping this route", targetSw, dstSw);
+                return true;
+            }
+            Map<DeviceId, Set<DeviceId>> nhops = new HashMap<>();
+            nhops.put(dstSw, nextHops);
+            if (!populateEcmpRoutingRulePartial(targetSw, dstSw, null, nhops,
+                                                (subnets == null) ? Sets.newHashSet() : subnets)) {
+                return false; // abort routing and fail fast
+            }
+            log.debug("Populating flow rules from target: {} to dst: {}"
+                              + " is successful", targetSw, dstSw);
+            return true;
+        }
+
+        @Override
+        public int hint() {
+            return targetSw.hashCode();
+        }
+    }
+
     /**
      * Populate ECMP rules for subnets from target to destination via nexthops.
      *
@@ -858,11 +931,8 @@
      * @param subnets Subnets to be populated. If empty, populate all configured subnets.
      * @return true if it succeeds in populating rules
      */ // refactor
-    private boolean populateEcmpRoutingRulePartial(DeviceId targetSw,
-                                                   DeviceId destSw1,
-                                                   DeviceId destSw2,
-                                                   Map<DeviceId, Set<DeviceId>> nextHops,
-                                                   Set<IpPrefix> subnets) {
+    private boolean populateEcmpRoutingRulePartial(DeviceId targetSw, DeviceId destSw1, DeviceId destSw2,
+                                                   Map<DeviceId, Set<DeviceId>> nextHops, Set<IpPrefix> subnets) {
         boolean result;
         // If both target switch and dest switch are edge routers, then set IP
         // rule for both subnet and router IP.
@@ -921,9 +991,7 @@
             // individual destinations, even if the dsts are part of edge-pairs.
             log.debug(". populateEcmpRoutingRulePartial in device{} towards {} for "
                     + "all MPLS rules", targetSw, destSw1);
-            result = rulePopulator.populateMplsRule(targetSw, destSw1,
-                                                    nextHops.get(destSw1),
-                                                    dest1RouterIpv4);
+            result = rulePopulator.populateMplsRule(targetSw, destSw1, nextHops.get(destSw1), dest1RouterIpv4);
             if (!result) {
                 return false;
             }
@@ -936,8 +1004,7 @@
                     log.warn(e.getMessage());
                 }
                 if (v4sid != v6sid) {
-                    result = rulePopulator.populateMplsRule(targetSw, destSw1,
-                                                            nextHops.get(destSw1),
+                    result = rulePopulator.populateMplsRule(targetSw, destSw1, nextHops.get(destSw1),
                                                             dest1RouterIpv6);
                     if (!result) {
                         return false;
@@ -952,9 +1019,7 @@
             log.debug(". populateEcmpRoutingRulePartial in device{} towards {} for "
                               + "all MPLS rules", targetSw, destSw1);
 
-            result = rulePopulator.populateMplsRule(targetSw, destSw1,
-                                                        nextHops.get(destSw1),
-                                                        dest1RouterIpv4);
+            result = rulePopulator.populateMplsRule(targetSw, destSw1, nextHops.get(destSw1), dest1RouterIpv4);
             if (!result) {
                 return false;
             }
@@ -968,8 +1033,7 @@
                     log.warn(e.getMessage());
                 }
                 if (v4sid != v6sid) {
-                    result = rulePopulator.populateMplsRule(targetSw, destSw1,
-                                                            nextHops.get(destSw1),
+                    result = rulePopulator.populateMplsRule(targetSw, destSw1, nextHops.get(destSw1),
                                                             dest1RouterIpv6);
                     if (!result) {
                         return false;
@@ -1139,14 +1203,43 @@
      * @return true if succeed
      */
     protected boolean revokeSubnet(Set<IpPrefix> subnets) {
-        statusLock.lock();
-        try {
-            return Sets.newHashSet(srManager.deviceService.getAvailableDevices()).stream()
-                    .map(Device::id)
-                    .filter(this::shouldProgram)
-                    .allMatch(targetSw -> srManager.routingRulePopulator.revokeIpRuleForSubnet(targetSw, subnets));
-        } finally {
-            statusLock.unlock();
+        DeviceId targetSw;
+        List<Future<Boolean>> futures = Lists.newArrayList();
+        for (Device sw : srManager.deviceService.getAvailableDevices()) {
+            targetSw = sw.id();
+            if (shouldProgram(targetSw)) {
+                futures.add(routePopulators.submit(new RevokeSubnet(targetSw, subnets)));
+            } else {
+                futures.add(CompletableFuture.completedFuture(true));
+            }
+        }
+        // check the execution of each job
+        return checkJobs(futures);
+    }
+
+    private final class RevokeSubnet implements PickyCallable<Boolean> {
+        private DeviceId targetSw;
+        private Set<IpPrefix> subnets;
+
+        /**
+         * Builds a RevokeSubnet task, which provides a result.
+         *
+         * @param subnets a set of prefixes
+         * @param targetSw target switch
+         */
+        RevokeSubnet(DeviceId targetSw, Set<IpPrefix> subnets) {
+            this.targetSw = targetSw;
+            this.subnets = subnets;
+        }
+
+        @Override
+        public Boolean call() throws Exception {
+            return srManager.routingRulePopulator.revokeIpRuleForSubnet(targetSw, subnets);
+        }
+
+        @Override
+        public int hint() {
+            return targetSw.hashCode();
         }
     }
 
@@ -1869,4 +1962,24 @@
             prevRun = (thisRun == null) ? prevRun : thisRun;
         }
     }
+
+    // Check jobs completion. It returns false if one of the job fails
+    // and cancel the remaining
+    private boolean checkJobs(List<Future<Boolean>> futures) {
+        boolean completed = true;
+        for (Future<Boolean> future : futures) {
+            try {
+                if (completed) {
+                    if (!future.get()) {
+                        completed = false;
+                    }
+                } else {
+                    future.cancel(true);
+                }
+            } catch (InterruptedException | ExecutionException e) {
+                completed = false;
+            }
+        }
+        return completed;
+    }
 }
diff --git a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
index 28fe080..fe61cd0 100644
--- a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
+++ b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
@@ -76,7 +76,6 @@
 import static org.onlab.packet.ICMP6.ROUTER_SOLICITATION;
 import static org.onlab.packet.IPv6.PROTOCOL_ICMP6;
 import static org.onosproject.segmentrouting.SegmentRoutingManager.INTERNAL_VLAN;
-
 import static org.onosproject.segmentrouting.SegmentRoutingManager.PSEUDOWIRE_VLAN;
 
 /**
@@ -481,10 +480,11 @@
         // Route simplification will be off in case of the nexthop location at target switch is down
         // (routing through spine case)
         boolean routeSimplOff = pairDev.isPresent() && pairDev.get().equals(destSw1) && destSw2 == null;
-        // Iterates over the routes
+        // Iterates over the routes. Checking:
         // If route simplification is enabled
         // If the target device is another leaf in the network
         if (srManager.routeSimplification && !routeSimplOff) {
+            Set<IpPrefix> subnetsToBePopulated = Sets.newHashSet();
             for (IpPrefix subnet : subnets) {
                 // Skip route programming on the target device
                 // If route simplification applies
@@ -495,19 +495,12 @@
                     continue;
                 }
                 // populate the route in the remaning scenarios
-                if (!populateIpRuleForRouter(targetSw, subnet, destSw1, destSw2, nextHops)) {
-                    return false;
-                }
+                subnetsToBePopulated.add(subnet);
             }
-        } else {
-            // Populate IP flow rules for all the subnets.
-            for (IpPrefix subnet : subnets) {
-                if (!populateIpRuleForRouter(targetSw, subnet, destSw1, destSw2, nextHops)) {
-                    return false;
-                }
-            }
+            subnets = subnetsToBePopulated;
         }
-        return true;
+        // populate the remaining routes in the target switch
+        return populateIpRulesForRouter(targetSw, subnets, destSw1, destSw2, nextHops);
     }
 
     /**
@@ -527,11 +520,11 @@
     }
 
     /**
-     * Populates IP flow rules for an IP prefix in the target device. The prefix
-     * is reachable via destination device(s).
+     * Populates IP flow rules for a set of IP prefix in the target device.
+     * The prefix are reachable via destination device(s).
      *
      * @param targetSw target device ID to set the rules
-     * @param ipPrefix the IP prefix
+     * @param subnets the set of IP prefix
      * @param destSw1 destination switch where the prefixes are reachable
      * @param destSw2 paired destination switch if one exists for the subnets/prefixes.
      *                Should be null if there is no paired destination switch (by config)
@@ -542,98 +535,111 @@
      *                  should not be an entry for destSw2 in this map.
      * @return true if all rules are set successfully, false otherwise
      */
-    private boolean populateIpRuleForRouter(DeviceId targetSw,
-                                           IpPrefix ipPrefix, DeviceId destSw1,
-                                           DeviceId destSw2,
-                                           Map<DeviceId, Set<DeviceId>> nextHops) {
-        int segmentId1, segmentId2 = -1;
+    private boolean populateIpRulesForRouter(DeviceId targetSw,
+                                             Set<IpPrefix> subnets,
+                                             DeviceId destSw1, DeviceId destSw2,
+                                             Map<DeviceId, Set<DeviceId>> nextHops) {
+        // pre-compute the needed information
+        int segmentIdIPv41, segmentIdIPv42 = -1;
+        int segmentIdIPv61, segmentIdIPv62 = -1;
+        TrafficTreatment treatment = null;
+        DestinationSet dsIPv4, dsIPv6;
+        TrafficSelector metaIpv4Selector, metaIpv6Selector = null;
+        int nextIdIPv4, nextIdIPv6, nextId;
+        TrafficSelector selector;
+        // start with MPLS SIDs
         try {
-            if (ipPrefix.isIp4()) {
-                segmentId1 = config.getIPv4SegmentId(destSw1);
-                if (destSw2 != null) {
-                    segmentId2 = config.getIPv4SegmentId(destSw2);
-                }
-            } else {
-                segmentId1 = config.getIPv6SegmentId(destSw1);
-                if (destSw2 != null) {
-                    segmentId2 = config.getIPv6SegmentId(destSw2);
-                }
+            segmentIdIPv41 = config.getIPv4SegmentId(destSw1);
+            segmentIdIPv61 = config.getIPv6SegmentId(destSw1);
+            if (destSw2 != null) {
+                segmentIdIPv42 = config.getIPv4SegmentId(destSw2);
+                segmentIdIPv62 = config.getIPv6SegmentId(destSw2);
             }
         } catch (DeviceConfigNotFoundException e) {
             log.warn(e.getMessage() + " Aborting populateIpRuleForRouter.");
             return false;
         }
-
-        TrafficSelector.Builder sbuilder = buildIpSelectorFromIpPrefix(ipPrefix);
-        TrafficSelector selector = sbuilder.build();
-
-        TrafficTreatment.Builder tbuilder = DefaultTrafficTreatment.builder();
-        DestinationSet ds;
-        TrafficTreatment treatment;
-        DestinationSet.DestinationSetType dsType;
-
+        // build the IPv4 and IPv6 destination set
         if (destSw2 == null) {
             // single dst - create destination set based on next-hop
             // If the next hop is the same as the final destination, then MPLS
             // label is not set.
             Set<DeviceId> nhd1 = nextHops.get(destSw1);
             if (nhd1.size() == 1 && nhd1.iterator().next().equals(destSw1)) {
-                tbuilder.immediate().decNwTtl();
-                ds = DestinationSet.createTypePushNone(destSw1);
-                treatment = tbuilder.build();
+                dsIPv4 = DestinationSet.createTypePushNone(destSw1);
+                dsIPv6 = DestinationSet.createTypePushNone(destSw1);
+                treatment = DefaultTrafficTreatment.builder()
+                        .immediate()
+                        .decNwTtl()
+                        .build();
             } else {
-                ds = DestinationSet.createTypePushBos(segmentId1, destSw1);
-                treatment = null;
+                dsIPv4 = DestinationSet.createTypePushBos(segmentIdIPv41, destSw1);
+                dsIPv6 = DestinationSet.createTypePushBos(segmentIdIPv61, destSw1);
             }
         } else {
             // dst pair - IP rules for dst-pairs are always from other edge nodes
             // the destination set needs to have both destinations, even if there
             // are no next hops to one of them
-            ds = DestinationSet.createTypePushBos(segmentId1, destSw1, segmentId2, destSw2);
-            treatment = null;
+            dsIPv4 = DestinationSet.createTypePushBos(segmentIdIPv41, destSw1, segmentIdIPv42, destSw2);
+            dsIPv6 = DestinationSet.createTypePushBos(segmentIdIPv61, destSw1, segmentIdIPv62, destSw2);
         }
-
         // setup metadata to pass to nextObjective - indicate the vlan on egress
         // if needed by the switch pipeline. Since neighbor sets are always to
         // other neighboring routers, there is no subnet assigned on those ports.
-        TrafficSelector.Builder metabuilder = DefaultTrafficSelector.builder(selector);
-        metabuilder.matchVlanId(SegmentRoutingManager.INTERNAL_VLAN);
+        metaIpv4Selector = buildIpv4Selector()
+                .matchVlanId(SegmentRoutingManager.INTERNAL_VLAN)
+                .build();
+        metaIpv6Selector = buildIpv6Selector()
+                .matchVlanId(SegmentRoutingManager.INTERNAL_VLAN)
+                .build();
+        // get the group handler of the target switch
         DefaultGroupHandler grpHandler = srManager.getGroupHandler(targetSw);
         if (grpHandler == null) {
             log.warn("populateIPRuleForRouter: groupHandler for device {} "
-                    + "not found", targetSw);
+                             + "not found", targetSw);
             return false;
         }
-
-        int nextId = grpHandler.getNextObjectiveId(ds, nextHops,
-                                                   metabuilder.build(), false);
-        if (nextId <= 0) {
-            log.warn("No next objective in {} for ds: {}", targetSw, ds);
+        // get next id
+        nextIdIPv4 = grpHandler.getNextObjectiveId(dsIPv4, nextHops, metaIpv4Selector, false);
+        if (nextIdIPv4 <= 0) {
+            log.warn("No next objective in {} for ds: {}", targetSw, dsIPv4);
             return false;
         }
-
-        ForwardingObjective.Builder fwdBuilder = DefaultForwardingObjective
-                .builder()
-                .fromApp(srManager.appId)
-                .makePermanent()
-                .nextStep(nextId)
-                .withSelector(selector)
-                .withPriority(getPriorityFromPrefix(ipPrefix))
-                .withFlag(ForwardingObjective.Flag.SPECIFIC);
-        if (treatment != null) {
-            fwdBuilder.withTreatment(treatment);
+        nextIdIPv6 = grpHandler.getNextObjectiveId(dsIPv6, nextHops, metaIpv6Selector, false);
+        if (nextIdIPv6 <= 0) {
+            log.warn("No next objective in {} for ds: {}", targetSw, dsIPv6);
+            return false;
         }
-        log.debug("Installing IPv4 forwarding objective for router IP/subnet {} "
-                + "in switch {} with nextId: {}", ipPrefix, targetSw, nextId);
-        ObjectiveContext context = new DefaultObjectiveContext(
-                (objective) -> log.debug("IP rule for router {} populated in dev:{}",
-                                         ipPrefix, targetSw),
-                (objective, error) ->
-                        log.warn("Failed to populate IP rule for router {}: {} in dev:{}",
-                                 ipPrefix, error, targetSw));
-        srManager.flowObjectiveService.forward(targetSw, fwdBuilder.add(context));
-        rulePopulationCounter.incrementAndGet();
-
+        // build all the flow rules and send to the device
+        for (IpPrefix subnet : subnets) {
+            selector = buildIpSelectorFromIpPrefix(subnet).build();
+            if (subnet.isIp4()) {
+                nextId = nextIdIPv4;
+            } else {
+                nextId = nextIdIPv6;
+            }
+            ForwardingObjective.Builder fwdBuilder = DefaultForwardingObjective
+                    .builder()
+                    .fromApp(srManager.appId)
+                    .makePermanent()
+                    .nextStep(nextId)
+                    .withSelector(selector)
+                    .withPriority(getPriorityFromPrefix(subnet))
+                    .withFlag(ForwardingObjective.Flag.SPECIFIC);
+            if (treatment != null) {
+                fwdBuilder.withTreatment(treatment);
+            }
+            log.debug("Installing {} forwarding objective for router IP/subnet {} "
+                              + "in switch {} with nextId: {}", subnet.isIp4() ? "IPv4" : "IPv6",
+                      subnet, targetSw, nextId);
+            ObjectiveContext context = new DefaultObjectiveContext(
+                    (objective) -> log.debug("IP rule for router {} populated in dev:{}",
+                                             subnet, targetSw),
+                    (objective, error) -> log.warn("Failed to populate IP rule for router {}: {} in dev:{}",
+                                                   subnet, error, targetSw));
+            srManager.flowObjectiveService.forward(targetSw, fwdBuilder.add(context));
+        }
+        rulePopulationCounter.addAndGet(subnets.size());
         return true;
     }
 
@@ -1192,20 +1198,26 @@
         }
     }
 
-    /**
-     * Method to build IPv4 or IPv6 selector.
-     *
-     * @param addressToMatch the address to match
-     */
+    // Method for building an IPv4 selector
+    private TrafficSelector.Builder buildIpv4Selector() {
+        TrafficSelector.Builder selectorBuilder = DefaultTrafficSelector.builder();
+        selectorBuilder.matchEthType(Ethernet.TYPE_IPV4);
+        return selectorBuilder;
+    }
+
+    // Method for building an IPv6 selector
+    private TrafficSelector.Builder buildIpv6Selector() {
+        TrafficSelector.Builder selectorBuilder = DefaultTrafficSelector.builder();
+        selectorBuilder.matchEthType(Ethernet.TYPE_IPV6);
+        return selectorBuilder;
+    }
+
+    // Method for building an IPv4 or IPv6 selector from an IP address
     private TrafficSelector.Builder buildIpSelectorFromIpAddress(IpAddress addressToMatch) {
         return buildIpSelectorFromIpPrefix(addressToMatch.toIpPrefix());
     }
 
-    /**
-     * Method to build IPv4 or IPv6 selector.
-     *
-     * @param prefixToMatch the prefix to match
-     */
+    // Method for building an IPv4 or IPv6 selector from an IP prefix
     private TrafficSelector.Builder buildIpSelectorFromIpPrefix(IpPrefix prefixToMatch) {
         TrafficSelector.Builder selectorBuilder = DefaultTrafficSelector.builder();
         // If the prefix is IPv4
@@ -1582,4 +1594,5 @@
             (srManager.getInternalVlanId(cp) != null && srManager.getInternalVlanId(cp).equals(vlanId))
         );
     }
+
 }