PSA/ebpf backend: add support for generating programs for XDP hook #3752

tatry · 2022-12-02T15:00:23Z

This PR adds functionality to generate programs for the XDP layer for the PSA/ebpf (part of NIKSS) backend.

Update of libbpf (new version v0.3) is required for definition of bpf_devmap_val struct which is available starting from this version.

CC: @osinstom @kmateuszssak @fdangtran @elfadel

This PR adds functionality to generate programs for the XDP layer for the PSA/ebpf (part of NIKSS) backend. Update of `libbpf` (new version v0.3) is required for definition of `bpf_devmap_val` struct which is available starting from this version. Co-authored-by: Tomasz Osiński <[email protected]> Co-authored-by: Mateusz Kossakowski <[email protected]> Co-authored-by: Frederic Dang Tran <[email protected]> Co-authored-by: El-Fadel Bonfoh <[email protected]>

mihaibudiu · 2022-12-02T19:21:36Z

backends/ebpf/ebpfBackend.cpp

@@ -73,7 +73,10 @@ void run_ebpf_backend(const EbpfOptions& options, const IR::ToplevelBlock* tople

    Target* target;
    if (options.target.isNullOrEmpty() || options.target == "kernel") {
-        target = new KernelSamplesTarget(options.emitTraceMessages);
+        if (!options.generateToXDP)


why not a new target?

I think the main reason is that we treat XDP as kind of optimization of the default TC-based design, since our design in XDP doesn't implement full PSA spec (see README - no CoS, no CLONE_E2E, no packet recirculation).

But to be honest, I don't have a strong opinion here. We could probably make it a new target pretty easily but then we would require some refactoring of other targets (e.g., kernel target is not a good name anymore since we have more than 1 kernel-based target, also how to resolve conflicts between PSA-eBPF and p4c-xdp?).

mihaibudiu · 2022-12-02T19:23:03Z

backends/ebpf/ebpfOptions.h

-            std::cout << "Setting XDP2TC 'meta' mode by default." << std::endl;
-            // Use 'meta' mode by default.
+        if (generateToXDP && xdp2tcMode == XDP2TC_META) {
+            std::cerr << "XDP2TC 'meta' mode cannot be used if XDP is enabled. "


the compiler has no nice way to talk to the user - it has warning and error.
I wonder whether we need an additional method...
this is good enough for now

These error message require a sophisticated user.
Can you give some URLs where the naive ones could find more information?

mihaibudiu · 2022-12-02T22:19:43Z

backends/ebpf/psa/README.md

@@ -11,9 +11,16 @@ This directory implements PSA (Portable Switch Architecture) for the eBPF backen

 # Design

+The PSA to eBPF compiler provides two flavors of generated eBPF code: TC-based design and XDP-based design.
+The TC-based design leverages eBPF TC (Traffic Control) hook and is able to implement any PSA program at the cost of performance. 


you can probably remove "at the cost of performance".

mihaibudiu · 2022-12-02T22:20:38Z

backends/ebpf/psa/README.md

 P4 packet processing is translated into a set of eBPF programs attached to the TC hook. The eBPF programs implement packet processing defined
 in a P4 program written according to the PSA model. The TC hook is used as a main engine, because it enables a full implementation of the PSA specification.
-We also plan to contribute the XDP-based version of the PSA implementation that does not implement the full specification, but provides better performance.
+The XDP-based version of the PSA implementation does not implement the full specification, but provides better performance.


can you provide a ballpark estimate of the improvement so people can decide which one to use?

Yes, I think we could @tatry, but the improvement depends on the P4 program. A ballpark estimate is over 2x improvement in throughput per CPU core measured in MPPS.

You can find an extensive performance evaluation in our recent paper: https://dl.acm.org/doi/10.1145/3555050.3569117. We could also reference this paper in the README?

mihaibudiu · 2022-12-02T22:24:56Z

backends/ebpf/psa/ebpfPipeline.cpp

+
+    // if Traffic Manager decided to pass packet to the kernel stack earlier, send it up immediately
+    builder->emitIndent();
+    builder->appendFormat("if (%s->pass_to_kernel == true) return %s;", compilerGlobalMetadata,


usually == true does not improve readability

mihaibudiu · 2022-12-02T22:26:28Z

backends/ebpf/psa/ebpfPipeline.cpp

+    builder->append(
+        "    void *data = (void *)(long)skb->data;\n"
+        "    void *data_end = (void *)(long)skb->data_end;\n"
+        "    if (((char *) data + 14 + sizeof(struct xdp2tc_metadata)) > (char *) data_end) {\n"


The size of Ethernet header. It could be replaced by a proper sizeof.

mihaibudiu · 2022-12-02T22:31:12Z

backends/ebpf/psa/ebpfPsaDeparser.cpp

+        builder->newline();
+        builder->blockEnd(true);
+        builder->append(
+            "    data = (void *)(long)skb->data;\n"


this pattern shows up a lot, is it worthwhile to factor into a macro?

I agree that it requires something like that.

I think rather about a function that takes a block of C code. It would split code into lines and for each line emits indent, line of code, and makes some string replace if required. Then it would be applied to a lot of places of PSA/eBPF backend. In such case I think this should be separate PR. What do you think about it?

mihaibudiu · 2022-12-02T22:31:38Z

backends/ebpf/psa/ebpfPsaDeparser.cpp

+        builder->appendLine("__builtin_memmove(data, data + sizeof(struct xdp2tc_metadata), 14);");
+        builder->emitIndent();
+        builder->appendLine(
+            "__builtin_memcpy(data + 14, "


in general I think it's good to document these magic constants

mihaibudiu · 2022-12-02T22:36:23Z

backends/ebpf/psa/ebpfPsaGen.cpp

-void PSAArchTC::emitInitializerSection(CodeBuilder* builder) const {
-    builder->appendLine("SEC(\"classifier/map-initializer\")");
+void PSAArchXDP::emitDummyProgram(CodeBuilder* builder) const {
+    // this is static program, so we can just paste a piece of code.


I don't understand this comment

mihaibudiu approved these changes Dec 2, 2022

View reviewed changes

tatry added 5 commits December 5, 2022 10:24

Add link about XDP to TC mode

53f4ffc

Remove at the cost of performance in README.md

48f3c81

Remove compare to true

c9157e0

Replace magic constant 14 with Ethernet header size

a45c7d7

Clarify comment on dummy XDP program

8d3738a

mihaibudiu merged commit 00d76bc into p4lang:main Dec 5, 2022

mihaibudiu mentioned this pull request Dec 8, 2022

XDP metadata support vmware-archive/p4c-xdp#136

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSA/ebpf backend: add support for generating programs for XDP hook #3752

PSA/ebpf backend: add support for generating programs for XDP hook #3752

tatry commented Dec 2, 2022

mihaibudiu Dec 2, 2022

osinstom Dec 3, 2022

mihaibudiu Dec 2, 2022

mihaibudiu Dec 2, 2022

mihaibudiu Dec 2, 2022

mihaibudiu Dec 2, 2022

osinstom Dec 3, 2022 •

edited

Loading

mihaibudiu Dec 2, 2022

mihaibudiu Dec 2, 2022

tatry Dec 5, 2022

mihaibudiu Dec 2, 2022

tatry Dec 5, 2022

mihaibudiu Dec 2, 2022

mihaibudiu Dec 2, 2022

PSA/ebpf backend: add support for generating programs for XDP hook #3752

PSA/ebpf backend: add support for generating programs for XDP hook #3752

Conversation

tatry commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osinstom Dec 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osinstom Dec 3, 2022 •

edited

Loading