p4c-ubpf: new uBPF back-end for p4c #2134

osinstom · 2020-01-10T15:00:22Z

This PR implements the uBPF (Userspace BPF) backend for the P4 compiler

The p4c-ubpf compiler allows to translate P4 programs into the uBPF programs.

The backend for uBPF is mostly based on p4c-ebpf, but generates the C code, which is compatible with the user space BPF implementation. Moreover, it has support for P4 registers and programmable actions (packet's modifications + tunneling).

We tested p4c-ubpf with the Oko switch [1], but the uBPF Virtual Machine can be used in any software switch implementing the kernel bypass (e.g. DPDK apps).

Basic usage:

# compile P4 program to C code
$ p4c-ubpf -o test.c PROGRAM.p4 
# compile test.c to BPF machine code
$ clang-6.0 -O2 -I .. -target bpf -c test.c -o /tmp/test.o

[1] https://github.com/Orange-OpenSource/oko/tree/p4rt-ovs

mihaibudiu · 2020-01-10T18:15:27Z

I took a quick look at the PR, it looks pretty good.
I think that the main thing missing is integrating your tests in the existing test suite.
The tests can be run at several levels: just compiling - and this we should always to - and running the programs on generated traffic. Since you depend on a lot of infrastructure these tests may be disabled by default - like the ebpf kernel tests are today.

I don't know if we can setup Travis to run Vagrant, DPDK and all the other stuff necessary for your tests. If we can, we should add your tests to the continuous integration, otherwise it's very easy for them to get broken by some random commit elsewhere.

For just the compiler tests we could add your files to testdata/p4_16_samples, like we do with the ebpf tests (I am assuming they can be compiled with p4test); we could also suffix all your test programs with something like -ubpf.p4, which would make it easy for the test scripts to pick up only your tests for running them.

We can leave your test programs where they are as well, but then it would still be nice for make check to run p4c-ubpf on your programs at the very least.

I will start reviewing your contribution, but it may take a while. Perhaps I will submit multiple reviews so you can start addressing the issues one by one.

mihaibudiu · 2020-01-10T18:17:37Z

I also assume that you are committing to maintaining this implementation and fixing bugs that people may report for the foreseeable future.

osinstom · 2020-01-11T15:16:45Z

Ok. We will work to integrate our tests with existing test suite. We'll take a look at how it is done for ebpf and, then, prepare a separate commit.

I also assume that you are committing to maintaining this implementation and fixing bugs that people may report for the foreseeable future.

Definitely! We devote some time to maintaining p4c-ubpf.

jnfoster · 2020-01-11T16:44:05Z

Just to be clear, I think what Mihai is proposing is to split the testing into: - Compile-only tests, which can be run whether or not the backend is installed - uBPF-specific tests, which would run if the backend were installed Cheers, -N On Sat, Jan 11, 2020 at 10:16 AM Tomek Osiński <[email protected]<mailto:[email protected]>> wrote: Ok. We will work to integrate our tests with existing test suite. We'll take a look at how it is done for ebpf and, then, prepare a separate commit. I also assume that you are committing to maintaining this implementation and fixing bugs that people may report for the foreseeable future. Definitely! We devote some time to maintaining p4c-ubpf. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#2134?email_source=notifications&email_token=AAOCFUQNTRZC23SQNEGTWILQ5HPF7A5CNFSM4KFJABPKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWEBBY#issuecomment-573325447>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAOCFUQ7DTQ5DUXV56OWVV3Q5HPF7ANCNFSM4KFJABPA>.

mihaibudiu

I will continue reviewing in a separate comment.
Your testing infrastructure is very good.

backends/ubpf/README.md

mihaibudiu · 2020-01-13T19:17:44Z

backends/ubpf/README.md

+This repository implements the uBPF (Userspace BPF) backend for the P4 compiler (https://github.com/p4lang/p4c).
+
+The **p4c-ubpf** compiler allows to translate P4 programs into the uBPF programs. We use the uBPF implementation provided 
+by [the Oko/P4rt-OVS switch](https://github.com/Orange-OpenSource/oko/tree/p4rt-ovs). The Oko's uBPF VM is based on the


is this a fork of ovs? Do you expect that this fork will be merged within OvS?

Yep, this is a fork of OVS. I presented the details at OVSCon 19' [1]. There are no official plans to merge it with OVS yet. It is still a research prototype, but once it will be mature enough we can consider to propose such a contribution to OVS.

However, as far as I know, P4rt-OVS is the only switch to test uBPF back-end.

[1] https://www.openvswitch.org/support/ovscon2019/#s4.3F

mihaibudiu · 2020-01-13T19:24:39Z

backends/ubpf/docs/EXAMPLES.md

+- `key_type` - is bit array type (i.e. bit<32>) or struct like type  
+- `number_of_elements` - the maximum number of key-value pairs
+
+Currently registers have a limitation - they are not being initialized with default values. Initialization has to be done by a control plane. 


P4 does not have a way to initialize registers, but your architecture can allow it if you want.
If there is a way in your architecture to initialize registers to default values before the start of the program you can promise this to your users. But at least for the in-kernel bfp I don't think there is a way to do it.

We analyzed different design options to initialize BPF maps (P4 registers) before the start of the program. And we decided that intializing them from a control plane is the best option for the moment. We could think about better approach later.

mihaibudiu · 2020-01-13T19:26:03Z

backends/ubpf/docs/EXAMPLES.md

+
+This is very simple example of stateful firewall. Every TCP packet is analyzed to track the state of the TCP connection. 
+If the traffic belongs to known connection it is passed. Otherwise, it is dropped.  
+Notice that the example program uses hash function which is constrained to hash only 64 bit values - that's why TCP connection is identified via IP source and destination address.  


To make it clear: this is a constraint that you have chosen. In P4 you could even declare a generic hash<T>(in T data) which works for any P4 datatype.

Yep, that's the limitation of our implementation. In particular, it is the limitation of the ubpf_hash() helper that we use to compute hash value. Also, to be improved in the future.

backends/ubpf/examples/gtp.p4

backends/ubpf/p4c-ubpf.cpp

mihaibudiu · 2020-01-13T20:40:38Z

backends/ubpf/p4include/ubpf_model.p4

+                filter<H, M> filt,
+                deparser<H> dprs);
+
+extern void mark_to_drop();


I would suggest documenting these functions.

In particular, for mark_to_drop() and mark_to_pass(), what is the default behavior after a packet is done processing? Probably pass?

Then if the P4 program does mark_to_drop() and/or mark_to_pass() calls, whichever one was last determines whether the packet is passed or dropped later?

And there are no user-visible P4 variables that are modified by mark_to_drop() or mark_to_pass()? I would not ask, except this was an issue for several years with v1model's mark_to_drop(), and why it was changed to take a parameter later. It appears you have nothing that corresponds to standard_metadata, so it would make sense if in your architecture, mark_to_drop() and mark_to_pass() only modify state "hidden from the user's P4 program" inside the architecture implementation.

Another good thing to document about these extern functions is which of the architecture's parsers/controls they are allowed to be called in, e.g. maybe you intended something like "mark_to_drop() and mark_to_pass() may only be called in the filter control".

Comments in the p4include/v1model.p4 file would be good to look at for examples. If any of them happen to match the behavior you provide (e.g. that might be true for your Register extern compared to v1model's register extern), feel free to copy and paste whatever is accurate for your architecture.

backends/ubpf/tests/environment/setup-pktgen.sh

mihaibudiu · 2020-01-13T20:48:21Z

backends/ubpf/tests/environment/setup-switch.sh

+# Install clang-10
+if ! type "/vagrant/llvm-project/build/bin/clang" > /dev/null; then
+  cd /vagrant
+  git clone https://github.com/P4-Research/llvm-project


so you have your own clone of llvm as well?
it would be nice to use an off-the-shelf version

Yep, we are aware of that.

This is related to the limitation of BPF itself. It was also discussed in regards of p4c-xdp [1]. As we are in the userspace, we can simply increase the BPF's stack size. However, it requires to have a new flag in clang compiler (this is also on our TODO list), as proposed in this thread [2].

Currently, we have our fork of clang / llvm, in which the stack size is increased and hardcoded.

[1] vmware-archive/p4c-xdp#22
[2] http://lists.llvm.org/pipermail/cfe-users/2017-March/001126.html

backends/ubpf/tests/testdata/oko-test-actions.p4

mihaibudiu

There seems to be some code duplication with the ebpf back-end, but I don't think it will be easy to fix that.
Ideally your code should have subclassed the various ebfp classes and reused most methods that can be resued, but I understand that this is very difficult if you want to make any changes in the base classes.

It's probably OK to have the code duplication, although some bugs may need to be fixed multiple times.

I can't promise that I read every line very carefully, because there are lots of them, but hopefully your testing will cover the gaps that I had in my reading.

I will request some changes, although we could probably merge as it is an do the changes later too.

backends/ubpf/ubpfControl.cpp

mihaibudiu · 2020-01-14T16:47:28Z

backends/ubpf/ubpfControl.h

+    public:
+        const UBPFControl *control;
+        std::set<const IR::Parameter *> toDereference;
+        std::vector<cstring> saveAction;


Can you please add a comment explaining this field?

It seems to be the last action executed.

This is used to implement action_run. It stores the action that was executed. This approach is based on and taken from p4c-ebpf.

backends/ubpf/ubpfControl.cpp

backends/ubpf/ubpfRegister.cpp

backends/ubpf/ubpfProgram.h

backends/ubpf/ubpfProgram.cpp

osinstom · 2020-01-15T06:56:47Z

@mbudiu-vmw thank you for your comments! I'll address them soon. We also started to prepare compile-only tests. Currently, in parallel, we work on a demo showcase for our internal purposes, so it grabs us a part of time and we cannot work intensively on PR, sorry for that!

osinstom · 2020-01-17T15:31:10Z

I've just submitted a small commit (with some minor modifications) addressing some of your comments. In particular, there was some code, which was useless, but remained in the files.

The rest of comments we will address in the next week.

mihaibudiu · 2020-01-21T18:24:26Z

testdata/p4_16_samples_outputs/ipv4-actions_ubpf.c

@@ -0,0 +1,358 @@
+/* Automatically generated by p4c-ubpf from /home/mateusz/p4c/testdata/p4_16_samples/ipv4-actions_ubpf.p4 on Mon Jan 20 15:27:07 2020


Do you want to save these files in the repository?
Are you comparing with them in some tool?
The ebpf back-end does not save the C files - I was afraid that they would change too often with compiler changes to make for good tests, but perhaps that's wrong.

You're it is not the best way, but I think it is the only reasonable (at least for now) way to test if the C files generated by p4c-ubpf are correct. We compare them at very basic level, using a minimalistic Python code.

We are aware that it could require changes to these file once we will add some new functionality to p4c-ubpf. I would keep these files for now, until we will integrate P4rt-OVS (or some other uBPF verifier) with make check-ubpf.

We've removed these files and replaced them by userspace-only tests (based on those implemented for ebpf)

osinstom · 2020-01-30T09:19:14Z

@mbudiu-vmw I think we addressed most of your comments. Build was successful and all tests passed. There are two actions on our TODO list for next days:

implement checksum compuation based on p4c-xdp
refactor packet size's check, as it needs to be more sophisticated. Current approach is too generic as it compares pkt_len with the size of the whole Headers_t structure.

I'll update you once we will implement this two features.

osinstom · 2020-02-12T15:43:35Z

@mbudiu-vmw Could you guide me how to replicate test environment of Travis locally? I'm running tests locally and they are passed, but once I push them to master, Travis-generated tests fail. I use ./bootstrap.sh script with different options for UNIFIED and GMP, but environments still seems to be different.

mihaibudiu · 2020-02-12T17:36:00Z

You have 3 failures in your Travis tests: https://travis-ci.org/p4lang/p4c/jobs/649504069?utm_medium=notification&utm_source=github_status
One of them is a compiler crash, which you should be able to reproduce without any special setup.
The other two are differences in the output files produced by the compiler that you have committed; probably you have to update these files for the tests to pass.

* Registers, get_timestamp extern, rate limiter example. * Remove unnecessary includings. * Move register class to separated file and other small fixes.

Take size of table from P4 code.

…itional nullptr check in UBPFDeparser.

…rol') by removing dead code

osinstom · 2020-02-20T14:52:04Z

@mbudiu-vmw Finally, we've made all modifications and tests are passed. I think we can move on with review and merge.

jnfoster · 2020-02-20T15:01:28Z

🚀

mihaibudiu · 2020-02-20T16:10:23Z

Great, I will try to review it next week.

mihaibudiu

I haven't re-read all the code, I mostly checked to see whether the comments from the previous review have been addressed. I think we can merge this.

backends/ubpf/README.md

backends/ubpf/examples/packet-counter.p4

backends/ubpf/examples/vxlan.p4

mihaibudiu · 2020-02-24T21:06:04Z

backends/ubpf/target.cpp

+    }
+
+    void UbpfTarget::emitUbpfHelpers(EBPF::CodeBuilder *builder) const {
+        builder->append(


this looks a bit brittle. Is there a guarantee that the numbers associated with these functions will never change?

This is a strict requirement, which is specific to target that we use (P4rt-OVS). Perhaps, it could be generalized in the future, but at the moment the BPF program would not work otherwise.

backends/ubpf/ubpfDeparser.cpp

mihaibudiu · 2020-02-24T21:17:21Z

backends/ubpf/ubpfTable.cpp

+                    } else if (auto block = s->to<IR::BlockStatement>()) {
+                        convertActionBody(&block->components);
+                        continue;
+                    } else if (s->is<IR::ReturnStatement>()) {


Do you want to put here a BUG_CHECK that no return or exit is encountered?

I shouldn't put BUG_CHECK here, because retrun or exit could be encountered here (as both exit and return are allowed for actions according to specs). However, we use this else if block to implement a proper behavior, i.e. don't emit any statements following exit or return.

mihaibudiu · 2020-03-04T18:10:56Z

Since there are no other comments I will go ahead and merge this huge PR.

jnfoster · 2020-03-05T04:24:35Z

Yay!

mihaibudiu reviewed Jan 13, 2020

View reviewed changes

mihaibudiu requested changes Jan 14, 2020

View reviewed changes

mihaibudiu reviewed Jan 21, 2020

View reviewed changes

osinstom and others added 18 commits February 14, 2020 15:30

Initial phase of implementing the p4c-ubpf compiler

ecd39c0

Adding support for UBPF parser

ec22005

UBPF Parser - first implementation

a687110

The UBPF parser implemented

75fdcf6

UBPF initial control block.

2fcdcf8

Working Ubpf control block.

acfaa6e

Change ubpf table name.

99d6854

Small fixes

78335fa

Remove accept parameter from reference model.

93643c2

Change table key names.

118b2b2

Change table key names v2.

884c40a

Some fixes to the p4c-ubpf + docs

d57742f

Copyright headers added

dcf0903

Update README.md

3d64116

Packet modifications support

9231f53

Take size of table from P4 code.

dbb237e

registers_support (#4)

1090527

* Registers, get_timestamp extern, rate limiter example. * Remove unnecessary includings. * Move register class to separated file and other small fixes.

Merge pull request #5 from P4-Research/master2

c0bc2c5

Take size of table from P4 code.

osinstom and others added 22 commits February 14, 2020 15:30

Documentation for architecture model + code cleanup

1fd859e

Remove dependencies on BMv2 helpers.h

08b7de0

Refactor of hash() generation. Generate tuple from Type_List.

b8b3f06

Remove useless code

e22ad6f

Move metadata to tests

38acee5

Path expression code refactor

c648326

Add check register type

c5c324e

Refactor method to generate register read()

7f0b72e

Small fixes to test scripts and UBPFRegister

2b6020c

Refactor pointer variables' handling in UBPFControl/UBPFRegister. Add…

997d5db

…itional nullptr check in UBPFDeparser.

Fix build error (invalid use of incomplete type 'class UBPF::UBPFCont…

db6c4db

…rol') by removing dead code

Refactor Exit and Return statement's handling

2e12ee3

Implement checksum computation

6bc8674

Optimizations of test environment installation script

7ec6f33

Add missing test file

2be7ec7

Refactor check of packet's length in Parser

b1479ad

User-space only tests (#18)

668e808

Fix p4c-ubpf error.

6ae22e4

Fix test errors related to frontend

cc59b0d

Register read refactor (#19)

fb07d84

Macro redefinition

daad335

Update travis.yml to exclude ubpf tests for OSx

f5bcc01

mihaibudiu approved these changes Feb 24, 2020

View reviewed changes

Small code refactor and update of docs

89eb992

mihaibudiu merged commit 5a06a3c into p4lang:master Mar 4, 2020

		@@ -0,0 +1,358 @@
		/* Automatically generated by p4c-ubpf from /home/mateusz/p4c/testdata/p4_16_samples/ipv4-actions_ubpf.p4 on Mon Jan 20 15:27:07 2020

p4c-ubpf: new uBPF back-end for p4c #2134

p4c-ubpf: new uBPF back-end for p4c #2134

Conversation

osinstom commented Jan 10, 2020

mihaibudiu commented Jan 10, 2020

mihaibudiu commented Jan 10, 2020

osinstom commented Jan 11, 2020

jnfoster commented Jan 11, 2020 via email

mihaibudiu left a 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

osinstom Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

mihaibudiu left a 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 commented Jan 15, 2020

osinstom commented Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osinstom commented Jan 30, 2020 • edited Loading

osinstom commented Feb 12, 2020

mihaibudiu commented Feb 12, 2020

osinstom commented Feb 20, 2020

jnfoster commented Feb 20, 2020

mihaibudiu commented Feb 20, 2020

mihaibudiu left a 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

mihaibudiu commented Mar 4, 2020

jnfoster commented Mar 5, 2020

osinstom Jan 17, 2020 •

edited

Loading

osinstom commented Jan 17, 2020 •

edited

Loading

osinstom commented Jan 30, 2020 •

edited

Loading