Implement EBPF deparser #3136

osinstom · 2022-03-16T07:45:12Z

This PR introduces a common, re-usable implementation of EBPF Deparser that can be used by different architecture models. The implementation is based on https://github.com/vmware/p4c-xdp.

Since ebpf_model.p4 doesn't define a deparser block, this PR doesn't affect its code. The main use case is PSA implementation for eBPF backend. It would be good to leverage the common deparser implementation in p4c-xdp at some point.

This PR is one from the series of PRs bringing the support for the PSA model to eBPF backend. The goal is to shrink the "main PR" with the full implementation of PSA for eBPF to facilitate review.

CC: @kmateuszssak @tatry

Co-authored-by: Mateusz Kossakowski <[email protected]> Co-authored-by: Jan Palimąka <[email protected]>

rst0git · 2022-03-16T13:56:46Z

@osinstom Would it be possible to add any test cases with this PR?

stuart-chu · 2022-03-16T16:45:12Z

@osinstom is this file also need to be updated ?
/backends/ebpf/p4include/ebpf_model.p4
cus I got error like this
ebpfFilter: 3 arguments supplied while 2 are expected
it's much more appreciated if some examples are presented.

osinstom · 2022-03-16T17:39:44Z

@rst0git what kind of tests do you have in mind?

@stuart-chu Please read the PR description carefully. This PR doesn't extend ebpf_model.p4 with deparser. This PR just contributes a piece of code that could be used for an architecture model that defines a deparser block. The driving use case is PSA implementation for eBPF, which will be contributed soon.

rst0git · 2022-03-16T21:52:00Z

what kind of tests do you have in mind?

It would be good if we could add a P4 program with STF test (e.g. rst0git@a5fdc50) to validate the functionality of the output code. However, it looks like such test would currently fail because p4c-ebpf doesn't apply the deparser code.

osinstom · 2022-03-17T08:00:05Z

@rst0git yes, the goal of this PR isn't to extend ebpf_model.p4 with deparser. The driving use case is PSA implementation for eBPF. We have almost 70 test cases based on the PTF framework that verify different mechanisms of PSA impl. We'll contribute all of them.

osinstom · 2022-03-17T08:00:50Z

@mbudiu-vmw could I get a review for this PR please?

osinstom · 2022-03-17T08:04:57Z

@rst0git BTW is the test passing on your branch? I'm asking out of curiosity. If there is a decision from ebpf_model maintainers (@mbudiu-vmw ?) to extend the architecture model with deparser, I can surely add it, but I want to avoid introducing breaking modifications to the already-defined arch model.

rst0git · 2022-03-17T08:13:56Z

is the test passing on your branch?

Yes, it is passing without STF test, i.e., the compiler works, but doesn't include deparser code in the output C program.

The driving use case is PSA implementation for eBPF.

I am happy to approve this PR but I am not sure I understand how this code will be used.
Would the PSA implementation use a different compiler, or p4c-ebpf would be compatible with both ebpf_model and PSA?

jonny-zzy · 2022-03-17T10:10:36Z

@osinstom If I want to use this code in ebpf_model for testing, can I modify the ebpf_model.p4 only or any other code which need to be modified?

osinstom · 2022-03-17T15:08:08Z

@rst0git p4c-ebpf will be compatible with both ebpf_model and PSA. Once this PR and #3137 are merged, we'll submit a PR with PSA impl based on the TC hook. You'll have a better understanding how this deparser can be used.

osinstom · 2022-03-17T15:11:21Z

@jonny-zzy You also need to extend ebpfProgram.cpp as follows:

extend build() to create EBPFDeparser object and assign IR::Parameter's based on the P4 package. Similarly as it is done for Parser and Control: https://github.com/p4lang/p4c/blob/main/backends/ebpf/ebpfProgram.cpp#L56
extend emitC with a line to call deparser->emit(), as it is done for parser and control: https://github.com/p4lang/p4c/blob/main/backends/ebpf/ebpfProgram.cpp#L98

mihaibudiu · 2022-03-17T15:38:06Z

@rst0git BTW is the test passing on your branch? I'm asking out of curiosity. If there is a decision from ebpf_model maintainers (@mbudiu-vmw ?) to extend the architecture model with deparser, I can surely add it, but I want to avoid introducing breaking modifications to the already-defined arch model.

The solution is not to modify ebpf_model.p4, but to introduce a new model, perhaps ebpf_switch.p4. This can be done in a similar way to xdp_model.p4. Note that p4c-xdp can compile programs for both models, and there is no reason that p4c-ebpf can't do that. It can detect which model is being used from the way main is instantiated.

