Binding action args to serializable enums

[thantry]

It appears that compiler does not support binding of action args to simple enum types. This helps with readability.

Ask is to support accepting serializable enum types as action arguments.

enum bit<5> TableType {
  TT_ACL = 0,
  TT_FWD = 1
};

action A(TableType t) {
  <some_metadata> = t;
}

table X {
   key = {
      meta.m: ternary;
  }
  actions = {
    A;
  }
  const entries = {
    (0x0): A(TableType.TT_ACL);
    (0x1): A(TableType.TT_FWD);
    (_): A(TableType.TT_FWD);
  }
}

[jnfoster]

It's always good to open an issue with a complete example... otherwise people have to fill in the details which is more work and creates possibility of them doing it wrong.

Anyway, here's an attempt. The code works fine (and can be processed using p4test), but if you add the cast in action A. Note that I had to change the key on the final const entry -- no parens are needed.

#include <core.p4>

enum bit<5> TableType {
    TT_ACL = 0,
    TT_FWD = 1
};

control C() {
    bit<5> x; 
    bit<4> y;

    action A(TableType t) {
        x = (bit<5>) t;
    }

    table X {
        key = { y : ternary; }
        actions = { A; }
        const entries = {
            (0x0): A(TableType.TT_ACL);
            (0x1): A(TableType.TT_FWD);
            _: A(TableType.TT_FWD);

        }
    }
    apply {
        X.apply();
    }
}

Is your request to make the cast implicit instead of explicit?

[vgurevich]

It might be an issue with p4info generation -- the language itself allows it just fine. But, let's wait for the full example from @thantry

[jnfoster]

Ahh, good point.

[thantry]

``

#include "core.p4"
#include "v1model.p4"
enum bit<5> TableType {
     TT_ACL = 0, 
     TT_FWD = 1
};

struct headers_t {};
struct local_metadata_t {};

control egress(inout headers_t headers, inout local_metadata_t local_metadata,
               inout standard_metadata_t standard_metadata) {
  apply {}
}
control compute_ck(inout headers_t headers,
                   inout local_metadata_t local_metadata) {
  apply {}
}
control verify_ck(inout headers_t headers,
                  inout local_metadata_t local_metadata) {
  apply {}
}

control deparser(packet_out packet, in headers_t headers) {
  apply {}
}

control ingress(inout headers_t headers, inout local_metadata_t local_metadata,
                inout standard_metadata_t standard_metadata) {
  TableType p;
  action A(TableType t) { p = t; }
  bit<4> y;
  table X {
    key = { y : ternary;
  }
  actions = { A;
}
const entries = {(0x0) : A(TableType.TT_ACL);
(0x1) : A(TableType.TT_FWD);
_ : A(TableType.TT_FWD);
}
}
apply { X.apply(); }
}

parser packet_parser(packet_in pkt, out headers_t hdrs,
                     inout local_metadata_t local_meta,
                     inout standard_metadata_t standard_meta) {
  state start { transition accept; }
}
V1Switch(packet_parser(), verify_ck(), ingress(), egress(), compute_ck(),
         deparser()) main;

~

The code above (sorry for the formatting, we use a different IDE) fails compile with test.p4(37): [--Werror=unsupported] error: TableType.TT_ACL unsupported argument expression const entries = {(0x0) : A(TableType.TT_ACL);

[thantry]

Might it have something to do with the backend? https://github.com/p4lang/p4c/blob/main/backends/bmv2/README.md#unsupported-p4_16-language-features

[mihaibudiu]

Tell us which backend you are using.

[thantry]

bmv2

[vgurevich]

It looks to me that when the compiler sees const entries, it tries to generate some information in p4info (or some other format, depending on the backend) and that is something that is not supported.

[thantry]

I can understand why p4info code generation might be difficult for composite types, but for a simple enum, it should not be as difficult?

[vgurevich]

Meanwhile, it looks like we might have another (minor) bug, either in the compiler or in the spec: empty const entries, i.e.:

const entries = {}

are not allowed :)

[mihaibudiu]

What good is a table with no entries?

[vgurevich]

@mbudiu-vmw -- I think it is good for a couple of reasons:

1. Empty tables are generally allowed
2. In the process of conditional compilation, the entries can be removed and it might happen that all of them will be. It should still be supported
3. Where would we be today if not for zero and an empty set?
4. I think accepting this kind of input is a matter of consistency, robustness and good taste
5. For the same reason we have empty statements, empty blocks, zero-length arrays, etc. in other languages.

[mihaibudiu]

Sure we can accept this, but warn about it and remove it from the flow. If will never have any effect at runtime.

