Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Control-plane name clash introduced by LocalizeAllActions and RemoveActionParameters #2749

Open
jnfoster opened this issue Apr 26, 2021 · 22 comments
Open

Control-plane name clash introduced by LocalizeAllActions and RemoveActionParameters #2749

jnfoster opened this issue Apr 26, 2021 · 22 comments
Labels
bug This behavior is unintended and should be fixed. control-plane Topics related to the control-plane or P4Runtime. core Topics concerning the core segments of the compiler (frontend, midend, parser) question This is a topic requesting clarification.

Comments

@jnfoster
Copy link
Contributor

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...
@jnfoster jnfoster changed the title Name clash in Localize All Actions Control-plane name clash introduced by LocalizeAllActions and RemoveActionParameters Apr 26, 2021
@jafingerhut
Copy link
Contributor

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: #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
Copy link
Contributor

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.

@mihaibudiu mihaibudiu added the question This is a topic requesting clarification. label Apr 29, 2021
@jnfoster
Copy link
Contributor Author

jnfoster commented Apr 29, 2021 •
edited
Loading

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@mihaibudiu
Copy link
Contributor

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
Copy link
Contributor Author

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

@mihaibudiu
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

jafingerhut commented Apr 30, 2021 •
edited
Loading

"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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Only that some optimizations may take advantage of this fact.

@jafingerhut
Copy link
Contributor

We can optimize after API generation, yes?

@mihaibudiu
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

This is an experiment we should try.

@jafingerhut jafingerhut added the control-plane Topics related to the control-plane or P4Runtime. label Dec 27, 2021
@jafingerhut jafingerhut mentioned this issue Jan 20, 2022
Closed
@mihaibudiu mihaibudiu added the bug This behavior is unintended and should be fixed. label Jan 20, 2022
@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Mar 2, 2023
@jafingerhut jafingerhut mentioned this issue Oct 8, 2024
Closed
@jafingerhut jafingerhut mentioned this issue Oct 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug This behavior is unintended and should be fixed. control-plane Topics related to the control-plane or P4Runtime. core Topics concerning the core segments of the compiler (frontend, midend, parser) question This is a topic requesting clarification.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jafingerhut @jnfoster @fruffy @mihaibudiu