Implement PSA for eBPF backend #3139
Conversation
Co-authored-by: Mateusz Kossakowski <[email protected]> Co-authored-by: Jan Palimąka <[email protected]>
|
@mbudiu-vmw @rst0git I've synced with master and minimized build dependencies. |
|
So I guess this is now ready for review? |
I think so |
backends/ebpf/ebpfControl.h
Outdated
| protected: | ||
| const EBPFControl* control; | ||
| std::set<const IR::Parameter*> toDereference; | ||
| std::vector<cstring> saveAction; | ||
| P4::P4CoreLibrary& p4lib; | ||
|
|
||
| int commentDescriptionDepth; |
There was a problem hiding this comment.
you should document this variable.
backends/ebpf/ebpfParser.cpp
Outdated
| // eBPF can pass 64 bits of data as one argument passed in 64 bit register, | ||
| // so value of the field is printed only when it fits into that register | ||
| // eBPF can pass 64 bits of data as one argument, so value of the field is | ||
| // printed only when its fits into register |
backends/ebpf/psa/ebpfPsaArch.cpp
Outdated
| /* | ||
| * 6. BPF map initialization | ||
| */ | ||
| emitInitializer(builder); |
There was a problem hiding this comment.
interesting, I expected that this is done at runtime. I wonder how bpf does it, since maps do not exist until the program is started.
There was a problem hiding this comment.
Do you mean "loaded" by "started"? This initialization function is actually called by psabpf pipeline load. I'll add a section to documentation about this.
backends/ebpf/psa/ebpfPsaArch.cpp
Outdated
|
|
||
| parser = new EBPFPsaParser(program, prsr, typemap); | ||
|
|
||
| auto it = pl->parameters.begin(); |
backends/ebpf/psa/ebpfPsaArch.cpp
Outdated
| } | ||
|
|
||
| // use 1024 by default | ||
| size_t size = 1024; |
There was a problem hiding this comment.
hidden like this it will be hard to find and change. Perhaps it should also be a compiler option.
backends/ebpf/psa/ebpfPsaArch.cpp
Outdated
|
|
||
| bool ConvertToEBPFControlPSA::preorder(const IR::AssignmentStatement *a) { | ||
| // the condition covers both ingress and egress timestamp | ||
| if (a->right->toString().endsWith("timestamp")) { |
There was a problem hiding this comment.
Yeah, I agree it is. The goal is to find out if ingress_timestamp or egress_timestamp is used in a control block. We haven't found a better way to do this so far.
|
Please make any changes as new commits, I can't read this much code again. |
|
@mbudiu-vmw Thanks for review! I believe I've address all of your comments. |
backends/ebpf/psa/ebpfPsaGen.cpp
Outdated
|
|
||
| bool ConvertToEBPFControlPSA::preorder(const IR::AssignmentStatement *a) { | ||
| // the condition covers both ingress and egress timestamp | ||
| if (a->right->toString().endsWith("timestamp")) { |
There was a problem hiding this comment.
A more robust way is to look for a Member named "timestamp" from a type that is like the metadata. Or at least a member...
| CE2E is implemented by invoking `bpf_clone_redirect()` helper in the Egress path. Output ports are determined based on the | ||
| `clone_session_id` and lookup to "clone_session" BPF map, which is shared among TC ingress and egress (eBPF subsystem allows for map sharing between programs). | ||
|
|
||
| ## Sending packet to CPU |
There was a problem hiding this comment.
The subtitle "Sending packet to CPU" is slightly confusing in the context of eBPF because everything is running on CPU. Perhaps it would be more appropriate to change the subtitle to "Sending packet to Control Plane" or "Sending packet to Userspace"?
@jafingerhut What do you think?
There was a problem hiding this comment.
The current PSA specification might be contributing to some confusion here, since that spec does refer to a "CPU port" to send packets from the data plane to the controller. That specification does have at least several places with a kind of built-in assumption that the P4 data plane is separate from a nearby control CPU, at least in how it names things.
I'm not sure if there are only a few small surgical changes in the PSA spec that would match your suggestion here, but unfortunately if we want to avoid changing names in the P4 include file for the architecture, there are already a few names like these in there (https://github.com/p4lang/p4c/blob/main/p4include/bmv2/psa.p4):
const PortId_t PSA_PORT_CPU = (PortId_t) 0xfffffffd;
const CloneSessionId_t PSA_CLONE_SESSION_TO_CPU = (CloneSessionId_t) 0;
|
@mbudiu-vmw I've addressed your last comments. Please resolve conversations/let me know what else I need to address. |
backends/ebpf/psa/ebpfPsaGen.cpp
Outdated
| emitCommonPreamble(builder); | ||
| builder->newline(); | ||
|
|
||
| // TODO: enable configuring MAX_PORTS/MAX_INSTANCES/MAX_SESSIONS using compiler optios. |
backends/ebpf/psa/ebpfPsaGen.cpp
Outdated
| // the condition covers both ingress and egress timestamp | ||
| if (a->right->toString().endsWith("timestamp")) { | ||
| control->timestampIsUsed = true; | ||
| if (auto m = a->right->to<IR::Member>()) { |
There was a problem hiding this comment.
I think you can do even better: just use the IR::Member visitor function and check there.
|
Will give you a chance to address the last two small comments. |
This PR implements the PSA model for eBPF backend. Please read the included README to learn about the design and implementation details. This PR adds the PSA model that leverages the eBFP/TC hook. In subsequent PRs we're planning to add PTF-based test cases, support for XDP-based design, support for all PSA externs and ternary matching.
The basic usage model:
Note that current code contains many placeholders that are not used now, but they will make adding support for PSA externs easier.
This is a big milestone for the team that have been working on this project for over 1 year. Thanks to main developers @kmateuszssak @tatry and other folks that contributed to this project: @fdangtran and @elfadel.