[vgurevich]

Yes, in most cases such a table can be removed, although I'd be careful, e.g., in a case it has some meaningful default action :)

BTW, it's interesting how we specify that the entries are constant, but I do not think we mention whether we mandate the default action to be constant in such tables too :) Because if not, people might still want to have the table, just so that they can change the non-constant default action.

[thantry]

There are use cases where we'd want a table to have pre-populated entries AND insert entries at runtime to alter certain runtime behavior. Is such a behavior allowed? Or is it the case that any table with pre-populated entries cannot be programmed through the P4Runtime?

[mihaibudiu]

At this point the language spec only allows const entries to be specified. If the entries can change they must be specified all dynamically. This is because the compiler cannot properly assign priorities if entries will change dynamically.

[thantry]

I would imagine that the const entries are simply generated and assigned in the order in which they appear, and form part of the config blob. The runtime would need to deal with management of these entries, and that would be no different whether these were inserted at config time, or at runtime prior to packet reception. So, in that sense, supporting this feature should not break anything at the language level. One advantage of following such an approach is that it helps with default entry programming as part of the configuration process. And with certain backend structures (e.g. TCAMs), helps to quickly resolve blackouts (e.g. reprogramming a single TCAM entry to do some sort of catch-all behavior).

[vgurevich]

@thantry -- imagine that you have a ternary table, where each entry is supposed to have a priority. Indeed, the spec defines that the priority of individual entries is the same as the program order, that's fine and that works. Now, imagine that your program contains the entries attribute with 2 elements:

table t {
    key = { k : ternary; }
    actions = { a; }
    size = 100;
    entries = {
        0x1010 &&& 0x1010 : a(1);
        0x0101 &&& 0x0101 : a(2);
   }
}

And now a couple of questions:

1. What specific value of $MATCH_PRIORITY should the control plane specify in order to place a new entry in front of a(1)?
2. What specific value $MATCH_PRIORITY should the control plane specify in order to place a new entry between a(1) and a(2)?
3. What specific value $MATCH_PRIORITY should the control plane specify in order to place a new entry after a(2)?

Can you answer those question with specific numbers? Because if not, then you might not be able to add new entries to this table in a meaningful way.

[mihaibudiu]

i have filed https://github.com/p4lang/p4c/issues/2905 to track tables with no entries.

[thantry]

@thantry -- imagine that you have a ternary table, where each entry is supposed to have a priority. Indeed, the spec defines that the priority of individual entries is the same as the program order, that's fine and that works. Now, imagine that your program contains the entries attribute with 2 elements:

table t {
    key = { k : ternary; }
    actions = { a; }
    size = 100;
    entries = {
        0x1010 &&& 0x1010 : a(1);
        0x0101 &&& 0x0101 : a(2);
   }
}

And now a couple of questions:

1. What specific value of $MATCH_PRIORITY should the control plane specify in order to place a new entry in front of a(1)?
2. What specific value $MATCH_PRIORITY should the control plane specify in order to place a new entry between a(1) and a(2)?
3. What specific value $MATCH_PRIORITY should the control plane specify in order to place a new entry after a(2)?

Can you answer those question with specific numbers? Because if not, then you might not be able to add new entries to this table in a meaningful way.

The same issue exists when inserting entries into a partially filled P4Runtime visible TCAM table, where these two entries were added in a specific sequence. The runtime is responsible for moving entries around based on the TCAM implementation to guarantee a particular priority order. What I'm concerned about is the language preventing expressiveness based on a presumed usage pattern. Indeed, it should be possible for the runtime to iterate over entries in a P4 table and determine the location chosen by the configuration, and move them around should it feel the need to insert entries in a certain order (this is quite standard for most TCAM type tables where there is some table management entity for space/priority management).

I feel this is a very useful feature to have, and I see its use quite clearly for some of the P4 pipelines in the SmartNIC space.

[thantry]

It looks to me that when the compiler sees const entries, it tries to generate some information in p4info (or some other format, depending on the backend) and that is something that is not supported.

Is this something that could be fixed, at least for simple enum types?

[mihaibudiu]

Only if the p4info format actually supports serializable enums for this field.

[vgurevich]

@thantry -- there is a big difference between this case and a partially filled table. In the partially filled table, you (the control plane) know the priorities of all the entries, because they were all added by you (the control plane) at some point before that.

In the case where these entries have been declared in the P4 program, you do not know which absolute priorities were assigned to them and there is no way to express them in the program itself. Therefore, you can't choose the priorities for the new entries.

