p4lang / behavioral-model

The reference P4 software switch
Apache License 2.0
532 stars 327 forks source link

Behavior of tables with empty keys #1208

Closed kheradmandG closed 11 months ago

kheradmandG commented 11 months ago

Following the discussions in https://github.com/p4lang/p4-spec/issues/1120 , newer versions of P416 define the following behavior for tables with empty keys.

If a table has no key property, or if the value of its key property is the empty tuple, i.e. key = {}, then it contains no look-up table, just a default action—i.e., the associated lookup table is always the empty map.

I have the following table (in a v1model ingress pipeline)

table disable_xyz_table {
    key = {}
    actions = {
      @proto_id(1) disable_xyz_action;
    }
  }

And in BMv2, using P4Runtime I install an entry with empty set of keys and the action being disable_xyz_action (Note that is_default_action is NOT set).

BMv2's log shows that entry successfully gets installed and the trace shows that the entry gets hit:

[17:37:10.785] [bmv2] [T] [thread 5262] [0.0] [cxt 0] Applying table '...disable_xyz_table'
[17:37:10.785] [bmv2] [D] [thread 5262] [0.0] [cxt 0] Looking up key:

[17:37:10.785] [bmv2] [D] [thread 5262] [0.0] [cxt 0] Table '...disable_xyz_table': hit with handle 0
[17:37:10.786] [bmv2] [D] [thread 5262] [0.0] [cxt 0] Dumping entry 0
Match key:
Action entry: ...disable_xyz_action - 

[17:37:10.786] [bmv2] [D] [thread 5262] [0.0] [cxt 0] Action entry is ...disable_xyz_action - 
[17:37:10.786] [bmv2] [T] [thread 5262] [0.0] [cxt 0] Action ...disable_xyz_action

Am I missing something here or the BMv2 and P416 are not in agreement in this regard?

antoninbas commented 11 months ago

Based on what you describe, this sounds like a bug. bmv2 should probably reject entries if the table has no match key, and this error case should probably also be caught early in the P4Runtime server (p4lang/PI).

jafingerhut commented 11 months ago

@kheradmandG @smolkaj Sorry if I sound like a broken record here saying this 2 days in a row -- If someone wants bug fixes or enhancements in the BMv2 software switch, the ones who want it are the best ones to make it happen in a timely fashion. I am aware of only one legal entity actively using BMv2 for commercial purposes.

antoninbas commented 11 months ago

I can actually implement the bmv2 part of this. bmv2 will start returning an error if an entry (any entry) is added to a table with no match key. For the P4Runtime side of things, it will need to be discussed by the API WG (but because bmv2 will start returning an error, the P4Runtime Write will fail).

smolkaj commented 11 months ago

If someone wants bug fixes or enhancements in the BMv2 software switch, the ones who want it are the best ones to make it happen in a timely fashion

We are aware, Andy. But I think there is value in reporting issues even if we or someone else does not get around to fixing them right away (or even ever).

jafingerhut commented 11 months ago

@smolkaj I fully support reporting such issues. Sorry if I gave the impression that I was against the creating the issue. What I am advocating for is a larger number of people than in the past to be willing to step forward and make such changes themselves, if they want them, and not rely on the very small set of people who have done so in the past.

antoninbas commented 11 months ago

I have closed the issue in this repository since it has been addressed. Feel free to open an issue in p4lang/PI - maybe after it has been discussed in the P4 API WG (Andy already has an issue for p4lang/p4runtime).