add text to support sending cloned packet to multicast group

[hanw]

No description provided.

[jafingerhut]

Currently PSA v1.0 defines for normal packets sent from ingress to the PRE, that it will be multicast replicated if the multicast group is non-0, otherwise it will be unicast to the one egress port specified.

Is there a motivation for why we would want cloning to work differently than that?

If we do want to allow a clone to be both multicast, and sent to a specified single egress port, for the same original packet, perhaps we should say that it is multicast if the multicast group value is non-0, otherwise no multicasting of the clone occurs. Also, perhaps there should be a valid bit<1>/boolean flag to indicate whether the clone should be unicast to egress_port or not? If we do not have that, the text proposed here does not seem to have much useful to say about how the decision is made to make a unicast clone.

[hanw]

Good point, it would be nice to keep the multicast behavior consistent between the normal and cloned packet. I have no objection with updating the text.

PortId_t         egress_port;
MulticastGroup_t multicast_group;
ClassOfService_t class_of_service;
bool             truncate;
PacketLength_t   packet_length_bytes;  /// only used if truncate is true

As these clone session fields are managed by the control plane, not data plane. This part of the spec should follow what P4Runtime does. Below is how P4Runtime defines the CloneSessionEntry.

message CloneSessionEntry {
  uint64 session_id = 1;
  repeated Replica replicas = 2;
  uint64 class_of_service = 3;
  uint64 packet_length_bytes = 4;
}

We could say control plane can configure both egress_port AND multicast_group on a clone session, which will be implemented as a repeated (egress_port, instance).

We could also say control plane can configure either egress_port OR multicast_group on a clone session, which will also be implemented the same way by P4Runtime.

I am not sure if we need that boolean flag for unicast in a clone session, as it cannot be set by dataplane anyway.

[jafingerhut]

Given the P4Runtime message you show above, it is kind of invisible to the P4Runtime client how it is done, and the data plane might use a multicast_group value to implement it, or it might not. Documenting in PSA that there is an egress_port or multicast_group in the per-clone-session state is potentially misleading.

Perhaps PSA should document the clone session state more similarly to how the P4Runtime message contents look, and not mention a multicast_group value at all, just a "set of (egress_port, instance) pairs"?

[cc10512]

I thought the whole point was that we wanted to clone to multicast groups. @jafingerhut are you saying that this should be accomplished by explicitly listing all the members of a multicast group as (egress_port, instance) pairs? Isn't this an overkill?

I do agree that we should keep the behavior consistent between unicast/multicast and cloning. If one wants to clone to both a port and a multicast group, is it too onerous to require that the port is added to a "new" multicast group, and thus simply multicast?

[jafingerhut]

I think the main point is that we want to add the capability to have a cloned packet be multicasted, somehow.

One way to do that is to associate a multicast group id to a clone session.

Another is to associate a set of (egress_port, instance) values to a clone session.

The second way is what the current P4Runtime API spec does.

We could say that this must be implemented in PSA under the covers by using a multicast group id, but requiring that seems to unnecessarily restrict implementations. If we have PSA define as a set of (egress_port, instance) pairs, a (PSA data plane + P4Runtime server) is free to implement it using some kind of reserved multicast group id value that P4Runtime cannot see, or by some other mechanism that doesn't need a multicast group id value at all, if the data plane supports that.

Maybe this is too minor a point to worry much about, but the "set of pairs" way seems to give slightly more freedom to implementers on how they want to do it.

[jafingerhut]

Yet another minor point I thought of since my last comment on this issue, but adding it in case it helps make progress resolving this.

I was thinking that if we defined a clone session to mention a PRE multicast group id number directly, that would imply that an implementation must implement cloning by using the PRE 'machinery' of a device.

It doesn't.

We can simply say it means that when cloning a packet using clone session id with multicast group id X, the set of (egress_port, egress_rid) values used when making clone copies of the packet must be the same as if you did a normal multicast packet using group id X.

Under the covers, a PSA implementation is free to reuse the same PRE machinery for cloning if it wants to, but it is also free to have custom cloning 'machinery' separate from the PRE. When the control plane updates the set of (egress_port, egress_rid) values associated with group id X, "under the covers" the agent software can implement that by changing that set in two different places in the hardware, for example, one in the PRE and another in the cloning machinery. Such an update in two places won't be atomic, but we can explicitly mention that PSA implementations are allowed to do it this way if they want, and control plane writers should not expect this kind of atomicity of multicast group id updates between cloning and normal multicasting operations.

[jafingerhut]

This PR should also include a small change to the table in Section 6.4 "Initial values of packets processed by egress", the table with the caption: "Initial values for packets processed by egress."

Right now it specifies the value for the "instance" field at the start of egress processing to be:

"From PacketReplicationEngine configuration for NM packets. 0 for all other kinds of packets."

Perhaps that should change to something like the following?

"From PacketReplicationEngine configuration for NM packets, or cloned packets that are multicast via a multicast group. 0 for NU packets, and for cloned packets that are not multicast replicated."

[cc10512]

Nit: no need for commas.

[cc10512]

I thought the whole point was that we wanted to clone to multicast groups. @jafingerhut are you saying that this should be accomplished by explicitly listing all the members of a multicast group as (egress_port, instance) pairs? Isn't this an overkill?

I do agree that we should keep the behavior consistent between unicast/multicast and cloning. If one wants to clone to both a port and a multicast group, is it too onerous to require that the port is added to a "new" multicast group, and thus simply multicast?

[jafingerhut]

Thanks! I think this does a good job of matching the desired multicast clone capability, and what the P4Runtime API does.

[jafingerhut]

Looking good.

[hanw]

Can I merge this?

[jafingerhut]

@cc10512 Do you want to look at this before it is merged?

[cc10512]

Since this is pseudo code, would it be too outside P4 to write:
(egress_port, instance) = egress_port and instance in the set of configured sessions for ostd.clone_session_id in PRE?

[cc10512]

hmm, just as you did in line 1023 :)

[cc10512]

I'm not sure I follow the text in the CI2E column. Maybe prepend a 'value from PRE configuration ...'

[cc10512]

Have we not defined 'normal port' before? If not, maybe we should, and then we can use it throughout.