mihaibudiu · 2022-03-17T17:08:38Z

backends/ebpf/ebpfDeparser.cpp

+
+bool DeparserPrepareBufferTranslator::preorder(const IR::BlockStatement* s) {
+    for (auto a : s->components) {
+        if (auto method = a->to<IR::MethodCallStatement>()) {


You already check the function name below, so it may be enough to just call visit(a) for MethodCallStatements.

mihaibudiu · 2022-03-17T17:10:20Z

backends/ebpf/ebpfDeparser.cpp

+    if (method->method->name.name == p4lib.packetOut.emit.name) {
+        auto decl = method->object;
+        if (decl == deparser->packet_out) {
+            auto expr = method->expr->arguments->at(0)->expression;


you should check that enough arguments exist.
The model seems to check that, but people can load the wrong model, and you don't want to crash.
Then you should call modelError.

mihaibudiu · 2022-03-17T17:11:36Z

backends/ebpf/ebpfDeparser.cpp

+            }
+            builder->blockEnd(true);
+        }
+    }


Is doing nothing for other methods the right thing?
Should there be an else error here?

mihaibudiu · 2022-03-17T17:12:55Z

backends/ebpf/ebpfDeparser.cpp

+                alignment %= 8;
+            }
+            builder->blockEnd(true);
+        }


and what if this is not packet_out?

does P4 language allow to call emit() for other fields than packet_out? Anyway, I'll produce a bug alert in such a case.

emit() is a method of the extern object packet_out, so no: https://github.com/p4lang/p4c/blob/main/p4include/core.p4#L60

yes, but no one precludes a program writer from defining their own extern with an emit method.
So you should check that it is the right extern and method.

mihaibudiu · 2022-03-17T17:14:25Z

backends/ebpf/ebpfDeparser.cpp

+        return;
+    }
+    unsigned widthToEmit = et->widthInBits();
+    unsigned loadSize = 0;


load?
perhaps this should be renamed

mihaibudiu · 2022-03-17T17:16:52Z

backends/ebpf/ebpfDeparser.cpp

