No need to map table counters in PI pipeline interpreter

This is related to ONOS-7595. In a recent P4Runtime update, it has been
made explicit that tables can support at most 1 direct counter. Hence,
the pipeline interpreter no longer needs to provide a mapping between a
table and one of potentially many counters. If needed, such mapping can
be derived from the pipeline model (i.e. the p4info)

Change-Id: Ibdece52f35a4d187ab9dbeb90f5527b6285e9788

apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java

@@ -37,7 +37,6 @@ import org.onosproject.net.packet.OutboundPacket;
 import org.onosproject.net.pi.model.PiActionId;
 import org.onosproject.net.pi.model.PiActionParamId;
 import org.onosproject.net.pi.model.PiControlMetadataId;
-import org.onosproject.net.pi.model.PiCounterId;
 import org.onosproject.net.pi.model.PiMatchFieldId;
 import org.onosproject.net.pi.model.PiPipelineInterpreter;
 import org.onosproject.net.pi.model.PiTableId;
@@ -136,11 +135,6 @@ public final class PipelineInterpreterImpl extends AbstractHandlerBehaviour impl
         }
     }
 
-    @Override
-    public Optional<PiCounterId> mapTableCounter(PiTableId piTableId) {
-        return Optional.empty();
-    }
-
     @Override
     public Collection<PiPacketOperation> mapOutboundPacket(OutboundPacket packet)
             throws PiInterpreterException {

core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java

@@ -84,17 +84,6 @@ public interface PiPipelineInterpreter extends HandlerBehaviour {
     PiAction mapTreatment(TrafficTreatment treatment, PiTableId piTableId)
             throws PiInterpreterException;
 
-    /**
-     * Returns a PI direct counter ID for the given table to be used to to compute flow entry statistics, if present. If
-     * not present, it means that the given table does not offer any counter suitable for the purpose of computing flow
-     * rule statistics. Other direct counters might be defined for the given table (check pipeline model), however none
-     * of them should be used for flow entry statistics except for this one.
-     *
-     * @param piTableId table ID
-     * @return optional direct counter ID
-     */
-    Optional<PiCounterId> mapTableCounter(PiTableId piTableId);
-
     /**
      * Returns a collection of PI packet operations equivalent to the given outbound packet instance.
      *

drivers/p4runtime/src/main/java/org/onosproject/drivers/p4runtime/P4RuntimeFlowRuleProgrammable.java

@@ -29,7 +29,6 @@ import org.onosproject.net.flow.DefaultFlowEntry;
 import org.onosproject.net.flow.FlowEntry;
 import org.onosproject.net.flow.FlowRule;
 import org.onosproject.net.flow.FlowRuleProgrammable;
-import org.onosproject.net.pi.model.PiCounterId;
 import org.onosproject.net.pi.model.PiPipelineInterpreter;
 import org.onosproject.net.pi.model.PiPipelineModel;
 import org.onosproject.net.pi.model.PiTableId;
@@ -87,6 +86,11 @@ public class P4RuntimeFlowRuleProgrammable
     private static final String READ_FROM_MIRROR = "tableReadFromMirror";
     private static final boolean DEFAULT_READ_FROM_MIRROR = false;
 
+    // If true, we read counters when reading table entries (if table has
+    // counters). Otherwise, we don't.
+    private static final String SUPPORT_TABLE_COUNTERS = "supportTableCounters";
+    private static final boolean DEFAULT_SUPPORT_TABLE_COUNTERS = true;
+
     // If true, we read all direct counters of a table with one request.
     // Otherwise, we send as many requests as the number of table entries.
     private static final String READ_ALL_DIRECT_COUNTERS = "tableReadAllDirectCounters";
@@ -166,13 +170,8 @@ public class P4RuntimeFlowRuleProgrammable
             }
 
             // Read table direct counters (if any).
-            final Map<PiTableEntry, PiCounterCellData> counterCellMap;
-            if (interpreter.mapTableCounter(piTableId).isPresent()) {
-                PiCounterId piCounterId = interpreter.mapTableCounter(piTableId).get();
-                counterCellMap = readEntryCounters(piCounterId, installedEntries);
-            } else {
-                counterCellMap = Collections.emptyMap();
-            }
+            final Map<PiTableEntry, PiCounterCellData> counterCellMap =
+                    readEntryCounters(installedEntries);
 
             // Forge flow entries with counter values.
             for (PiTableEntry installedEntry : installedEntries) {
@@ -391,7 +390,12 @@ public class P4RuntimeFlowRuleProgrammable
     }
 
     private Map<PiTableEntry, PiCounterCellData> readEntryCounters(
-            PiCounterId counterId, Collection<PiTableEntry> tableEntries) {
+            Collection<PiTableEntry> tableEntries) {
+        if (!driverBoolProperty(SUPPORT_TABLE_COUNTERS,
+                                DEFAULT_SUPPORT_TABLE_COUNTERS)) {
+            return Collections.emptyMap();
+        }
+
         Collection<PiCounterCellData> cellDatas;
         try {
             if (driverBoolProperty(READ_ALL_DIRECT_COUNTERS,
@@ -403,6 +407,7 @@ public class P4RuntimeFlowRuleProgrammable
                 cellDatas = Collections.emptyList();
             } else {
                 Set<PiCounterCellId> cellIds = tableEntries.stream()
+                        .filter(e -> tableHasCounter(e.table()))
                         .map(PiCounterCellId::ofDirect)
                         .collect(Collectors.toSet());
                 cellDatas = client.readCounterCells(cellIds, pipeconf).get();
@@ -412,13 +417,18 @@ public class P4RuntimeFlowRuleProgrammable
         } catch (InterruptedException | ExecutionException e) {
             if (!(e.getCause() instanceof StatusRuntimeException)) {
                 // gRPC errors are logged in the client.
-                log.error("Exception while reading counter '{}' from {}: {}",
-                          counterId, deviceId, e);
+                log.error("Exception while reading table counters from {}: {}",
+                          deviceId, e);
             }
             return Collections.emptyMap();
         }
     }
 
+    private boolean tableHasCounter(PiTableId tableId) {
+        return pipelineModel.table(tableId).isPresent() &&
+                !pipelineModel.table(tableId).get().counters().isEmpty();
+    }
+
     enum Operation {
         APPLY, REMOVE
     }

pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java

@@ -32,7 +32,6 @@ import org.onosproject.net.flow.instructions.Instruction;
 import org.onosproject.net.packet.DefaultInboundPacket;
 import org.onosproject.net.packet.InboundPacket;
 import org.onosproject.net.packet.OutboundPacket;
-import org.onosproject.net.pi.model.PiCounterId;
 import org.onosproject.net.pi.model.PiMatchFieldId;
 import org.onosproject.net.pi.model.PiPipelineInterpreter;
 import org.onosproject.net.pi.model.PiTableId;
@@ -59,8 +58,6 @@ import static org.onosproject.pipelines.basic.BasicConstants.ACT_NOACTION_ID;
 import static org.onosproject.pipelines.basic.BasicConstants.ACT_PRM_PORT_ID;
 import static org.onosproject.pipelines.basic.BasicConstants.ACT_SEND_TO_CPU_ID;
 import static org.onosproject.pipelines.basic.BasicConstants.ACT_SET_EGRESS_PORT_ID;
-import static org.onosproject.pipelines.basic.BasicConstants.CNT_TABLE0_ID;
-import static org.onosproject.pipelines.basic.BasicConstants.CNT_WCMP_TABLE_ID;
 import static org.onosproject.pipelines.basic.BasicConstants.HDR_ETH_DST_ID;
 import static org.onosproject.pipelines.basic.BasicConstants.HDR_ETH_SRC_ID;
 import static org.onosproject.pipelines.basic.BasicConstants.HDR_ETH_TYPE_ID;
@@ -71,7 +68,6 @@ import static org.onosproject.pipelines.basic.BasicConstants.PKT_META_EGRESS_POR
 import static org.onosproject.pipelines.basic.BasicConstants.PKT_META_INGRESS_PORT_ID;
 import static org.onosproject.pipelines.basic.BasicConstants.PORT_BITWIDTH;
 import static org.onosproject.pipelines.basic.BasicConstants.TBL_TABLE0_ID;
-import static org.onosproject.pipelines.basic.BasicConstants.TBL_WCMP_TABLE_ID;
 
 /**
  * Interpreter implementation for basic.p4.
@@ -83,11 +79,7 @@ public class BasicInterpreterImpl extends AbstractHandlerBehaviour
             new ImmutableBiMap.Builder<Integer, PiTableId>()
                     .put(0, TBL_TABLE0_ID)
                     .build();
-    private static final ImmutableBiMap<PiTableId, PiCounterId> TABLE_COUNTER_MAP =
-            new ImmutableBiMap.Builder<PiTableId, PiCounterId>()
-                    .put(TBL_TABLE0_ID, CNT_TABLE0_ID)
-                    .put(TBL_WCMP_TABLE_ID, CNT_WCMP_TABLE_ID)
-                    .build();
+
     private static final ImmutableBiMap<Criterion.Type, PiMatchFieldId> CRITERION_MAP =
             new ImmutableBiMap.Builder<Criterion.Type, PiMatchFieldId>()
                     .put(Criterion.Type.IN_PORT, HDR_IN_PORT_ID)
@@ -142,11 +134,6 @@ public class BasicInterpreterImpl extends AbstractHandlerBehaviour
         }
     }
 
-    @Override
-    public Optional<PiCounterId> mapTableCounter(PiTableId piTableId) {
-        return Optional.ofNullable(TABLE_COUNTER_MAP.get(piTableId));
-    }
-
     @Override
     public Collection<PiPacketOperation> mapOutboundPacket(OutboundPacket packet)
             throws PiInterpreterException {

pipelines/basic/src/main/java/org/onosproject/pipelines/basic/IntInterpreterImpl.java

@@ -1,48 +0,0 @@
-/*
- * Copyright 2017-present Open Networking Foundation
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.onosproject.pipelines.basic;
-
-import com.google.common.collect.ImmutableBiMap;
-import org.onosproject.net.pi.model.PiCounterId;
-import org.onosproject.net.pi.model.PiTableId;
-
-import java.util.Optional;
-
-import static org.onosproject.pipelines.basic.BasicConstants.CNT_TABLE0_ID;
-import static org.onosproject.pipelines.basic.BasicConstants.TBL_TABLE0_ID;
-import static org.onosproject.pipelines.basic.IntConstants.*;
-
-/**
- * Interpreter implementation for INT pipeline.
- */
-public class IntInterpreterImpl extends BasicInterpreterImpl {
-
-    private static final ImmutableBiMap<PiTableId, PiCounterId> TABLE_COUNTER_MAP =
-            new ImmutableBiMap.Builder<PiTableId, PiCounterId>()
-                    .put(TBL_TABLE0_ID, CNT_TABLE0_ID)
-                    .put(TBL_SET_SOURCE_SINK_ID, CNT_SET_SOURCE_SINK_ID)
-                    .put(TBL_INT_SOURCE_ID, CNT_INT_SOURCE_ID)
-                    .put(TBL_INT_INSERT_ID, CNT_INT_INSERT_ID)
-                    .put(TBL_INT_INST_0003_ID, CNT_INT_INST_0003_ID)
-                    .put(TBL_INT_INST_0407_ID, CNT_INT_INST_0407_ID)
-                    .build();
-
-    @Override
-    public Optional<PiCounterId> mapTableCounter(PiTableId piTableId) {
-        return Optional.ofNullable(TABLE_COUNTER_MAP.get(piTableId));
-    }
-}

pipelines/basic/src/main/java/org/onosproject/pipelines/basic/PipeconfLoader.java

@@ -100,7 +100,7 @@ public final class PipeconfLoader {
         return DefaultPiPipeconf.builder()
                 .withId(INT_PIPECONF_ID)
                 .withPipelineModel(parseP4Info(p4InfoUrl))
-                .addBehaviour(PiPipelineInterpreter.class, IntInterpreterImpl.class)
+                .addBehaviour(PiPipelineInterpreter.class, BasicInterpreterImpl.class)
                 .addBehaviour(Pipeliner.class, DefaultSingleTablePipeline.class)
                 .addBehaviour(PortStatisticsDiscovery.class, PortStatisticsDiscoveryImpl.class)
                 .addExtension(P4_INFO_TEXT, p4InfoUrl)

pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java

@@ -29,14 +29,12 @@ import org.onosproject.net.Port;
 import org.onosproject.net.PortNumber;
 import org.onosproject.net.device.DeviceService;
 import org.onosproject.net.driver.AbstractHandlerBehaviour;
-import org.onosproject.net.driver.Driver;
 import org.onosproject.net.flow.TrafficTreatment;
 import org.onosproject.net.flow.criteria.Criterion;
 import org.onosproject.net.flow.instructions.Instructions;
 import org.onosproject.net.packet.DefaultInboundPacket;
 import org.onosproject.net.packet.InboundPacket;
 import org.onosproject.net.packet.OutboundPacket;
-import org.onosproject.net.pi.model.PiCounterId;
 import org.onosproject.net.pi.model.PiMatchFieldId;
 import org.onosproject.net.pi.model.PiPipelineInterpreter;
 import org.onosproject.net.pi.model.PiTableId;
@@ -133,18 +131,6 @@ public class FabricInterpreter extends AbstractHandlerBehaviour
                     .put(FabricConstants.HF_ICMP_ICMP_CODE_ID, Criterion.Type.ICMPV6_CODE)
                     .build();
 
-    private static final ImmutableBiMap<PiTableId, PiCounterId> TABLE_COUNTER_MAP =
-            ImmutableBiMap.<PiTableId, PiCounterId>builder()
-                    .put(FabricConstants.TBL_FWD_CLASSIFIER_ID, FabricConstants.CNT_FWD_CLASSIFIER_COUNTER_ID)
-                    .put(FabricConstants.TBL_HASHED_ID, FabricConstants.CNT_HASHED_COUNTER_ID)
-                    .put(FabricConstants.TBL_INGRESS_PORT_VLAN_ID, FabricConstants.CNT_INGRESS_PORT_VLAN_COUNTER_ID)
-                    .put(FabricConstants.TBL_SIMPLE_ID, FabricConstants.CNT_SIMPLE_COUNTER_ID)
-                    .put(FabricConstants.TBL_BRIDGING_ID, FabricConstants.CNT_BRIDGING_COUNTER_ID)
-                    .put(FabricConstants.TBL_UNICAST_V4_ID, FabricConstants.CNT_UNICAST_V4_COUNTER_ID)
-                    .put(FabricConstants.TBL_MPLS_ID, FabricConstants.CNT_MPLS_COUNTER_ID)
-                    .build();
-    private static final String SUPPORT_TABLE_COUNTERS_PROP = "supportTableCounters";
-
     @Override
     public Optional<PiMatchFieldId> mapCriterionType(Criterion.Type type) {
         return Optional.ofNullable(CRITERION_MAP.get(type));
@@ -180,18 +166,6 @@ public class FabricInterpreter extends AbstractHandlerBehaviour
         }
     }
 
-    @Override
-    public Optional<PiCounterId> mapTableCounter(PiTableId piTableId) {
-        Driver driver = handler().driver();
-        boolean supportTableCounters = Boolean.parseBoolean(driver.getProperty(SUPPORT_TABLE_COUNTERS_PROP));
-
-        if (supportTableCounters) {
-            return Optional.ofNullable(TABLE_COUNTER_MAP.get(piTableId));
-        } else {
-            return Optional.empty();
-        }
-    }
-
     private PiPacketOperation createPiPacketOperation(DeviceId deviceId, ByteBuffer data, long portNumber)
             throws PiInterpreterException {
         PiControlMetadata metadata = createPacketMetadata(portNumber);