Add named constants for instance_type to v1model.p4 ?

[jafingerhut]

The following numeric values are currently what behavioral-model's simple_switch uses for numeric values assigned to the instance_type field of standard_metadata. If people want to write P4_16 + v1model architecture programs that process resubmitted, recirculated, or cloned packets differently than normal packets, they will need to use these values.

Does it seem reasonable to add these to the file p4include/v1model.p4?

If so, does the name prefix seem good, or should it be something else? I believe the prefix should definitely include 'V1MODEL', since these values are specific to that architecture at least, and perhaps also to BMV2's simple_switch implementation.

const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_NORMAL        = 0;
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_INGRESS_CLONE = 1;
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_EGRESS_CLONE  = 2;
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_COALESCED     = 3;
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_RECIRC        = 4;
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_REPLICATION   = 5;
const bit<32> BMV2_V1MODEL_INSTANCE_TYPE_RESUBMIT      = 6;

[mihaibudiu]

Shouldn't these be enums?

[jafingerhut]

The instance_type field that one would want to compare against these values has type bit<32>, and I'm not sure it would be worth the effort to change its type to enum for v1model. For example, if these constants were enums and we changed the instance_type field to enum, auto-translating from P4_14 to P4_16+v1model would probably have some tricky cases, especially if someone was crazy enough to have a P4_14 program that did arithmetic on instance_type values.

The corresponding fields and constants in PSA are enums already.

[antoninbas]

This should definitely be an enum. The required changes in bmv2-simple_switch are very straightforward. The only issue is that changes have to be synchronized between the compiler and simple_switch, and that I don't see a convenient way to roll-out these changes in a backward-compatible way, but I can try anyway and have a PR ready for this in bmv2.

[antoninbas]

I have opened https://github.com/p4lang/behavioral-model/pull/656 Assuming that the following is added to v1model.p4:

enum InstanceType {
  Normal,
  IngressClone,
  EgressClone,
  Coaesced,
  Recirc,
  Replication,
  Resubmit
}

and assuming that the compiler backend does the right thing (I have tested at the JSON level, not at the P4 level) bmv2 should support it.

[antoninbas]

If I try the necessary changes to v1model.p4, I get the following compiler bug:

In file: /home/antonin/Documents/p4lang/p4c/backends/bmv2/common/header.cpp:226
Compiler Bug: /home/antonin/Documents/p4lang/p4c/build/backends/bmv2/p4include/v1model.p4(30): <Type_Enum>(153): unexpected type for struct standard_metadata @metadata @name("standard_metadata") {
  bit<9> ingress_port;
  bit<9> egress_spec;
  bit<9> egress_port;
  bit<32> clone_spec;
  <Type_Enum>(153) instance_type;
  bit<1> drop;
  bit<16> recirculate_port;
  bit<32> packet_length;
  @alias("queueing_metadata.enq_timestamp") bit<32> enq_timestamp;
  @alias("queueing_metadata.enq_qdepth") bit<19> enq_qdepth;
  @alias("queueing_metadata.deq_timedelta") bit<32> deq_timedelta;
  @alias("queueing_metadata.deq_qdepth") bit<19> deq_qdepth;
  @alias("intrinsic_metadata.ingress_global_timestamp") bit<48> ingress_global_timestamp;
  @alias("intrinsic_metadata.egress_global_timestamp") bit<48> egress_global_timestamp;
  @alias("intrinsic_metadata.lf_field_list") bit<32> lf_field_list;
  @alias("intrinsic_metadata.mcast_grp") bit<16> mcast_grp;
  @alias("intrinsic_metadata.resubmit_flag") bit<32> resubmit_flag;
  @alias("intrinsic_metadata.egress_rid") bit<16> egress_rid;
  bit<1> checksum_error;
  @alias("intrinsic_metadata.recirculate_flag") bit<32> recirculate_flag;
  error { NoError, PacketTooShort, NoMatch, StackOutOfBounds, HeaderTooShort, ParserTimeout } parser_error; }.instance_type
