Control-plane name clash introduced by LocalizeAllActions and RemoveActionParameters

[jnfoster]

The following program compiles incorrectly:

#include <core.p4>
#include <v1model.p4>

struct headers { }

struct metadata { }

parser MyParser(packet_in packet, out headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
    state start {
        transition accept;
    }
}

control MyVerifyChecksum(inout headers hdr, inout metadata meta) {
    apply { }
}

control MyIngress(inout headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
    action a(bit<9> port) {
        standard_metadata.egress_spec = port;
    }
    table t {
        actions = {
            a;
            NoAction;
        }
        key = { }
        default_action = NoAction();
    }
    apply {
      a(0);
      t.apply();
    }
}

control MyEgress(inout headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
    apply { }
}

control MyComputeChecksum(inout headers hdr, inout metadata meta) {
    apply { }
}

control MyDeparser(packet_out packet, in headers hdr) {
    apply { }
}

V1Switch(MyParser(),
	 MyVerifyChecksum(),
	 MyIngress(),
	 MyEgress(),
	 MyComputeChecksum(),
	 MyDeparser()) main;

Note that action a is invoked via table t and directly in MyIngress.

However, the LocalizeAllActions and RemoveActionParameters passes generate this program:

#include <core.p4>
#include <v1model.p4>

struct headers {
}

struct metadata {
}

parser MyParser(packet_in packet, out headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
    state start {
        transition accept;
    }
}

control MyVerifyChecksum(inout headers hdr, inout metadata meta) {
    apply {
    }
}

control MyIngress(inout headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
    @name(".NoAction") action NoAction_0() {
    }
    @name("MyIngress.a") action a(bit<9> port) {
        standard_metadata.egress_spec = port;
    }
    bit<9> port_0;
    @name("MyIngress.a") action a_2() {
        port_0 = 9w0;
        standard_metadata.egress_spec = port_0;
    }
    @name("MyIngress.t") table t_0 {
        actions = {
            a();
            NoAction_0();
        }
        key = {
        }
        default_action = NoAction_0();
    }
    apply {
        a_2();
        t_0.apply();
    }
}

control MyEgress(inout headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
    apply {
    }
}

control MyComputeChecksum(inout headers hdr, inout metadata meta) {
    apply {
    }
}

control MyDeparser(packet_out packet, in headers hdr) {
    apply {
    }
}

V1Switch<headers, metadata>(MyParser(), MyVerifyChecksum(), MyIngress(), MyEgress(), MyComputeChecksum(), MyDeparser()) main;

Note that there are two actions named MyIngress.a, but they take different numbers of parameters.

This may break the control plane when trying to insert an entry into t with action a.

I can provide more detail if helpful, but hopefully the issue is clear...

[jafingerhut]

I do not know if this is exactly the same, or different, but there are at least similarities with this behavior and the behavior described in this issue: https://github.com/p4lang/p4c/issues/1936

I have the vague impression that there is something subtly wrong here, but not sure how to make it more precise other than "two actions with the same names seem likely to cause problems in at least rare situations, if not common situations". Maybe Nate's example is a more common situation that makes the kinds of questions in the other issue more relevant to a typical P4 program?

[mihaibudiu]

Indeed, the algorithm for creating @name annotations had no freedom in naming these actions. However, note that action a_2 does not appear in any table that is configurable by the control-plane, so the control-plane may not even need to know about it.

[jnfoster]

True, but simple_switch_CLI rejects additions into t with action a...

I think p4c should really preserve uniqueness of named actions during compilation (rather than fixing the control plane to disambiguate).

[mihaibudiu]

The P4 spec has a section on how names are generated. I don't exactly know how this should be modified. The problem is that some names should really be relative names (action a as used in table t), and not absolute. The context should be enough to identify the component that is referred.

[jnfoster]

Why are spec changes needed? When the compiler duplicates a to get a_2, I think it should drop the name annotation.

[mihaibudiu]

