[P4TC] Align key components to the nearest 8-bit size #5024

komaljai · 2024-11-21T13:24:09Z

As of now, whenever there is a key component that is a bit-type which doesn't correspond to primitive C type (u8, u16, u32, ...),
the compiler generates it as using the closest primitive C type that is larger then X in bit
For example -

bit<24> dest_op;

dest_op here is converted into u32 like below -

u32 dest_op;

This won't work, because the P4TC side is expecting the bit<24> (dest_op) to be contained in 3 bytes and not 4. All key components are expected to be 8-bit aligned. The new declaration for 'dest_op' would be -

u8 dest_op[3];

For all operations, compiler will call function 'getPrimitiveXX()' which will convert u8[] into primitive type (u32 or u64).
For reverse assignment operations compiler calls 'storePrimitiveXX()' which converts primitive types to u8 [ ] array.

@vbnogueira @jhsmt

vbnogueira

Some small compilation issues
Will do another round after running more complex tests

testdata/p4tc_samples_outputs/calculator_parser.c

testdata/p4tc_samples_outputs/calculator_parser.h

testdata/p4tc_samples_outputs/calculator_parser.c

testdata/p4tc_samples_outputs/add_entry_3_example_control_blocks.c

backends/tc/ebpfCodeGen.cpp

testdata/p4tc_samples_outputs/calculator_parser.h

vbnogueira · 2024-12-05T14:45:55Z

As we discussed on the meeting, here's an example that shows the sum bug sum_example.tar.gz
You'll see the bug in generated/sum_example_control_blocks.c line 85

vbnogueira · 2025-01-02T01:48:46Z

The PR looks ok, but can you rebase on top of main when you have some time?
There are fixes in the merged skb metadata PR which I need to test this PR with

komaljai · 2025-01-13T04:24:44Z

Hi @vbnogueira , @fruffy , Can you review the PR once?

vbnogueira · 2025-01-13T12:29:56Z

Hi @vbnogueira , @fruffy , Can you review the PR once?

Sorry, we got a bit overwhelmed, but will do today

vbnogueira

Aside from these small issues, the PR seems ok

testdata/p4tc_samples_outputs/add_entry_1_example_parser.h

vbnogueira

LGTM

komaljai · 2025-01-23T14:25:48Z

Hi @jafingerhut / @fruffy Could you review and approve the PR?

jafingerhut · 2025-01-23T14:58:23Z

I have no objections to merging these changes, as long as whoever is a stakeholder in the EBPF and P4TC backends is OK with them. I am currently so unfamiliar with the EBPF and P4TC back ends that I don't think I can review C++ code changes effectively. I took a quick look at the changes in the P4TC expected output files, and it appears the changes described in the initial comment of this PR have been made there.

Is there currently any automated testing of the P4TC back end, e.g. tests that compile a P4 program, load it into the kernel, add some table entries, send some packets in, and check that the expected packets come out (and no others)? If not, is anyone considering developing such automated tests as a way to test changes like this?

I see that this PR makes some changes to some ebpf p4c files, but does not make any changes to any expected EBPF output files. Is that because there are no EBPF expected output files in the p4c repo? Or because this PR should have no effect on the output of the p4c-ebpf compiler?

vbnogueira · 2025-01-28T19:49:02Z

Is there currently any automated testing of the P4TC back end, e.g. tests that compile a P4 program, load it into the kernel, add some table entries, send some packets in, and check that the expected packets come out (and no others)? If not, is anyone considering developing such automated tests as a way to test changes like this?

Yes, there is a PR we are currently finishing for that: #5011
In this case I also tested the PR manually as well

jafingerhut · 2025-01-28T20:42:23Z

Hi @vbnogueira , @fruffy , Can you review the PR once?

So I don't have a lot of stake in personally using the P4TC back end at this time, and I am not the best person to review C++ code in p4c's internals.

You have one approving review from someone who is more knowledgeable of, and invested in, P4TC, and I personally think that is enough for merging (assuming all of the CI tests pass, the PR is up to date with the main branch code, etc.)

I am glad you are working towards adding more automated CI tests for the P4TC back end!

vbnogueira · 2025-01-29T01:55:35Z

Hi @vbnogueira , @fruffy , Can you review the PR once?

So I don't have a lot of stake in personally using the P4TC back end at this time, and I am not the best person to review C++ code in p4c's internals.

You have one approving review from someone who is more knowledgeable of, and invested in, P4TC, and I personally think that is enough for merging (assuming all of the CI tests pass, the PR is up to date with the main branch code, etc.)

That's fair, but from what Komal told us she needs either your approval or Fabian's to be able to merge this PR

jafingerhut · 2025-01-29T02:30:37Z

Hi @vbnogueira , @fruffy , Can you review the PR once?

So I don't have a lot of stake in personally using the P4TC back end at this time, and I am not the best person to review C++ code in p4c's internals.
You have one approving review from someone who is more knowledgeable of, and invested in, P4TC, and I personally think that is enough for merging (assuming all of the CI tests pass, the PR is up to date with the main branch code, etc.)

That's fair, but from what Komal told us she needs either your approval or Fabian's to be able to merge this PR

OK. @komajai I'm pretty good for reviewing changes to test cases, but not really for a thorough review of C++ code changes, except in very localized parts of the p4c repo, and not at all for ebpf or tc back ends.

If Fabian is willing to review and approve, then great. If not, we could ask if @thomascalvert-xlnx might be willing to do so? I see he has done at least some work on the EBPF and TC back ends, which is more than I can say for me.

komaljai · 2025-01-29T09:03:36Z

Hi @vbnogueira , @fruffy , Can you review the PR once?

So I don't have a lot of stake in personally using the P4TC back end at this time, and I am not the best person to review C++ code in p4c's internals.
You have one approving review from someone who is more knowledgeable of, and invested in, P4TC, and I personally think that is enough for merging (assuming all of the CI tests pass, the PR is up to date with the main branch code, etc.)

That's fair, but from what Komal told us she needs either your approval or Fabian's to be able to merge this PR

OK. @komajai I'm pretty good for reviewing changes to test cases, but not really for a thorough review of C++ code changes, except in very localized parts of the p4c repo, and not at all for ebpf or tc back ends.

If Fabian is willing to review and approve, then great. If not, we could ask if @thomascalvert-xlnx might be willing to do so? I see he has done at least some work on the EBPF and TC back ends, which is more than I can say for me.

Hi @jafingerhut , This PR doesnt affect any ebpf testcases. And for the changes made in C++ code, there are testcases added along with the expected compiler output files. (The diff in output files reflect compiler changes due to current PR).
Like Victor mentioned above, he ran those testcases output files manually on his system as we don't have the automated framework to run testcases along with traffic yet for TC.

fruffy · 2025-01-29T13:16:51Z

backends/ebpf/codeGen.cpp