I understand the desire, and we had this desire as well, believe me, but there is a reason why we decided to go this way in the spec. Before we figured out that we'll require const it looked like this feature would not make it in the language at all.

If you have any specific proposals to improve the situation, you are more than welcome to submit a proposal. You now even know at least some the questions you will be asked during the review :)

[thantry]

Personally, I feel this goes back to the implementation of how the static entries are actually inserted. Given that P4Runtime supports SetPipelineConfig, one could have an implementation where the P4Runtime stack on the endpoint extracts these entries and pushes them down (control plane equivalent) or have a bulk API interface that requires metadata to be passed back to the on-host P4Runtime agent about the exact locations that these entries were inserted.

I think a formal proposal would be a good idea.

[vgurevich]

@thantry -- this is pretty much how this is implemented. The point is that the specific implementation is free to choose any values of $MATCH_PRIORITY when the entries are inserted as long as their relative values match the program order. However, we do not know what they will be and therefore whether they will be appropriate for further modifications.

For example, if it chooses $MATCH_PRIORITY to be 0 for the first entry, you will not be able to insert anything in front. If the compiler chooses $MATCH_PRIORITY for the first entry to be 10, you will be able to insert some entries in front, but maybe no more than 10 9assuming you want them to have different priorities. If the compiler chooses the priorities of the first and the second entry to be consecutive, then you won't be able to insert an entry in-between and so on. Note, that this has nothing to do with the physical positioning of these entries in the TCAM.

[lzhzero]

``

#include "core.p4"
#include "v1model.p4"
enum bit<5> TableType {
     TT_ACL = 0, 
     TT_FWD = 1
};

struct headers_t {};
struct local_metadata_t {};

control egress(inout headers_t headers, inout local_metadata_t local_metadata,
               inout standard_metadata_t standard_metadata) {
  apply {}
}
control compute_ck(inout headers_t headers,
                   inout local_metadata_t local_metadata) {
  apply {}
}
control verify_ck(inout headers_t headers,
                  inout local_metadata_t local_metadata) {
  apply {}
}

control deparser(packet_out packet, in headers_t headers) {
  apply {}
}

control ingress(inout headers_t headers, inout local_metadata_t local_metadata,
                inout standard_metadata_t standard_metadata) {
  TableType p;
  action A(TableType t) { p = t; }
  bit<4> y;
  table X {
    key = { y : ternary;
  }
  actions = { A;
}
const entries = {(0x0) : A(TableType.TT_ACL);
(0x1) : A(TableType.TT_FWD);
_ : A(TableType.TT_FWD);
}
}
apply { X.apply(); }
}

parser packet_parser(packet_in pkt, out headers_t hdrs,
                     inout local_metadata_t local_meta,
                     inout standard_metadata_t standard_meta) {
  state start { transition accept; }
}
V1Switch(packet_parser(), verify_ck(), ingress(), egress(), compute_ck(),
         deparser()) main;

~

