From 33f36a0ec7b00bfc2e4abc8603159d7f76112bd3 Mon Sep 17 00:00:00 2001 From: Carmelo Cascone Date: Wed, 17 Apr 2019 20:05:21 -0700 Subject: [PATCH] Clean up PipelineInterpreter API by removing ambiguous methods Such as mapping from PiMatchFieldId to Criterion.Type. This should not be required since the only translation happening is from north (Criterion.Type) to south (PiMatchFieldId). Change-Id: I204e0bd66b3996fd60bc11d4241e8a0408e11582 --- .../pipeconf/PipelineInterpreterImpl.java | 22 ++----- .../net/pi/model/PiPipelineInterpreter.java | 22 ------- .../net/pi/impl/PiFlowRuleTranslatorImpl.java | 60 ++++++++++++++----- .../pipelines/basic/BasicInterpreterImpl.java | 21 ++----- .../pipelines/fabric/FabricInterpreter.java | 31 ---------- 5 files changed, 56 insertions(+), 100 deletions(-) diff --git a/apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java b/apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java index fd3b504f57..828d623e44 100644 --- a/apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java +++ b/apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java @@ -16,9 +16,8 @@ package org.onosproject.p4tutorial.pipeconf; -import com.google.common.collect.BiMap; -import com.google.common.collect.ImmutableBiMap; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import org.onlab.packet.DeserializationException; import org.onlab.packet.Ethernet; @@ -50,6 +49,7 @@ import org.onosproject.net.pi.runtime.PiPacketOperation; import java.nio.ByteBuffer; import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.Optional; import static java.lang.String.format; @@ -98,13 +98,13 @@ public final class PipelineInterpreterImpl private static final PiActionParamId ACT_PARAM_ID_PORT = PiActionParamId.of("port"); - private static final BiMap TABLE_MAP = - new ImmutableBiMap.Builder() + private static final Map TABLE_MAP = + new ImmutableMap.Builder() .put(0, TABLE_L2_FWD_ID) .build(); - private static final BiMap CRITERION_MAP = - new ImmutableBiMap.Builder() + private static final Map CRITERION_MAP = + ImmutableMap.builder() .put(Criterion.Type.IN_PORT, INGRESS_PORT_ID) .put(Criterion.Type.ETH_DST, ETH_DST_ID) .put(Criterion.Type.ETH_SRC, ETH_SRC_ID) @@ -116,21 +116,11 @@ public final class PipelineInterpreterImpl return Optional.ofNullable(CRITERION_MAP.get(type)); } - @Override - public Optional mapPiMatchFieldId(PiMatchFieldId headerFieldId) { - return Optional.ofNullable(CRITERION_MAP.inverse().get(headerFieldId)); - } - @Override public Optional mapFlowRuleTableId(int flowRuleTableId) { return Optional.ofNullable(TABLE_MAP.get(flowRuleTableId)); } - @Override - public Optional mapPiTableId(PiTableId piTableId) { - return Optional.ofNullable(TABLE_MAP.inverse().get(piTableId)); - } - @Override public PiAction mapTreatment(TrafficTreatment treatment, PiTableId piTableId) throws PiInterpreterException { diff --git a/core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java b/core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java index 1d1301e4fa..1ba85e0335 100644 --- a/core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java +++ b/core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java @@ -46,16 +46,6 @@ public interface PiPipelineInterpreter extends HandlerBehaviour { */ Optional mapCriterionType(Criterion.Type type); - /** - * Returns the criterion type that is equivalent to the given PI match field - * ID, if present. If not present, it means that the given match field is - * not supported by this interpreter. - * - * @param fieldId match field ID - * @return optional criterion type - */ - Optional mapPiMatchFieldId(PiMatchFieldId fieldId); - /** * Returns a PI table ID equivalent to the given numeric table ID (as in * {@link org.onosproject.net.flow.FlowRule#tableId()}). If not present, it @@ -71,18 +61,6 @@ public interface PiPipelineInterpreter extends HandlerBehaviour { // specific PiTableId even when mapping to a single table. Optional mapFlowRuleTableId(int flowRuleTableId); - /** - * Returns an integer table ID equivalent to the given PI table ID. If not - * present, it means that the given PI table ID cannot be mapped to any - * integer table ID, because such mapping would be meaningless or because - * such PI table ID is not defined by the pipeline model. - * - * @param piTableId PI table ID - * @return numeric table ID - */ - // FIXME: as above - Optional mapPiTableId(PiTableId piTableId); - /** * Returns an action of a PI pipeline that is functionally equivalent to the * given traffic treatment for the given table. diff --git a/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java b/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java index e910116a07..561d62a80b 100644 --- a/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java +++ b/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java @@ -54,7 +54,6 @@ import org.slf4j.LoggerFactory; import java.util.Collection; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.StringJoiner; @@ -80,7 +79,8 @@ final class PiFlowRuleTranslatorImpl { } /** - * Returns a PI table entry equivalent to the given flow rule, for the given pipeconf and device. + * Returns a PI table entry equivalent to the given flow rule, for the given + * pipeconf and device. * * @param rule flow rule * @param pipeconf pipeconf @@ -148,7 +148,7 @@ final class PiFlowRuleTranslatorImpl { tableEntryBuilder.withTimeout((double) rule.timeout()); } else { log.debug("Flow rule is temporary, but table '{}' doesn't support " + - "aging, translating to permanent.", tableModel.id()); + "aging, translating to permanent.", tableModel.id()); } } @@ -158,16 +158,19 @@ final class PiFlowRuleTranslatorImpl { /** - * Returns a PI action equivalent to the given treatment, optionally using the given interpreter. This method also - * checks that the produced PI table action is suitable for the given table ID and pipeline model. If suitable, the - * returned action instance will have parameters well-sized, according to the table model. + * Returns a PI action equivalent to the given treatment, optionally using + * the given interpreter. This method also checks that the produced PI table + * action is suitable for the given table ID and pipeline model. If + * suitable, the returned action instance will have parameters well-sized, + * according to the table model. * * @param treatment traffic treatment * @param interpreter interpreter * @param tableId PI table ID * @param pipelineModel pipeline model * @return PI table action - * @throws PiTranslationException if the treatment cannot be translated or if the PI action is not suitable for the + * @throws PiTranslationException if the treatment cannot be translated or + * if the PI action is not suitable for the * given pipeline model */ static PiTableAction translateTreatment(TrafficTreatment treatment, PiPipelineInterpreter interpreter, @@ -185,7 +188,8 @@ final class PiFlowRuleTranslatorImpl { } /** - * Builds a PI action out of the given treatment, optionally using the given interpreter. + * Builds a PI action out of the given treatment, optionally using the given + * interpreter. */ private static PiTableAction buildAction(TrafficTreatment treatment, PiPipelineInterpreter interpreter, PiTableId tableId) @@ -285,8 +289,9 @@ final class PiFlowRuleTranslatorImpl { } /** - * Builds a collection of PI field matches out of the given selector, optionally using the given interpreter. The - * field matches returned are guaranteed to be compatible for the given table model. + * Builds a collection of PI field matches out of the given selector, + * optionally using the given interpreter. The field matches returned are + * guaranteed to be compatible for the given table model. */ private static Collection translateFieldMatches(PiPipelineInterpreter interpreter, TrafficSelector selector, PiTableModel tableModel) @@ -312,18 +317,41 @@ final class PiFlowRuleTranslatorImpl { Set usedPiCriterionFields = Sets.newHashSet(); Set ignoredPiCriterionFields = Sets.newHashSet(); + + Map criterionMap = Maps.newHashMap(); + if (interpreter != null) { + // NOTE: if two criterion types map to the same match field ID, and + // those two criterion types are present in the selector, this won't + // work. This is unlikely to happen since those cases should be + // mutually exclusive: + // e.g. ICMPV6_TYPE -> metadata.my_normalized_icmp_type + // ICMPV4_TYPE -> metadata.my_normalized_icmp_type + // A packet can be either ICMPv6 or ICMPv4 but not both. + selector.criteria() + .stream() + .map(Criterion::type) + .filter(t -> t != PROTOCOL_INDEPENDENT) + .forEach(t -> { + PiMatchFieldId mfid = interpreter.mapCriterionType(t) + .orElse(null); + if (mfid != null) { + if (criterionMap.containsKey(mfid)) { + log.warn("Detected criterion mapping " + + "conflict for PiMatchFieldId {}", + mfid); + } + criterionMap.put(mfid, selector.getCriterion(t)); + } + }); + } + for (PiMatchFieldModel fieldModel : tableModel.matchFields()) { PiMatchFieldId fieldId = fieldModel.id(); int bitWidth = fieldModel.bitWidth(); - Optional criterionType = - interpreter == null - ? Optional.empty() - : interpreter.mapPiMatchFieldId(fieldId); - - Criterion criterion = criterionType.map(selector::getCriterion).orElse(null); + Criterion criterion = criterionMap.get(fieldId); if (!piCriterionFields.containsKey(fieldId) && criterion == null) { // Neither a field in PiCriterion is available nor a Criterion mapping is possible. diff --git a/pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java b/pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java index fe3c22fdea..6e05a5b53a 100644 --- a/pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java +++ b/pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java @@ -16,8 +16,8 @@ package org.onosproject.pipelines.basic; -import com.google.common.collect.ImmutableBiMap; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import org.onlab.packet.DeserializationException; import org.onlab.packet.Ethernet; import org.onlab.util.ImmutableByteSequence; @@ -45,6 +45,7 @@ import org.onosproject.net.pi.runtime.PiPacketOperation; import java.nio.ByteBuffer; import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.Optional; import static java.lang.String.format; @@ -80,12 +81,12 @@ public class BasicInterpreterImpl extends AbstractHandlerBehaviour private static final int PORT_BITWIDTH = 9; - private static final ImmutableBiMap TABLE_MAP = - new ImmutableBiMap.Builder() + private static final Map TABLE_MAP = + new ImmutableMap.Builder() .put(0, BasicConstants.INGRESS_TABLE0_CONTROL_TABLE0) .build(); - private static final ImmutableBiMap CRITERION_MAP = - new ImmutableBiMap.Builder() + private static final Map CRITERION_MAP = + new ImmutableMap.Builder() .put(Criterion.Type.IN_PORT, HDR_STANDARD_METADATA_INGRESS_PORT) .put(Criterion.Type.ETH_DST, HDR_HDR_ETHERNET_DST_ADDR) .put(Criterion.Type.ETH_SRC, HDR_HDR_ETHERNET_SRC_ADDR) @@ -235,18 +236,8 @@ public class BasicInterpreterImpl extends AbstractHandlerBehaviour return Optional.ofNullable(CRITERION_MAP.get(type)); } - @Override - public Optional mapPiMatchFieldId(PiMatchFieldId headerFieldId) { - return Optional.ofNullable(CRITERION_MAP.inverse().get(headerFieldId)); - } - @Override public Optional mapFlowRuleTableId(int flowRuleTableId) { return Optional.ofNullable(TABLE_MAP.get(flowRuleTableId)); } - - @Override - public Optional mapPiTableId(PiTableId piTableId) { - return Optional.ofNullable(TABLE_MAP.inverse().get(piTableId)); - } } diff --git a/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java b/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java index e5234729c7..dcd872bc1a 100644 --- a/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java +++ b/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java @@ -99,25 +99,6 @@ public class FabricInterpreter extends AbstractFabricHandlerBehavior .put(Criterion.Type.ICMPV6_CODE, FabricConstants.HDR_ICMP_CODE) .build(); - private static final ImmutableMap INVERSE_CRITERION_MAP = - ImmutableMap.builder() - .put(FabricConstants.HDR_IG_PORT, Criterion.Type.IN_PORT) - .put(FabricConstants.HDR_ETH_DST, Criterion.Type.ETH_DST_MASKED) - .put(FabricConstants.HDR_ETH_SRC, Criterion.Type.ETH_SRC_MASKED) - .put(FabricConstants.HDR_ETH_TYPE, Criterion.Type.ETH_TYPE) - .put(FabricConstants.HDR_MPLS_LABEL, Criterion.Type.MPLS_LABEL) - .put(FabricConstants.HDR_VLAN_ID, Criterion.Type.VLAN_VID) - .put(FabricConstants.HDR_IPV4_DST, Criterion.Type.IPV4_DST) - .put(FabricConstants.HDR_IPV4_SRC, Criterion.Type.IPV4_SRC) - .put(FabricConstants.HDR_IPV6_DST, Criterion.Type.IPV6_DST) - // FIXME: might be incorrect if we inverse the map.... - .put(FabricConstants.HDR_L4_SPORT, Criterion.Type.UDP_SRC) - .put(FabricConstants.HDR_L4_DPORT, Criterion.Type.UDP_DST) - .put(FabricConstants.HDR_IP_PROTO, Criterion.Type.IP_PROTO) - .put(FabricConstants.HDR_ICMP_TYPE, Criterion.Type.ICMPV6_TYPE) - .put(FabricConstants.HDR_ICMP_CODE, Criterion.Type.ICMPV6_CODE) - .build(); - private static final PiAction NOP = PiAction.builder() .withId(FabricConstants.NOP).build(); @@ -131,11 +112,6 @@ public class FabricInterpreter extends AbstractFabricHandlerBehavior return Optional.ofNullable(CRITERION_MAP.get(type)); } - @Override - public Optional mapPiMatchFieldId(PiMatchFieldId fieldId) { - return Optional.ofNullable(INVERSE_CRITERION_MAP.get(fieldId)); - } - @Override public Optional mapFlowRuleTableId(int flowRuleTableId) { // The only use case for Index ID->PiTableId is when using the single @@ -143,13 +119,6 @@ public class FabricInterpreter extends AbstractFabricHandlerBehavior return Optional.empty(); } - @Override - public Optional mapPiTableId(PiTableId piTableId) { - // The only use case for Index ID->PiTableId is when using the single - // table pipeliner. fabric.p4 is never used with such pipeliner. - return Optional.empty(); - } - @Override public PiAction mapTreatment(TrafficTreatment treatment, PiTableId piTableId) throws PiInterpreterException {