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.

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

p4runtime output includes a bunch of tbl_act_N tables and act_N actions. #425

Closed
antoninbas opened this issue Apr 3, 2017 · 20 comments
Closed
Assignees

Comments

@antoninbas
Copy link
Member

{
   "preamble": {
    "id": 33613915,
    "name": "tbl_act_0",
    "alias": "tbl_act_0"
   },
   "actionIds": [
    16829005
   ],
   "constDefaultActionId": 16829005,
   "size": "1024"
}
{
   "preamble": {
    "id": 16829005,
    "name": "act_0",
    "alias": "act_0"
   }
}

For a medium-sized P4 program, I can see about a dozen of them. I didn't remember having this problem before, so I rolled back to a p4c version from a week ago and confirmed that this was a new issue.
More precisely, at refpoint cbee56a I do not observe the issue but at refpoint b88b850 I do observe the issue. Which means that the issue must have been introduced by one of these 3 commits by @mbudiu-vmw :

@sethfowler
Copy link
Contributor

A lot changed in those commits, but my guess is that the new UniqueNames passes introduced in b88b850 may be responsible. I'll dig in more later.

@mihaibudiu
Copy link
Contributor

These actions are created by the compiler to execute code that is in a control but cannot be executed directly on the target. They are a symptom that the code is not very well optimized. They should not affect the correctness of the program.
These tables have constant entries - only a default action in fact. So they do not need to be exposed to the control-plane.

@sethfowler
Copy link
Contributor

That should be easy enough to fix, then, except that currently we don't have a way of expressing that a certain table or action shouldn't be exposed. I could add support for an @hidden annotation or something of the sort. Which code generates those tables and actions, so I can tag them appropriately?

@mihaibudiu
Copy link
Contributor

This is at most a performance bug, but not a correctness bug.
Solving it will require better optimizations.

These tables do not have a key - that's a hint that they do not have to be exposed. They should also have a size of 0 - but I guess that the default size is being used.

@sethfowler
Copy link
Contributor

I'd prefer not to use a heuristic to identify them; that seems fragile.

@mihaibudiu
Copy link
Contributor

It is not a heuristic.
If a table has no key it cannot have any actions.

@mihaibudiu
Copy link
Contributor

The passes in question are SynthesizeActions and MoveActionsToTables.
They have been there for a long time.
What has changed is that we use more temporaries in the program, and the expressions to compute these temporaries are lifted into these one-time-use actions.

@sethfowler
Copy link
Contributor

Thanks, Mihai. I'll take a look.

@mihaibudiu
Copy link
Contributor

But if the problem is the control-plane this is the correct solution: a table with no key and a const default_action should not be exposed. There is nothing the control-plane can do about it. (Well, perhaps it can, if it has counters...).
This is part of the P4 v1.0 spec too.

@mihaibudiu
Copy link
Contributor

One other thing you can do it to assign them a name such as @name(".") in the MoveActionsToTables pass.

@sethfowler
Copy link
Contributor

Yeah, that could work.

@jafingerhut
Copy link
Contributor

P4_14 allows one to create tables with no fields in the key, and one or more possible actions, and I thought (perhaps only assumed) that the control plane API would allow you to specify the default action on a miss, and that default action would be performed for every packet on which that table is applied. Does P4_16 plan to remove this capability, assuming I am not imagining this capability in P4_14?

Such tables are potentially useful for configuring one of several possible actions to take for all packets that reach a particular point in the control flow, regardless of that packet's contents.

@mihaibudiu
Copy link
Contributor

That should remain.
But if the default_action is also const, then it's really a read-only table for the control-plane.

@sethfowler
Copy link
Contributor

Even read-only tables should appear in the control plane, really, assuming the programmer introduced them and not the compiler. Although you can't change their behavior, it does seem to reasonable to query information about such a table (its default action, any annotations that were applied to it, etc).

@jafingerhut
Copy link
Contributor

I like @sethfowler's idea of having something like a @hidden annotation that is automatically added to compiler-synthesized tables like this. Even if they have no fields in the key, and a single action, and a const default_action, or whatever conditions you want to think of, if they are in the P4 source program, making them at least readable from the control plane seems like a good idea.

@mihaibudiu
Copy link
Contributor

If no one is working on this I can do it myself.
I would prefer to use something like @name(".") to avoid polluting the namespace of annotations.
Having these compiler-synthesized tables be exposed to the control plane violates the principle advocated by @jnfoster that one can read the control-plane from the P4 program.

@antoninbas
Copy link
Member Author

Any chance progress can be made on this? Did you reach consensus on one of these 2 solutions?

@sethfowler
Copy link
Contributor

@jnfoster, has the committee reached agreement on an approach here? Should we go ahead and implement @hidden or something like it?

@cole-barefoot
Copy link
Contributor

We discussed and are leaning toward the approach proposed here:

p4lang/p4-spec#138 (comment)

If I understand your discussion above, adding the @hidden annotation should address the issue above.

Thoughts? The next steps are to (a) draft a PR against the spec and (b) implement the changes and make a PR against p4c.

@mihaibudiu
Copy link
Contributor

OK, let's just use a @hidden annotation and get rid of this problem.
I will place this on all actions and tables synthesized by the compiler.

mihaibudiu pushed a commit to mihaibudiu/p4c-clone that referenced this issue Apr 12, 2017
antoninbas pushed a commit that referenced this issue Apr 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants