Skip to content
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

Enable variable-width extraction for the TC backend #4695

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vbnogueira
Copy link
Contributor

Enable variable-width extraction for the TC backend.
However note that it still doesn't add varbit support for the deparser, only for the parser

@fruffy fruffy added the p4tc Topics related to the P4-TC back end label May 31, 2024
@vbnogueira
Copy link
Contributor Author

@Sosutha @komaljai

@komaljai komaljai requested a review from Sosutha June 3, 2024 05:01
Copy link
Contributor

@Sosutha Sosutha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add example testcases

Enable variable-width extraction for the TC backend
However note that it still doesn't add varbit support for the deparser
@vbnogueira vbnogueira force-pushed the varbit_extract_support branch 2 times, most recently from 511bab5 to 3e4fd20 Compare June 4, 2024 14:54
@vbnogueira
Copy link
Contributor Author

Please add example testcases

Added testcase crafted by mouse

Copy link
Contributor

@Sosutha Sosutha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes are done in common ebpf code, Do we need to do these changes explicitly for tc backend? or applicable to ebpf backend as well? If the changes are only for tc backend, then changes have to be done in different way by creating tc specific children classes

@@ -0,0 +1,159 @@
/* -*- P4_16 -*- */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this testcase to the CMakelist in backends/tc folder and generate output files from compiler and place them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per your suggestion, modified simple_exact_example.p4 instead of creating a new test case so that the diff is easier to see

@fruffy fruffy added the run-sanitizer Use this tag to run a Clang+Sanitzers CI run. label Jun 6, 2024
Modify simple_exact_example to parse ipv4 options
Intent is to make it easier for reviewers to understand the changes
added by the previous commit
@Sosutha
Copy link
Contributor

Sosutha commented Jun 10, 2024

Code changes are done in common ebpf code, Do we need to do these changes explicitly for tc backend? or applicable to ebpf backend as well? If the changes are only for tc backend, then changes have to be done in different way by creating tc specific children classes

Any updates on this point?

unsigned alignment = hdrOffsetBits % 8;
unsigned widthToExtract = type->as<IHasWidth>().widthInBits();
auto program = state->parser->program;
cstring msgStr;
cstring fieldName = field->name.name;

builder->appendFormat("/* EBPF::StateTranslationVisitor::compileExtractField: field %s",
Copy link
Contributor

@Sosutha Sosutha Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are additional comments generated in the code. Do we really need this change in the code?

@vbnogueira
Copy link
Contributor Author

Code changes are done in common ebpf code, Do we need to do these changes explicitly for tc backend? or applicable to ebpf backend as well? If the changes are only for tc backend, then changes have to be done in different way by creating tc specific children classes

Any updates on this point?

Not yet, I forwarded your comments to mouse
He'll be back to work tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4tc Topics related to the P4-TC back end run-sanitizer Use this tag to run a Clang+Sanitzers CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants