Add the initial default action to tables.

[fruffy]

WiP. Corresponding PR: https://github.com/p4lang/p4c/pull/4773 Fixes #470.

[jafingerhut]

Is it planned to add to this PR modifications to the specification document describing this new field? Or perhaps part of a separate PR?

[fruffy]

I realized that this does not cover default arguments of the action. I think this requires adding a new message type.

[fruffy]

@jafingerhut Do you know whether there are any specific restrictions on default action instantiations on tables? Otherwise, we could just create a simple P4Info message that describes an action call. This can also be reused for entries, in case we implement them.

[jafingerhut]

The current file created by p4c for const entries (and initial entries) is a sequence of text-format Protobuf WriteRequest messages.

Perhaps for initial default actions, a WriteRequest that does a MODIFY (not an INSERT, which is not allowed on default actions at run time), and has the field is_default_entry=true in the TableEntry message, would be appropriate for this purpose?

To have that in the P4Info file would probably require copying, and/or somehow otherwise referencing, many of the definitions in the runtime Protobuf message definitions, which are not now part of the P4Info message definitions.

[jafingerhut]

Even if such a message was not WriteRequest, then Update, then TableEntry, at least the TableEntry part would be sufficient. I suspect even that would require a large number of other sub-message definitions to be available in the P4Info file message.

[fruffy]

I was thinking many of the features of a write request are not required here.

message TableAction {
  oneof type {
    Action action = 1;
    uint32 action_profile_member_id = 2;
    uint32 action_profile_group_id = 3;
    ActionProfileActionSet action_profile_action_set = 4;
  }
}

message Action {
  uint32 action_id = 1;
  message Param {
    uint32 param_id = 2;
    bytes value = 3;
  }
  repeated Param params = 4;
}

Something of this form might be the only required entities. The question is whether we even need the action profile specification, I have not tried whether a default action profile entry is possible.

[jafingerhut]

I agree that not everything that is in the TableEntry message is required for default actions. The one or two things that might make it worthwhile are the entry metadata. It would be good to hear from other P4Runtime users whether they use the metadata fields for default actions as well as table entries.

[jafingerhut]

@smolkaj @chrispsommers For default actions on tables, do either of you know what kinds of annotations and/or metadata someone might want to include in a P4 source file that could attach to the initial default action of the table, and would be nice to preserve in the output of the compiler describing that initial default action?

[chrispsommers]

Hi Andy, I've nothing to contribute here. Any standard annotations like @brief() or @doc() would presumably be preserved in the P4info, but I think you're hinting at possibly other existing, or perhaps new, annotations to annotate default action params, etc?

For default actions on tables, do either of you know what kinds of annotations and/or metadata someone might want to include in a P4 source file that could attach to the initial default action of the table, and would be nice to preserve in the output of the compiler describing that initial default action

[jafingerhut]

@fruffy Let me know if you want more details, or would prefer if I created a separate PR to carry this torch, but here was a suggestion resulting from the 2024-Jul-12 P4 API working group meeting:

Instead of having a new field initial_default_action_id in the Table message as this PR proposes right now, do the following:

• Copy and paste message Action from the file p4runtime.proto into p4info.proto. Rename it to something that has no name conflicts in the p4info.proto file, perhaps something like ActionSpec. This contains all the fields necessary to specify an action id, and values for all of its run-time parameters.

• Add a new field ActionSpec default_action_spec = <new_id> to message Table in p4info.proto. This would contain the action id and the action parameter values for the default_action of the table, regardless of whether it was default_action or const default_action, and would still be present even if there was no default_action in the source code for the table definition. In the last case, default_action_spec would contain an action id for a NoAction action, and 0 parameter values.

• Yes, this does mean that the existing field of Table named const_default_action_id would become redundant for tables with const default_action = .... declared. In the interest of backwards compatibility, it would stay in the Table message unchanged. The value of const_default_action_id being 0 would also be the only way that a reader of the P4Info Table message could use to distinguish whether the default_action of the table was declared const or not.

The current P4Info file already has all of the annotationan source location anyone could want about all actions, which must already include whatever a table's initial default_action happens to be, so nothing like that is needed in the new ActionSpec message.

[fruffy]

Sounds good to me! I can give this a crack. I can also reuse your descriptions for the documentation.

[chrispsommers]

See https://github.com/p4lang/p4runtime/issues/470

[chrispsommers]

@fruffy bump - can we do this by end of month to meet 1.4.0 release goals? Thx.

[fruffy]

@fruffy bump - can we do this by end of month to meet 1.4.0 release goals? Thx.

Currently have a lot on my plate. Will do my best but can not guarantee I will finish the implementation on both the compiler and here until then. :/

[smolkaj]

No worries, there will be a 1.5 if we can't make it in time.

[fruffy]

@fruffy Thanks for expediting this! It looks good to me, but before I "approve" I have a question. We often like to have a p4c front-end validation (or at least a proof-of-concept) of P4info schema changes when there's something new like this. It's not mandatory, but desirable. Are you intending to implement the p4c front-end changes, and if so, is there any possibility of doing so, say, by end of the month?

The corresponding PR is here https://github.com/p4lang/p4c/pull/4773

[fruffy]

LGTM, thanks!

How's your merge process for these things? Should I just go ahead and merge or wait for @smolkaj?

[chrispsommers]

@smolkaj Could you please take a look? @fruffy Let's aim to merge by Friday unless concerns are raised. You're welcome to do so.

[chrispsommers]

Thanks Fabian!