[psa_switch] first pass at i2e cloning

[peteli3]

As the title says, this is a first attempt at implementing i2e cloning.
It should be reminiscent of ingress cloning in simple_switch.cpp, except that the pre-ingress packet is not parsed out-of-band again because the pre-ingress packet's next step is a (newly) explicit egress parser.

stf test:
p4lang/p4c#2499

[codecov-commenter]

Codecov Report

Merging #935 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #935   +/-   ##
=======================================
  Coverage   74.33%   74.33%           
=======================================
  Files         115      115           
  Lines       10252    10252           
=======================================
  Hits         7621     7621           
  Misses       2631     2631           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f9ad70...c97af04. Read the comment docs.

[antoninbas]

Thanks for working on this. I left a few comments.

[peteli3]

Working on the comments above. Please do not review yet.

[peteli3]

@antoninbas @jafingerhut I addressed Antonin's comment's from above - and also took a stab at adding support for mirroring_* commands in pswitch_CLI which were useful for the corresponding STF tests p4lang/p4c#2499

Please review when you get a chance

[jfingerh]

@peteli3 This is looking pretty good to me. Have you built the latest behavioral-model plus these changes on your machine, plus latest p4c plus your p4c PR's new test case, and tried running all p4c tests to see which pass and which fail?

It is OK if p4c tests with file names beginning with 'ebpf' or 'ubpf' fail (at least, I have yet to figure out how to build p4c so that they pass), but the rest should all pass before we make changes to behavioral-model repo.

[jfingerh]

@antoninbas I do not know whether I am correct here in my belief, or even if I am, whether this PR needs to address it before we merge it in, since it can be improved later if it is an issue.

I believe the basic idea of these changes is that every clone session can either have an egress_port configured for it, or a multicast group id.

Later when we want to implement the P4Runtime API for PSA on bmv2, IIRC every clone session should have its own independent set of (egress_port, instance) pairs, and those should be numbered by the clone session id, independent of any multicast group ids configured in the device.

Does that sound correct to you? And if it does, then at least a little bit of this psa_switch code will need a few changes to accomodate that?

[antoninbas]

some minor comments, otherwise LGTM

[antoninbas]

@jafingerhut this is the correct implementation in order to reuse the existing P4Runtime implementation. That implementation actually always creates one multicast group for each clone session: https://github.com/p4lang/PI/blob/771d5c46106f14f33777e70f39df2ae772cdfe17/proto/frontend/src/pre_clone_mgr.cpp#L107. IIRC this is done regardless of whether the clone session asks for a single replica or more, to facilitate future updates to the clone session configuration.

[jafingerhut]

@antoninbas This PR has a function mirroring_session_add that creates a clone session and in one case assigns it the value of an mgid -- not a set of (egress_port, instance) pairs, but an mgid that can also be used for multicast replication in a P4 program by assigning that numeric value to the ostd.multicast_group metadata field in ingress.

It seems that either that needs to change before the P4Runtime API could possibly work correctly, or perhaps mirroring_session_add is only useful for implementing the Thrift API, and it would never be called from a P4Runtime API implementation?

[antoninbas]

@jafingerhut On the contrary, this is specifically to implement the P4Runtime API. The PI implementation will create a multicast group, using a mgid value from a reserved number space. The multicast group will be configured with the correct set of (egress_port, instance), then bmv2 will be configured through mirroring_session_add to map the clone session id to this mgid. When the clone session id is used for a packet, this packet should be multicast. All the copies sent to egress will thus have the correct egress_port and instance value. Am I misunderstanding the question? This code was pretty much copied over from simple_switch as is, but I may have missed something when I was reviewing.

[jafingerhut]

Ah, so the mgid values used for clone sessions come from a range completely outside of the [1, M] range that are used for multicast, and thus never conflict with them (for whatever maximum mgid value M that bmv2 supports)?

[antoninbas]

This is the relevant code in the implementation:

  // user-defined multicast group ids must be in the range
  // ]0,first_reserved_group[; ideally this should be configurable based on the
  // target.
  static constexpr GroupId first_reserved_group_id() { return 1 << 15; }

So the users can use up to 32K mgids. Everything else is reserved. The assumption here being that a target would typically support 64K mgids.

[jafingerhut]

Thanks for the explanation. That all makes sense to me now. I wouldn't be surprised if some targets might not use an mgid value at all internally for clone sessions that multicast, but for targets that do, this looks like a good way to do it, while still preserving the contiguous range of mgid's usable for non-clone mgid values.

[peteli3]

@peteli3 This is looking pretty good to me. Have you built the latest behavioral-model plus these changes on your machine, plus latest p4c plus your p4c PR's new test case, and tried running all p4c tests to see which pass and which fail?

It is OK if p4c tests with file names beginning with 'ebpf' or 'ubpf' fail (at least, I have yet to figure out how to build p4c so that they pass), but the rest should all pass before we make changes to behavioral-model repo.

@jafingerhut
I ran all tests on clean builds of both p4c and behavioral-model:

# p4c built with these flags
$ cmake .. -DCMAKE_BUILD_TYPE=DEBUG -DENABLE_EBPF=false && make -j4

# output from p4c tests
100% tests passed, 0 tests failed out of 1574

Label Time Summary:
XFAIL        =  20.14 sec*proc (17 tests)
bmv2         = 1420.44 sec*proc (439 tests)
cpplint      =  17.57 sec*proc (1 test)
err          =  37.20 sec*proc (231 tests)
gtest        =  13.81 sec*proc (1 test)
p14_to_16    = 505.02 sec*proc (191 tests)
p4           = 685.80 sec*proc (696 tests)
ubpf         =   8.37 sec*proc (15 tests)

Total Test time (real) = 2691.05 sec
Built target check-all
Scanning dependencies of target check
Built target check

[peteli3]

@antoninbas @jafingerhut please re-review when you get a chance, thanks!

[jfingerh]

Looks good to me. My focus has been on the functional behavior of the changes in the file psa_switch.cpp, and not the C++ style aspects of it, which Antonin is better at reviewing than me.

[antoninbas]

LGTM