-
Notifications
You must be signed in to change notification settings - Fork 453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
uBPF backend: Support for forwarding #2381
Merged
Merged
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
792fc7a
Support for port forwarding
osinstom 8968675
Tests for port forwarding
osinstom 1d3e267
Fix tests
osinstom 3dc2c5c
Fix travis build error
osinstom 4be07ca
ubpf_model: specify version of uBPF arch model
osinstom c85e2d9
Fix CI tests
osinstom c2c0ae8
Upgrade default version of uBPF model
osinstom File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
ubpf_model: specify version of uBPF arch model
- Loading branch information
commit 4be07ca50836f2df0a01d5238a862dd17d2853b9
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
#include <core.p4> | ||
#define UBPF_MODEL_VERSION 20200515 | ||
#include <ubpf_model.p4> | ||
|
||
struct Headers_t { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make such changes they will break the user programs that were depending on your prior architecture.
Perhaps you can follow the model of v1model.p4, which has a version and conditional changes?
You should add a version anyway, whether you want to conditionally add this additional argument and handle it in the compiler is your choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point! I've added version to
ubpf_model.p4
. However, it seems thatmake check-p4
does not see macro's definition from test files, for example (e.g. p4_16_samples/tunneling_ubpf.p4):Is there any specific workaround for v1model.p4 to let tests pass ? I don't observe this issue with
make check-ubpf
or standalone compilation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler inserts a #define of V1MODEL_VERSION when translating P4_14 to P4_16, but that should not be relevant here. Other than that, there's no special handling of V1MODEL_VERSION needed -- if it is set explicitly in a test file, that will override the default in the header file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, that hints at the problem -- the toP4 dumping code will insert a
#define V1MODEL_VERSION
in the dumped code if it is needed -- you may need something similar for this. See frontends/p4/toP4/toP4.cpp line 172; there's a check when dumping an#include <v1model.p4>
line; you may need something similar for#include <ubpf_model.p4>
. We should think about a way to make this easier to extend in a backend (a hook to the backend that is called from here, rather than having explicit checks for backend architectures.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like a dirty hack to me and it would require to add dependency on uBPF model in the P4 frontend code (what I think we should avoid). I think we can break the backward compatibility for the uBPF programs, but explicitly say users that the old programs must be updated or the old version must be defined by macro definition. I've added the note in the documentation: https://github.com/p4lang/p4c/pull/2381/files#diff-616b1ea3f7f6e6e72e5cb1e8d44899dd. I've also updated examples for uBPF.
Does it sound acceptable to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this, but we should find a portable way to do versioning.