+        BUG_CHECK((bitsToWrite > 0) && (bitsToWrite <= 8),
+                  "invalid bitsToWrite %d", bitsToWrite);
+        builder->emitIndent();
+        if (alignment == 0 && bitsToWrite == 8) {  // write whole byte


looking forward for tests that will validate this.
I got this kind of code several times wrong, so I don't trust myself to validate it by reading it.

yeah, this part is always tricky. We'll have tests for that.

mihaibudiu · 2022-03-18T17:41:36Z

backends/ebpf/ebpfDeparser.cpp

+            }
+            builder->blockEnd(true);
+        } else {
+            BUG("emit() should only be invoked for packet_out");


I think this should be an error, not a bug.

jonny-zzy · 2022-03-20T14:02:49Z

@jonny-zzy You also need to extend ebpfProgram.cpp as follows:

extend build() to create EBPFDeparser object and assign IR::Parameter's based on the P4 package. Similarly as it is done for Parser and Control: https://github.com/p4lang/p4c/blob/main/backends/ebpf/ebpfProgram.cpp#L56

extend emitC with a line to call deparser->emit(), as it is done for parser and control: https://github.com/p4lang/p4c/blob/main/backends/ebpf/ebpfProgram.cpp#L98

@osinstom
When I fellow this mothod, I found another place not fixed, Is there any places to modify?
/usr/local/bin/bin/p4c-ebpf ./example.p4 -o example.c
/usr/local/bin/share/p4c/p4include/ebpf_model.p4(70): [--Werror=expected] error: Expected toplevel package package ebpfFilter to have 2 parameters
package ebpfFilter(parse prs,
^^^^^^^^^^

ebpf_model.p4 like this:
/* architectural model for EBPF packet filter target architecture */

parser parse(packet_in packet, out H headers);
control filter(inout H headers, out bool accept);
control deparse(in H headers, out packet_out packet);

package ebpfFilter(parse prs,
filter filt,
deparse dprs);

rst0git · 2022-03-20T18:35:14Z

@jonny-zzy you need this change in backends/ebpf/ebpfProgram.cpp as well:
rst0git@a5fdc50#diff-c02a3f37e5b2624700d003c83df7aa6e130b3d5c3150c6dc54360faccb39dcd7L36

jonny-zzy · 2022-03-23T00:40:03Z

@osinstom @rst0git I modified the code to support ebpf_model.p4, and compiled .c file finally as expected. The p4 header only included ethernet, arp, ipv4, as the input parameter of deparser at the same time. clang-14 complier according to the compile option of -o2 or -o3, would exceed the limit stack of 512 bytes kernel, and I couldn't understand the compile optimization options, have you any sugestions? tks all.
p4 header
/*************************************************************************
*********************** H E A D E R ***********************************
*************************************************************************/
header ethernet_t {
macAddr_t dstAddr;
macAddr_t srcAddr;
bit<16> etherType;
}

header arp_t{
bit<16> hwType;
bit<16> protoType;
bit<8> hwAddrLen;
bit<8> protoAddrLen;
bit<16> opcode;
}

header arp_ipv4_t {
bit<48> hwSrcAddr;
bit<32> protoSrcAddr;
bit<48> hwDstAddr;
bit<32> protoDstAddr;
}

header ipv4_t {
bit<4> version;
bit<4> ihl;
bit<8> diffserv;
bit<16> totalLen;
bit<16> identification;
bit<3> flags;
bit<13> fragOffset;
bit<8> ttl;
bit<8> protocol;
bit<16> hdrChecksum;
ip4Addr_t srcAddr;
ip4Addr_t dstAddr;
}

struct headers {
@name("ethernet")
ethernet_t ethernet;
@name("arp")
arp_t arp;
arp_ipv4_t arp_ipv4;
@name("ipv4")
ipv4_t ipv4;
}

seems that if write_byte method is called multiple times, the origin stack of struct header will be enlarged for optimization.

mihaibudiu · 2022-03-23T00:50:02Z

Writing comments on a merged pull request is not the best solution; you should probably open a new issue.
In the past there were problems with the stack because llvm would use 8 bytes for each byte of data that was used as a local variable. I don't know if this has been solved in the ebpf llvm back-end.

Implement EBPF deparser

0d345e0

Co-authored-by: Mateusz Kossakowski <[email protected]> Co-authored-by: Jan Palimąka <[email protected]>

rst0git approved these changes Mar 17, 2022

View reviewed changes

mihaibudiu approved these changes Mar 17, 2022

View reviewed changes

address review

8143604

mihaibudiu reviewed Mar 18, 2022

View reviewed changes

mihaibudiu merged commit 9200532 into p4lang:main Mar 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement EBPF deparser #3136

Implement EBPF deparser #3136

osinstom commented Mar 16, 2022

rst0git commented Mar 16, 2022

stuart-chu commented Mar 16, 2022 •

edited

Loading

osinstom commented Mar 16, 2022

rst0git commented Mar 16, 2022

osinstom commented Mar 17, 2022

osinstom commented Mar 17, 2022

osinstom commented Mar 17, 2022

rst0git commented Mar 17, 2022 •

edited

Loading

jonny-zzy commented Mar 17, 2022

osinstom commented Mar 17, 2022

osinstom commented Mar 17, 2022

mihaibudiu commented Mar 17, 2022

mihaibudiu Mar 17, 2022

mihaibudiu Mar 17, 2022

mihaibudiu Mar 17, 2022

mihaibudiu Mar 17, 2022

osinstom Mar 18, 2022

jafingerhut Mar 18, 2022

mihaibudiu Mar 18, 2022

mihaibudiu Mar 17, 2022

mihaibudiu Mar 17, 2022

osinstom Mar 18, 2022

mihaibudiu Mar 18, 2022

jonny-zzy commented Mar 20, 2022

rst0git commented Mar 20, 2022

jonny-zzy commented Mar 23, 2022

mihaibudiu commented Mar 23, 2022

Implement EBPF deparser #3136

Implement EBPF deparser #3136

Conversation

osinstom commented Mar 16, 2022

rst0git commented Mar 16, 2022

stuart-chu commented Mar 16, 2022 • edited Loading

osinstom commented Mar 16, 2022

rst0git commented Mar 16, 2022

osinstom commented Mar 17, 2022

osinstom commented Mar 17, 2022

osinstom commented Mar 17, 2022

rst0git commented Mar 17, 2022 • edited Loading

jonny-zzy commented Mar 17, 2022

osinstom commented Mar 17, 2022

osinstom commented Mar 17, 2022

mihaibudiu commented Mar 17, 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

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

jonny-zzy commented Mar 20, 2022

rst0git commented Mar 20, 2022

jonny-zzy commented Mar 23, 2022

mihaibudiu commented Mar 23, 2022

stuart-chu commented Mar 16, 2022 •

edited

Loading

rst0git commented Mar 17, 2022 •

edited

Loading