+        auto tcTarget = dynamic_cast<const P4TCTarget *>(builder->target);
+        auto ebpfType = EBPFTypeFactory::instance->create(ftype);
+        EBPFScalarType *scalar = nullptr;
+        if (ebpfType->is<EBPFScalarType>()) {


Recommend an return-based flow here instead of if else chains.

Or just a single visit(expression) call and a return in the !isPrimitive case.

fruffy · 2025-01-29T13:18:03Z

backends/ebpf/codeGen.cpp

-    return;
+bool CodeGenInspector::storeBitAlignment(const IR::Type *ltype, const IR::Expression *lexpr,
+                                         cstring lpath) {
+    auto tcTarget = dynamic_cast<const P4TCTarget *>(builder->target);


Do not use dynamic cast, use P4C's handlers in lib/castable.h. The target should already be castable.

Actually, this code should be in eBPFCodeGen for TC, if it needs to be casted to a P4TCTarget.

fruffy · 2025-01-29T13:18:32Z

backends/ebpf/codeGen.cpp

-    return;
+bool CodeGenInspector::storeBitAlignment(const IR::Type *ltype, const IR::Expression *lexpr,
+                                         cstring lpath) {
+    auto tcTarget = dynamic_cast<const P4TCTarget *>(builder->target);


Actually, this code should be in eBPFCodeGen for TC, if it needs to be casted to a P4TCTarget.

fruffy · 2025-01-29T13:24:49Z

backends/ebpf/codeGen.cpp

@@ -568,8 +567,19 @@ void CodeGenInspector::emitTCBinaryOperation(const IR::Operation_Binary *b, bool
    return;
 }

-void CodeGenInspector::emitTCAssignmentEndianessConversion(const IR::Expression *lexpr,
-                                                           const IR::Expression *rexpr) {
+void CodeGenInspector::emitTCAssignmentEndianessConversion(const IR::Type *ltype,


Hmm, these functions should ideally be in the TC back end not here. Makes it much harder refactor this back end later on. I guess the problem is that there is no TC-specific codegen derived from the CodeGenInspector.

It wasn't easy change as codeGenInspector is the base class for many classes in EBPF code, so we took this path to update in-place. But you pointed it correctly that refactoring of code would be difficult.

One solution @syogender suggested is that we treat CodeGenInspector as a base class for new class VendorCodeGenInspector. This new class will be define by each target in a separate file, which gets included depends on the target it is built for, that way we can have clean code with minimum intrusion.

Please suggest your inputs.

Yes that sounds great. I can approve this PR to unblock things and since there are already TC-specific functions in this class. But the next PRs should introduce such a refactoring.

@fruffy Could you approve the PR? I will create next PR with separate CodeGenInspector for TC.

Have you addressed the other comments?

Signed-off-by: Komal, Jain <[email protected]>

komaljai added the p4tc Topics related to the P4-TC back end label Nov 21, 2024

vbnogueira reviewed Nov 21, 2024

View reviewed changes

testdata/p4tc_samples_outputs/calculator_parser.c Outdated Show resolved Hide resolved

testdata/p4tc_samples_outputs/calculator_parser.h Outdated Show resolved Hide resolved

vbnogueira reviewed Nov 25, 2024

View reviewed changes

testdata/p4tc_samples_outputs/calculator_parser.c Outdated Show resolved Hide resolved

vbnogueira reviewed Nov 27, 2024

View reviewed changes

testdata/p4tc_samples_outputs/add_entry_3_example_control_blocks.c Show resolved Hide resolved

vbnogueira reviewed Dec 4, 2024

View reviewed changes

backends/tc/ebpfCodeGen.cpp Outdated Show resolved Hide resolved

backends/tc/ebpfCodeGen.cpp Outdated Show resolved Hide resolved

backends/tc/ebpfCodeGen.cpp Outdated Show resolved Hide resolved

backends/tc/ebpfCodeGen.cpp Outdated Show resolved Hide resolved

vbnogueira reviewed Dec 5, 2024

View reviewed changes

testdata/p4tc_samples_outputs/calculator_parser.h Outdated Show resolved Hide resolved

komaljai force-pushed the align_key_bytewise branch 2 times, most recently from cb20ec1 to e5a365e Compare December 12, 2024 10:27

komaljai force-pushed the align_key_bytewise branch from e5a365e to 889848e Compare December 19, 2024 14:31

komaljai force-pushed the align_key_bytewise branch 3 times, most recently from 998f749 to ffca7b6 Compare January 7, 2025 06:20

komaljai requested review from vbnogueira and fruffy and removed request for vbnogueira January 7, 2025 06:20

vbnogueira reviewed Jan 14, 2025

View reviewed changes

testdata/p4tc_samples_outputs/add_entry_1_example_parser.h Outdated Show resolved Hide resolved

testdata/p4tc_samples_outputs/add_entry_1_example_parser.h Outdated Show resolved Hide resolved

komaljai force-pushed the align_key_bytewise branch from ffca7b6 to 3f9f128 Compare January 16, 2025 13:10

komaljai requested a review from vbnogueira January 16, 2025 13:11

komaljai force-pushed the align_key_bytewise branch from 3f9f128 to 1234582 Compare January 16, 2025 13:12

vbnogueira approved these changes Jan 16, 2025

View reviewed changes

komaljai requested a review from jafingerhut January 23, 2025 14:25

komaljai force-pushed the align_key_bytewise branch from 1234582 to 6395a85 Compare January 23, 2025 14:26

fruffy approved these changes Jan 29, 2025

View reviewed changes

fruffy reviewed Jan 29, 2025

View reviewed changes

fruffy requested changes Jan 29, 2025

View reviewed changes

fruffy reviewed Jan 29, 2025

View reviewed changes

Align key components to the nearest 8-bit size

7f7134f

Signed-off-by: Komal, Jain <[email protected]>

komaljai force-pushed the align_key_bytewise branch from 6395a85 to 7f7134f Compare February 4, 2025 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[P4TC] Align key components to the nearest 8-bit size #5024

[P4TC] Align key components to the nearest 8-bit size #5024

komaljai commented Nov 21, 2024 •

edited

Loading

vbnogueira left a comment

vbnogueira commented Dec 5, 2024

vbnogueira commented Jan 2, 2025

komaljai commented Jan 13, 2025

vbnogueira commented Jan 13, 2025

vbnogueira left a comment

vbnogueira left a comment

komaljai commented Jan 23, 2025

jafingerhut commented Jan 23, 2025

vbnogueira commented Jan 28, 2025

jafingerhut commented Jan 28, 2025

vbnogueira commented Jan 29, 2025 •

edited

Loading

jafingerhut commented Jan 29, 2025 •

edited

Loading

komaljai commented Jan 29, 2025 •

edited

Loading

fruffy Jan 29, 2025

fruffy Jan 29, 2025

fruffy Jan 29, 2025 •

edited

Loading

fruffy Jan 29, 2025 •

edited

Loading

fruffy Jan 29, 2025

komaljai Jan 30, 2025

fruffy Jan 30, 2025 •

edited

Loading

komaljai Feb 4, 2025

fruffy Feb 4, 2025

[P4TC] Align key components to the nearest 8-bit size #5024

Are you sure you want to change the base?

[P4TC] Align key components to the nearest 8-bit size #5024

Conversation

komaljai commented Nov 21, 2024 • edited Loading

vbnogueira left a comment

Choose a reason for hiding this comment

vbnogueira commented Dec 5, 2024

vbnogueira commented Jan 2, 2025

komaljai commented Jan 13, 2025

vbnogueira commented Jan 13, 2025

vbnogueira left a comment

Choose a reason for hiding this comment

vbnogueira left a comment

Choose a reason for hiding this comment

komaljai commented Jan 23, 2025

jafingerhut commented Jan 23, 2025

vbnogueira commented Jan 28, 2025

jafingerhut commented Jan 28, 2025

vbnogueira commented Jan 29, 2025 • edited Loading

jafingerhut commented Jan 29, 2025 • edited Loading

komaljai commented Jan 29, 2025 • edited Loading

fruffy Jan 29, 2025

Choose a reason for hiding this comment

fruffy Jan 29, 2025

Choose a reason for hiding this comment

fruffy Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

fruffy Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

fruffy Jan 29, 2025

Choose a reason for hiding this comment

komaljai Jan 30, 2025

Choose a reason for hiding this comment

fruffy Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

komaljai Feb 4, 2025

Choose a reason for hiding this comment

fruffy Feb 4, 2025

Choose a reason for hiding this comment

komaljai commented Nov 21, 2024 •

edited

Loading

vbnogueira commented Jan 29, 2025 •

edited

Loading

jafingerhut commented Jan 29, 2025 •

edited

Loading

komaljai commented Jan 29, 2025 •

edited

Loading

fruffy Jan 29, 2025 •

edited

Loading

fruffy Jan 29, 2025 •

edited

Loading

fruffy Jan 30, 2025 •

edited

Loading