serializer "introduces" compiler names

[antoninbas]

I am putting introduces in quotation marks because I know these are introduced by some pass before the p4runtime serializer.
Consider the following P4_14 program:

parser start {
    return ingress;
}

action a1(port) {
    modify_field(standard_metadata.egress_spec, port);
}

table t1 {
    actions { a1; }
}

control ingress {
    apply(t1);
}

The p4runtime serializer produces the following output for actions:

"actions": [
  {
   "preamble": {
    "id": 16832930,
    "name": "NoAction_1"
   }
  },
  {
   "preamble": {
    "id": 16792110,
    "name": "a1"
   },
   "params": [
    {
     "id": 54144512,
     "name": "port",
     "bitwidth": 9
    }
   ]
  }
 ]

NoAction_1 is not a P4 name. Because of this the control plane shouldn't be able to see this "default-default" action at all. I believe it should be removed from the serialized data.

[mihaibudiu]

Actually NoAction is from core.p4, it corresponds to the noop primitive action from p4-16.
(NoAction_1 is just a clone of NoAction; it should probably have a @name("NoAction") annotation).

[sethfowler]

Yeah, we'll have to @name all the things, as they say.

I'd like to avoid hardcoding things into p4RuntimeSerializer. If there's a need to not expose certain names from core.p4 or v1model.p4, we can add an annotation that expresses that.

[antoninbas]

@mbudiu-vmw is it as simple as adding the @name("NoAction") annotation to core.p4? If yes would it be possible to open a PR for this? I could do it if you want.

[mihaibudiu]

If this solves this particular problem, please do.
(I am on vacation this week.)

[sethfowler]

@antoninbas, #325 eliminates the excessive number of copies of NoAction. Thanks for taking care of that.

The next step is probably to detect NoAction in the serialization code and set the table's const_default_action_id to 0, right? As I mentioned in an earlier comment, for other similar cases we probably want to use annotations, but I think for this specific case we need some special handling, since P4Info has a special way of expressing this. (We could use an annotation to identify NoAction as the special no-op default action, but since it's in core.p4 we probably don't gain much over just hardcoding the name.)

I can put together a patch that does that.

[sethfowler]

Hmm, OK. So if the default action is not const, it's not exposed in P4Info? I guess this makes sense - in that situation, the default action is pretty much just specifying the initial state of the table, so it should be exposed via the PI functions that get and set table state.

That means I'm not handling default actions correctly, though. I realize now that I'm currently exposing any default action via const_default_action_id.

[antoninbas]

@sethfowler Yes const_default_action_id should probably be set to 0 when the default action was added by the compiler.
One question: do you know if in P4_16 there is a way to set a constant default action but allow the action data to be changed at runtime (which is what const_default_action_id is meant to indicate)? It seems to be that in some case P4 programmers want to guarantee const action + const action data, and in some cases just const action.

[sethfowler]

@antoninbas So here's what the P4-16 spec has to say:

Note that the specified default action must supply arguments for the control-plane bound param- eters (i.e., the directionless parameters), since the action is synthesized at compilation time. The expressions supplied as arguments for parameters with a direction (in, inout or out) are evaluated when the action is invoked while the expressions supplied as arguments for directionless parameters are evaluated at compile time.

The "control-plane bound parameters" here are the action data, so it sounds like the spec requires const action data. It sounds like it would be straightforward, syntactically at least, to permit the action data to be bound at runtime - the programmer could just not supply arguments for the control-plane bound parameters in the default action specification - but currently the spec forbids it.

[antoninbas]

IIRC in P4_14 you can do just that: default_action : action_with_1_arg;, as opposed to default_action : action_with_1_arg(0);. The problem is that P4_16 states that the behavior is undefined until the control plane actually provides action data. And it seems weird to provide "initial" action data that can be changed at runtime...
Anyway maybe we digress a little, for now I believe that const_default_action_id should be set to non-0 iff the P4 programmer provided a default_action (in P4_14) or a const default_action (in P4_16). If the action is added by the compiler (in this case NoAction), const_default_action_id should be set to 0. What do you think?

[sethfowler]

I went ahead and implemented the approach you proposed; the patch is in #326.

Are there more compiler names we need to fix, or can we close this issue once #326 lands?

[antoninbas]

@sethfowler You can go ahead and close once #326 is merged

[mihaibudiu]

Check out the @defaultonly annotation in the spec.