enum InstanceType {
     ^^^^^^^^^^^^
/home/antonin/Documents/p4lang/p4c/build/backends/bmv2/p4include/v1model.p4(42)
struct standard_metadata_t {
       ^^^^^^^^^^^^^^^^^^^
/home/antonin/Documents/p4lang/p4c/build/backends/bmv2/p4include/v1model.p4(47)
    InstanceType instance_type;
                 ^^^^^^^^^^^^^

[mihaibudiu]

I think this will be easy to fix.

[mihaibudiu]

@antoninbas : I think it works if you use an enum with explicit values:

enum bit<32> InstanceType {
  Normal = 0,
  IngressClone = 1,
  EgressClone = 2,
  Coaesced = 3,
  Recirc = 4,
  Replication = 5,
  Resubmit = 6
}

Would this be an adequate solution?

[jafingerhut]

For v1model.p4, that certainly seems reasonable to me (especially since I'm content if it isn't even an enum at all).

[antoninbas]

I don't think this is adequate, the whole point IMO was to not explicitly assign fixed integral values to these instance types. Is there any issue with using a safe enum, apart from the compiler bug which should probably be fixed anyway?

[mihaibudiu]

Whether you use constants or not depends on who else may have these values hardwired. If everyone uses the same json coming out of the compiler then you don't need to hardwire the values.

The compiler currently does not convert any enum defined in v1model.p4; the existing enums were only used symbolically, and they are never supposed to leak outside the program. We just have to change the enum conversion policy to actually convert this particular one to numbers. We also have to make a choice on the number of bits that we want to use; the default policy in the bmv2 back-end is to use 32 bits for all enums.

[antoninbas]

InstanceType is also supposed to only be used symbolically and only inside the P4 dataplane. My bmv2 patch implements that change (from hardwired constant values to symbols): https://github.com/p4lang/behavioral-model/pull/656/files#diff-fbe7c5fb21f612c9d07b34a7e4f60332R382 The fact that the bmv2 compiler backend has to transform every enum to a 32-bit integer has more to do with the fact that we need to support assignments for enum members. Once the compiler has verified the correctness of the enum usage in the program, it is irrelevant that enum operations in the bmv2 forwarding engine are implemented with integers.

[mihaibudiu]

I think I have a fix for this issue, but we have some complications:

• switch.p4 compares explicitly standard_metadata.instance_type with an integer
• P4-14 programs that use the @pragma parser_entry on a state already generate an enum called InstanceType in the translation from p4-14 to p4-16

In other words, making this change in v1model may break existing programs. We can change our test programs, but what about other users of the compiler?

[jafingerhut]

At the risk of sounding annoyingly like a broken record, I don't see a big advantage to pushing for an enum here. P4_14 didn't have them, P4_16+v1model is intended as an auto-translation target from P4_14 programs, so why try to figure out where they should be, and adjust expression involving them, when we could just leave them as the type they were born to be?

PSA had these values as an enum type from the start, but PSA isn't an auto-translation target for P4_14 programs.

[mihaibudiu]

True, we actually can't rewrite the p4-14 programs, since they have no enums, so this becomes very tricky.

[mihaibudiu]

If we use the enum bit<32> InstanceType we keep compatibility with P4-14 programs.

[jafingerhut]

It certainly sounds more straightforward for auto-translation from P4_14 to P4_16 by using the enum with backing type of bit<32>, vs. not using that P4_16 feature.

[antoninbas]

Since the translation from P4_14 is a valid concern, I believe it's better to go with Andy's original proposal and explicitly include "BMV2" in the constant name. The serializable enum doesn't buy you too much IMO. We could add a "policy" to the EliminateSerEnums midend pass to optionally let the backend replace the enum constants with the correct target-specific values (instead of using the default P4-provided values). bmv2 could use the default values but another target (e.g. Tofino?) may choose to remap the values. However that's a bit hacky and kind of goes against what a serializable enum is supposed to be.

[mihaibudiu]

so what is the status of this issue?

[jfingerh]

I can create a PR with my original proposal if there is interest. I don't think it is critical, but it wouldn't take much time.

I suspect P4_14 -> P4_16+v1model auto-translation is a gradually diminishing desire over time, and this issue was really just a minor enhancement to enable people using v1model to have another way to find out what these special numeric values are, and give them names.

[mihaibudiu]

either that or we can close this issue

[jafingerhut]

Closing this issue, since it seems that interest in v1model architecture is less now than it was when the issue was opened.