Support for 128 bit constants in dpdk backend #5034

maheswari-s · 2024-11-26T09:32:49Z

Added support for 128 bit constants usage in dpdk backend
Updated testcases and outputs

This PR overwrites the PR #5009

psivanup

Looks fine

maheswari-s · 2024-11-26T09:54:08Z

@fruffy , @syogender, @psivanup, @jafingerhut : Please review this PR for 128 bit implementation.
I addressed the issue raised by Andy in PR #5009. Addressed the issue as discussed with Cristian.

jafingerhut · 2024-12-01T19:30:38Z

It looks like some expected output files are missing, according to the failure of the test-p4c-debian / test-ubuntu22 test report. Can you please add those?

syogender

Looks Good

maheswari-s · 2024-12-02T12:23:37Z

@fruffy , @syogender, @psivanup, @jafingerhut : All checks have passed. Please review and approve.

fruffy · 2024-11-26T13:17:19Z

backends/dpdk/dpdkHelpers.h

@@ -37,6 +37,8 @@ limitations under the License.

 namespace P4::DPDK {

+static const int SupportedBitWidth = 128;


Suggested change

static const int SupportedBitWidth = 128;

namespace DpdkConstants {

static constexpr int SupportedBitWidth = 128;

}

fruffy · 2024-12-02T13:24:04Z

backends/dpdk/dpdkUtils.cpp

@@ -151,15 +151,13 @@ bool reservedNames(P4::ReferenceMap *refMap, const std::vector<cstring> &names,
    return true;
 }

-/// Update bitwidth of Metadata fields to 32 or 64 bits if it 8-bit aligned.
+/// The DPDK pipeline requirement is all header/metadata fields to be multiple of 8bits.


The BMv2 back end has a pass that throws an error when a type does not have the correct bit-width. Do the same thing here? Also isn't this restriction for total width? What if you have a 4-bit version field like in the IPv4 header?

I discussed with Cristian and he told that all header/metadata/variables should be multiple of 8bits. There may be sub fields within 8bit like IPV4 headers. That compiler handles as the 8bit are finally covered and all headers are always multiple of 8bits.

If user creates structure, for example with 3 fields as 24bits, 3bits, 32bits, here the 3bits field is not supported. For this, Cristian advises to issue error. But in compiler, i need some more time to do that. So, as of now, i am rounding to nearest 8bit multiples if fields are odd like in this example. In case if fields are 8bit, 2bits, 3bits, 1bit, 2bits, here compiler can pack the last 4fields into one 8bit field and will not round them individually.

I will address the error issuance separately. Currently, rounding to nearest 8bits is fine.

I will address the error issuance separately. Currently, rounding to nearest 8bits is fine.

Will this lead to incorrect processing? Or does it just waste resources? If it is the former it is a bug and should not be merged.

The BMv2 back end checks for headers like this fwiw: https://github.com/p4lang/p4c/blob/main/backends/bmv2/common/header.cpp#L257

testdata/p4_16_samples_outputs/pna-example-varIndex-1.p4.spec

jafingerhut · 2024-12-02T14:23:09Z

testdata/p4_16_dpdk_errors_outputs/psa-dpdk-128bitCast_error.p4-error

@@ -0,0 +1,3 @@
+psa-dpdk-128bitCast_error.p4(55): [--Werror=overlimit] error: DPDK target supports up-to 64-bit immediate values, 80w0xffffbbbb12345342aaaa exceeds the limit


I am a bit confused by this error message. These changes are to enable support in DPDK back end for 128-bit immediate values, yes?

If so, then the error message seems misleading. It seems best either to (a) support this assignment, or (b) give a more precise error message, whatever the restriction might actually be. Is the restriction given these changes something like "DPDK target supports immediate values that are up to 64-bit, and also exactly 128-bit, but no other sizes. 80w0xffffbbbb12345342aaaa is not a supported size" ?

Yes Andy. Constants supported are within 64bits or only 128bits. I rephrased the error as you have given. Please check now.

jafingerhut · 2024-12-03T17:13:53Z

Maheswari, we currently do not have any automated tests in this repo for trying to pass packets through P4-DPDK that consume the output of p4c-dpdk and check whether it seems to be operating correctly while processing packets. The .spec files output for the test programs here are a bit tedious for me to check by hand. Have you or someone else checked the .spec output files by hand and they look correct to you? Or have you run the .spec and other p4c-dpdk output files on a P4-DPDK software switch and validated that it is behaving as the P4 source program says it should?

fruffy · 2024-12-05T15:48:38Z

Maheswari, we currently do not have any automated tests in this repo for trying to pass packets through P4-DPDK that consume the output of p4c-dpdk and check whether it seems to be operating correctly while processing packets. The .spec files output for the test programs here are a bit tedious for me to check by hand. Have you or someone else checked the .spec output files by hand and they look correct to you? Or have you run the .spec and other p4c-dpdk output files on a P4-DPDK software switch and validated that it is behaving as the P4 source program says it should?

Note that we had a CI workflow to run DPDK PTF tests, but we disabled it: https://github.com/p4lang/p4c/blob/main/.github/workflows/disabled/ci-dpdk-ptf-p4testgen-tests.yml

The tests were generated by P4Testgen because we didn't have any packet tests available.

The reason we disabled it was that the DPDK test harness had stability problems and we were unable to find the root cause. Still, I do not think there is much work left to do to reinstate this.

jafingerhut · 2024-12-05T16:12:05Z

Maheswari, we currently do not have any automated tests in this repo for trying to pass packets through P4-DPDK that consume the output of p4c-dpdk and check whether it seems to be operating correctly while processing packets. The .spec files output for the test programs here are a bit tedious for me to check by hand. Have you or someone else checked the .spec output files by hand and they look correct to you? Or have you run the .spec and other p4c-dpdk output files on a P4-DPDK software switch and validated that it is behaving as the P4 source program says it should?

Note that we had a CI workflow to run DPDK PTF tests, but we disabled it: https://github.com/p4lang/p4c/blob/main/.github/workflows/disabled/ci-dpdk-ptf-p4testgen-tests.yml

The tests were generated by P4Testgen because we didn't have any packet tests available.

The reason we disabled it was that the DPDK test harness had stability problems and we were unable to find the root cause. Still, I do not think there is much work left to do to reinstate this.

Yes, automated CI tests would be the ideal long term solution.

Even in the absence of those, it seems reasonable to ask whether any manual tests have been run on these particular changes, to look for any bugs that such testing might find.

digesh099 · 2024-12-06T05:52:36Z

testdata/p4_16_samples/pna-dpdk_128bit_odd_size.p4

+    inout pna_main_output_metadata_t ostd)
+{
+    action a1(bit<73> x) {
+        hdr.custom.f128 = (bit<128>) x;


@maheswari-s @jafingerhut - I hope this is the line fix is addressing. I can manually run in my dpdk setup and post the packet processing result. please provide the correct case if this is error case

That is the line of code that the last commit to this PR is fixing. The DASH test program has several expressions involving 128-bit constants that this PR is also addressing.

maheswari-s · 2024-12-17T14:33:13Z

Maheswari, we currently do not have any automated tests in this repo for trying to pass packets through P4-DPDK that consume the output of p4c-dpdk and check whether it seems to be operating correctly while processing packets. The .spec files output for the test programs here are a bit tedious for me to check by hand. Have you or someone else checked the .spec output files by hand and they look correct to you? Or have you run the .spec and other p4c-dpdk output files on a P4-DPDK software switch and validated that it is behaving as the P4 source program says it should?

@jafingerhut, @fruffy, we have done the execution test using dpdk-pipeline and it works fine. We found a syntax issue in generated .spec file and that is also corrected and i pushed the changes now. You can review and approve it confidently.

We have the execution test artifacts. If that can be merged to DPDK test base, we can do that. Please advise or give us pointers to whom we need to contact for this. Thanks.

jafingerhut · 2024-12-17T14:57:36Z

@maheswari-s It looks like some expected output files for test programs need updating in order for the CI tests to pass.

jafingerhut · 2024-12-18T14:25:44Z

DCO check is failing, perhaps starting with most recent commit?

maheswari-s · 2024-12-18T15:06:59Z

@jafingerhut , I missed to add the signoff-by line so that the DCO check is failing. I just now added in the commit message and did a force push. But still it is failing. Any idea how to make this check to pass?

jafingerhut · 2024-12-18T15:33:23Z

@jafingerhut , I missed to add the signoff-by line so that the DCO check is failing. I just now added in the commit message and did a force push. But still it is failing. Any idea how to make this check to pass?

If you click on the "Details" link to the right of the failing DCO check, there are a list of step-by-step instructions, with specific git commands and their arguments, that you can try to use in updating this PR, such that it may pass the DCO check after you run those commands.

If those do not work, my best advice is to create a new PR, one where all commits are correctly signed, and delete this PR.

maheswari-s · 2024-12-18T17:23:01Z

@jafingerhut, I used the first method given in that link. Now it passed. Please check.

Signed-off-by : Sosutha Sethuramapandian <[email protected]> * Added support for 128 bit constants usage in dpdk backend * Updated testcases and outputs Signed-off-by: Maheswari Subramanian <[email protected]>

Signed-off-by: Maheswari Subramanian <[email protected]>

- The field name inside the compiler generated header is changed from 'tmp' to 'inter'. - The unnecessary extra string 'header' is removed from the variable instances.

Signed-off-by: Maheswari Subramanian <[email protected]>

…[email protected]> I, Maheswari Subramanian <[email protected]>, hereby add my Signed-off-by to this commit: 2d4d3db Signed-off-by: Maheswari Subramanian <[email protected]>

jafingerhut · 2024-12-23T17:12:04Z

Closing this PR, as #5074 is preferred over this one.

psivanup approved these changes Nov 26, 2024

View reviewed changes

fruffy added the dpdk Topics related to the DPDK back end label Nov 26, 2024

syogender approved these changes Dec 2, 2024

View reviewed changes

maheswari-s force-pushed the dpdk_128_bit branch from 28de38b to 3625864 Compare December 2, 2024 11:40

fruffy reviewed Dec 2, 2024

View reviewed changes

jafingerhut reviewed Dec 2, 2024

View reviewed changes

digesh099 reviewed Dec 6, 2024

View reviewed changes

maheswari-s force-pushed the dpdk_128_bit branch from bac7014 to dac8c5c Compare December 18, 2024 15:03

Sosutha and others added 10 commits December 19, 2024 10:24

Support for 128 bit constants in dpdk backend

4ea2d11

Signed-off-by : Sosutha Sethuramapandian <[email protected]> * Added support for 128 bit constants usage in dpdk backend * Updated testcases and outputs Signed-off-by: Maheswari Subramanian <[email protected]>

Issue in odd size variables is addressed.

801734e

Signed-off-by: Maheswari Subramanian <[email protected]>

Added test case for odd size in 128bit support.

f16af0a

Signed-off-by: Maheswari Subramanian <[email protected]>

Missing test result files are added

5dd040e

Signed-off-by: Maheswari Subramanian <[email protected]>

Issue in error file is corrected.

dd130c1

Signed-off-by: Maheswari Subramanian <[email protected]>

Error message for bigger constant ( > 64bits) is rephrased.

68bfe99

Signed-off-by: Maheswari Subramanian <[email protected]>

Lint reported coding issue is solved.

0bbeb88

Signed-off-by: Maheswari Subramanian <[email protected]>

Issue in generated spec file corrected.

3e3e874

- The field name inside the compiler generated header is changed from 'tmp' to 'inter'. - The unnecessary extra string 'header' is removed from the variable instances.

Issues in test case and its output files are corrected.

a21023d

Signed-off-by: Maheswari Subramanian <[email protected]>

DCO Remediation Commit for Maheswari Subramanian <maheswari.subramani…

31ecfb0

…[email protected]> I, Maheswari Subramanian <[email protected]>, hereby add my Signed-off-by to this commit: 2d4d3db Signed-off-by: Maheswari Subramanian <[email protected]>

maheswari-s force-pushed the dpdk_128_bit branch from 6a0a99b to 31ecfb0 Compare December 19, 2024 04:54

maheswari-s mentioned this pull request Dec 19, 2024

Added support for 128 bit constants usage in dpdk backend #5074

Merged

jafingerhut closed this Dec 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for 128 bit constants in dpdk backend #5034

Support for 128 bit constants in dpdk backend #5034

maheswari-s commented Nov 26, 2024

psivanup left a comment

maheswari-s commented Nov 26, 2024

jafingerhut commented Dec 1, 2024

syogender left a comment

maheswari-s commented Dec 2, 2024

fruffy Nov 26, 2024

fruffy Dec 2, 2024

maheswari-s Dec 3, 2024

fruffy Dec 3, 2024

jafingerhut Dec 2, 2024 •

edited

Loading

maheswari-s Dec 3, 2024

jafingerhut commented Dec 3, 2024

fruffy commented Dec 5, 2024

jafingerhut commented Dec 5, 2024

digesh099 Dec 6, 2024 •

edited

Loading

jafingerhut Dec 6, 2024

maheswari-s commented Dec 17, 2024

jafingerhut commented Dec 17, 2024

jafingerhut commented Dec 18, 2024

maheswari-s commented Dec 18, 2024

jafingerhut commented Dec 18, 2024

maheswari-s commented Dec 18, 2024

jafingerhut commented Dec 23, 2024

		@@ -37,6 +37,8 @@ limitations under the License.

		namespace P4::DPDK {

		static const int SupportedBitWidth = 128;

-static const int SupportedBitWidth = 128;
+namespace DpdkConstants {
+static constexpr int SupportedBitWidth = 128;
+}

		@@ -0,0 +1,3 @@
		psa-dpdk-128bitCast_error.p4(55): [--Werror=overlimit] error: DPDK target supports up-to 64-bit immediate values, 80w0xffffbbbb12345342aaaa exceeds the limit

Support for 128 bit constants in dpdk backend #5034

Support for 128 bit constants in dpdk backend #5034

Conversation

maheswari-s commented Nov 26, 2024

psivanup left a comment

Choose a reason for hiding this comment

maheswari-s commented Nov 26, 2024

jafingerhut commented Dec 1, 2024

syogender left a comment

Choose a reason for hiding this comment

maheswari-s commented Dec 2, 2024

fruffy Nov 26, 2024

Choose a reason for hiding this comment

fruffy Dec 2, 2024

Choose a reason for hiding this comment

maheswari-s Dec 3, 2024

Choose a reason for hiding this comment

fruffy Dec 3, 2024

Choose a reason for hiding this comment

jafingerhut Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

maheswari-s Dec 3, 2024

Choose a reason for hiding this comment

jafingerhut commented Dec 3, 2024

fruffy commented Dec 5, 2024

jafingerhut commented Dec 5, 2024

digesh099 Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

jafingerhut Dec 6, 2024

Choose a reason for hiding this comment

maheswari-s commented Dec 17, 2024

jafingerhut commented Dec 17, 2024

jafingerhut commented Dec 18, 2024

maheswari-s commented Dec 18, 2024

jafingerhut commented Dec 18, 2024

maheswari-s commented Dec 18, 2024

jafingerhut commented Dec 23, 2024

jafingerhut Dec 2, 2024 •

edited

Loading

digesh099 Dec 6, 2024 •

edited

Loading