The code above (sorry for the formatting, we use a different IDE) fails compile with test.p4(37): [--Werror=unsupported] error: TableType.TT_ACL unsupported argument expression const entries = {(0x0) : A(TableType.TT_ACL);

When I use p4c or p4c-bm2-ss to compile this example, it works ok. But when i use bazel build, it fails with the following error. Is this a bug or just by design?

 /bin/bash -c '
            # p4c invokes cc for preprocessing; we provide it below.
            function cc () { "/usr/bin/gcc" "$@"; }
            export -f cc

            "bazel-out/k8-fastbuild/bin/external/com_github_p4lang_p4c/p4c_bmv2" bug/test_enum.p4 --std p4-16 --arch v1model -Iexternal/com_github_p4lang_p4c/p4include -I. --target bmv2 --p4runtime-files bazel-out/k8-fastbuild/bin/bug/test_enum.p4info.txt

        ')
Execution platform: @local_config_platform//:host

Use --sandbox_debug to see verbose messages from the sandbox
bug/test_enum.p4(34): [--Wwarn=uninitialized_use] warning: y may be uninitialized
    key = { y : ternary;
            ^
bug/test_enum.p4(38): [--Werror=unsupported] error: TableType.TT_ACL unsupported argument expression
const entries = {(0x0) : A(TableType.TT_ACL);
                         ^^^^^^^^^^^^^^^^

[jfingerh]

Hari described several of the ideas proposed in earlier comments to me in a PNA meeting in November 2021, and I wrote up a proposal in mid-December that can be found here, with ideas for: (a) a definition for entries table property without const, (b) for tables requiring priorities, how to specify them in the P4 source code, so that the control plane would know what those priorities are when it later inserts new entries, (c) the idea of possibly limiting the range of relative priority values for a table, which could simplify and/or speed up modifications to such tables, and (d) the idea of a table with a guaranteed minimum number of entries it can support.

Please read and comment on the Google doc itself if you have questions, comments, corrections, etc.: https://docs.google.com/document/d/1oGlOpgIgUHxY4ytij1uwBrbwWdCbuyn5joFYH--F0M8

[jafingerhut]

@lzhzero I get some of those same error messages when running p4c built without using Bazel, when I invoke it with command line options that try to generate a P4Info file, e.g. --p4untime-files my.p4info.txt.

p4test only checks syntax and some semantics of the source P4 program, but does not generate P4Info file with the command line arguments shown below, so only a warning, and no error:

$ p4test --version
p4test
Version 1.2.2 (SHA: ce9d7df32 BUILD: DEBUG)

 p4test p4c-issue-2904-2.p4 
p4c-issue-2904-2.p4(37): [--Wwarn=uninitialized_use] warning: y may be uninitialized
        key = { y : ternary; }

With the command line options below, p4c generates a .p4i file output by the C preprocessor and BMv2 JSON file, but no P4Info file, so it is most likely not running the part of p4c that generates a P4Info file at all. Only warning messages are produced, no errors:

$ p4c --version
p4c 1.2.2 (SHA: ce9d7df32 BUILD: DEBUG)

$ p4c --target bmv2 --arch v1model p4c-issue-2904-2.p4 
p4c-issue-2904-2.p4(37): [--Wwarn=uninitialized_use] warning: y may be uninitialized
        key = { y : ternary; }
                ^
p4c-issue-2904-2.p4(34): [--Wwarn=unused] warning: Unused action parameter t
    action A(TableType t) { p = t; }
                       ^

$ ls -lrt
total 48
 4 -rwxrwx--- 1 root vboxsf  1381 Dec 29 14:04 p4c-issue-2904-2.p4*
36 -rwxrwx--- 1 root vboxsf 35051 Dec 30 01:25 p4c-issue-2904-2.p4i*
 8 -rwxrwx--- 1 root vboxsf  7444 Dec 30 01:25 p4c-issue-2904-2.json*

With the command line options below, p4c does generate a P4Info file, and does issue error messages. Thus it is very likely that these error messages are from the part of p4c that generates the P4Info file:

$ rm *.p4i *.p4info.txt *.json

$ p4c --target bmv2 --arch v1model --p4runtime-files my.p4info.txt p4c-issue-2904-2.p4 
p4c-issue-2904-2.p4(37): [--Wwarn=uninitialized_use] warning: y may be uninitialized
        key = { y : ternary; }
                ^
p4c-issue-2904-2.p4(40): [--Werror=unsupported] error: TableType.TT_ACL unsupported argument expression
            (0x0) : A(TableType.TT_ACL);
                      ^^^^^^^^^^^^^^^^
p4c-issue-2904-2.p4(41): [--Werror=unsupported] error: TableType.TT_FWD unsupported argument expression
            (0x1) : A(TableType.TT_FWD);
                      ^^^^^^^^^^^^^^^^
p4c-issue-2904-2.p4(42): [--Werror=unsupported] error: TableType.TT_FWD unsupported argument expression
            _ : A(TableType.TT_FWD);
                  ^^^^^^^^^^^^^^^^

$ ls -lrt
total 44
 4 -rwxrwx--- 1 root vboxsf  1381 Dec 29 14:04 p4c-issue-2904-2.p4*
36 -rwxrwx--- 1 root vboxsf 35051 Dec 30 01:26 p4c-issue-2904-2.p4i*
 4 -rwxrwx--- 1 root vboxsf   900 Dec 30 01:26 my.p4info.txt*

There is only one line of code in all of the p4c repo that contains "unsupported argument expression", and it is here: https://github.com/p4lang/p4c/blob/40ecc63288304db66c2d81a6b008390474a20bac/control-plane/p4RuntimeSerializer.cpp#L1550

Very likely among the cases handled for some compiler IR node shortly before that line, there is an additional if expression that could be added to support constants with a serializable enum type.

[mihaibudiu]

If these constants are represented like other numeric constants, it should be easy to support this. Is this what the P4Runtime spec expects?

[jfingerh]

For serializable enums, the P4Runtime spec says that these values must be represented as numbers, not as names (the name<->number mapping is also available to the controller in the P4Info file, if I recall correctly). Reference: https://p4.org/p4-spec/p4runtime/v1.3.0/P4Runtime-Spec.html#sec-enum-serializable-enum-and-error