Allow omitting metadata fields in PacketOut/In messages to improve protocol efficiency

[ccascone]

Assuming the following P4 header for controller packet-out that includes padding:

@controller_header("packet_out")
header packet_out_header_t {
    bit<9> egress_port;
    bit<7> _pad;
}

that produces the following P4Info:

controller_packet_metadata {
  preamble {
    id: 67121543
    name: "packet_out"
    annotations: "@controller_header(\"packet_out\")"
  }
  metadata {
    id: 1
    name: "egress_port"
    bitwidth: 9
  }
  metadata {
    id: 2
    name: "_pad"
    bitwidth: 7
  }
}

Section 16.1 in the spec says

when a P4Runtime client (or server) generates a PacketOut (or PacketIn) message, it needs to populate the metadata field with as many values as in ControllerPacketMetadata.metadata for the packet-out (or packet-in) case.

The spec does not say what happens if a metadata is missing. Should the server/client generate an error? In this example, it seems that the _pad field could be omitted to improve protocol efficiency. Can we say that a client or server should treat missing metadata fields in PacketOut or PacketIn messages as zero?

[antoninbas]

I'm not against the change myself, but it would break backward-compatibility. An older server expecting a full set of values may drop a packet-out, or an older client expecting a full set of values may drop a packet-in.

The spec indeed does not say exactly what should happen when a metadata value is missing, but since it says that the endpoints need to provide all values, the most logical thing to do for an implementation would be to either:

1. drop the message, increase some error counter and possibly log an error message (there is no way to signal an error on the stream, short of closing the stream), or
2. close the stream, which may be a bit extreme but is a sure way to the other end point know that something is wrong

Maybe we should update the spec to clarify what happens in case of an incorrect message.

Even if a large portion of packets exchanged on the stream channel tend to be small, I would imagine that the size of the metadata is small compared to the size of the payload? However, I can see how in the presence of a large amount of metadata fields, many of which could be optional based on the type of packet, the protocol could be made much more efficient by giving the possibility to the client / server to omit metadata values.

We can have a WG meeting on the 27th and discuss this, but we have agreed in the past to take a strong stance against breaking backward-compatibility and this issue was opened shortly after the P4Runtime release.

[ccascone]

Agreed that this would break backward compatibility. I don't know why didn't think of that.

I would go with option 2 and close the stream if the packet-in/out message is not formatted as required. It's a way to enforce client/servers to be compliant with the spec.

To make the protocol more efficient when there is a large number of metadata fields, how about we add a boolean flag to specify the expected behavior? For example, here missing_metadata_as_zero could be used to signal the client (or server) that it's OK to receive a PacketIn (or PacketOut) message with missing metadata fields and that those should be treated as with value zero:

message PacketIn { // or PacketOut
  bytes payload = 1;
  repeated PacketMetadata metadata = 2;
  bool missing_metadata_as_zero = 3;
}

An old server or client implementation would leave this new field unset. According to the protobuf spec, the default value for bool is false, thus not breaking backward compatibility (all metadata fields are required).

[jafingerhut]

The new boolean flag will not prevent an old receiver from closing the stream if it receives a message with missing_metadata_as_zero=true and some missing metadata fields, but I guess adding the flag does mean that eventually when both sides have been upgraded that the 0 metadata fields can start to be left out of the messages.

[antoninbas]

@jafingerhut you would not close the stream. The old receiver would not even see the flag, it would be ignored by the Protobuf library (see https://developers.google.com/protocol-buffers/docs/proto3#unknowns). Depending on which version of the Protobuf library (before or after 3.5) you are using, the field would be discarded or not, but in both cases the old receiver would be oblivious to it.

[jafingerhut]

The old receiver would be oblivious to the new flag, but if the new sender sent the flag true, and omitted some controller metadata fields that were 0, the old receiver would close the connection, if I understand what you and Carmelo were discussing earlier.

[jafingerhut]

That isn't a big deal once both sender and receiver upgrade, and I do not see any way forward to allowing future P4Runtime participants to elide the 0 fields in these messages unless the suggested or similar change is made some time.

[antoninbas]

I think what Carmelo was proposing was that the client set missing_metadata_as_zero to True in any PacketOut and then from that point on the server knows it's ok to send PacketIn messages back to the client with missing metadata fields (which are assumed to be 0). And vice versa in the other direction. Personally I think that can work, but I would recommend adding that field to the arbitration messages instead. We can add something like an Option message in the oneof. It would be more extensible and less clumsy. And the endpoints wouldn't have to check the flag value on every packet in / out, which adds overhead.

[ccascone]

@antoninbas I was proposing to use the new flag to describe the content of the same PacketIn/Out message carrying the flag, so a receiver would have to look at the flag before processing the metadata values. Based on this description, @jafingerhut is right when saying that an old receiver would always close the stream if some metadata are missing, even if the flag is set to true. Closing the stream with a known error code could be used by a sender to "learn" that the receiver doesn't support omitting metadata fields, so it could re-open the stream making sure to always specify all metadata fields...

BUT, I like more your idea of using an Option message as a way to exchange protocol capabilities (not to be confused with pipeline capabilities) for the same reasons you mentioned.

[ccascone]

At the WG meeting on April 24th 2019 we agreed to add an Option message to the stream channel to advertise client/server “capabilities”, including the ability to omit packet-in/out metadata fields. However, considering that:

• there is no real urgency to support omitting metadata fields. Indeed, the benefit is protocol efficiency, but no one is trying to send a high rate of packet-outs with a large number of metadata fields set to 0
• error reporting for packet out will be added with #217
• we tend to agree that we should limit adding new messages to v1 of the spec to avoid making the spec more complex and ambiguous

For all these reasons, we agreed to park for now the idea of adding an Option message to v1 of the spec, instead moving the discussion to P4Runtime v2.0.