There are other possibilities, like an action being used in two tables and optimized differently for each of them. This would lead to two actions with different names but the same @name. Their control-plane parameters should be the same, but the bodies could be different.

[jnfoster]

I see, I see. So simple_switch_CLI is really the thing that's broken and I should file an issue on Bmv2?

[mihaibudiu]

It depends on how the P4Runtime API is designed. If you can create an action and then insert in a table in separate operations, then the first operation does not have enough context to decide.

[jafingerhut]

There is no way to "create an action" in the P4Runtime API. There are ways to associate actions with newly added table entries, or to modify the action for an existing table entry, and to read table entries to see what their (key, action, action parameters are). In P4Runtime API, it only knows about actions that are in the P4Info file as generated by the compiler, and cannot refer to actions in any other way than how they are described there.

[jafingerhut]

My biggest question in this area is this.

Let us take it for granted that there are good reasons to take action definitions in a P4_16 source program, and create duplicates of those that can be optimized independently, e.g. perhaps for different tables where the same action can be invoked on a match, or as a default action.

It seems to me that this kind of activity is appropriate for a back end of a compiler, not as something that ever affects the control plane API. If the control plane sees that there are two tables with the same action name, it should be able to request adding that same action to either of the two tables, and it is up to the driver level target-specific code to determine which of several possible optimized versions of that action to invoke. The control plane API shouldn't ever see that detail.

Right now, p4c does action duplication per-table before the P4Info file is generated, in the front end. Why?

[mihaibudiu]

Pass ordering is not always an easy decision. The inlining passes are part of the front-end; they do generate new objects with new names. The runtime API generation pass is right at the end of the front-end.

[jafingerhut]

I understand it is not an easy decision. I am simply trying to point out that generating extra actions that are not in the user's program before P4Info generation seems counterproductive, and leads to difficult to fix and reason about corner cases.

[jafingerhut]

Note: I am not saying "don't inline". I am referring specifically to whatever part of the front end passes is duplication actions from the user's original program.

[mihaibudiu]

Even if we generate new actions after generating the API, the runtime still needs a way to identify which actions we are talking about. There could still be multiple actions with the same original name in different tables.

[jafingerhut]

"There could still be multiple actions with the same original name in different tables."

Can you give an example of this?

EDIT: Sorry, scratch that request. I know what you mean, but was having a brain fade there for a moment.

New response: So what if there can still be multiple actions with the same original name in different tables, after you generate the API? That is a perfectly fine and reasonable back-end optimization to perform. It is the the back end compiler's job, and driver's / runtime's job, to keep them straight.

[mihaibudiu]

If it is possible for this to happen if inlining is done API generation, it should also be possible to happen correctly if API generation is done after inlining. For the initial issue filed here, we should probably not generate an API at all for actions that do not appear in tables (why does the control-plane need to know about them?). That would handle this issue.

[jafingerhut]

It is either impossible, or very hard, to prove that it would be impossible to have a correct compiler that does duplication of actions before API generation, and I will not attempt it.

Is there a practical advantage that you know of to duplicating actions before API generation?

[mihaibudiu]

Only that some optimizations may take advantage of this fact.

[jafingerhut]

We can optimize after API generation, yes?

[mihaibudiu]

In principle yes. Today some passes are only run once. We will have to check whether any other passes require action duplication in the frontend. I suspect not.

[jafingerhut]

Take a look at the test programs for issue #2755, which generate incorrect P4Info files with latest p4c as of 2021-Apr-30.

Think about how one might develop a fix for that bug, such that the compiler gave a compile-time error for both of those input programs, and at the same time p4c still generated duplicate actions before API generation. It seems way easier to fix that bug by delaying the creation of duplicate actions until after API generation.

[mihaibudiu]

This is an experiment we should try.

[jafingerhut]

PR https://github.com/p4lang/p4c/pull/4975 has been merged, which might address part of the questions raised by this issue, but I suspect that #4975 does not fix all of the problems raised here.