Add skeleton implementation for BMv2 PNA Backend #4729

rupesh-chiluka-marvell · 2024-06-17T17:05:45Z

Hello everyone,
As mentioned in the issue: #4717, I created an initial skeleton code for the BMv2 PNA Backend.
It's in the early stages. I know it needs a lot of improvement.

Your feedback is much appreciated.

qobilidop · 2024-06-18T01:22:02Z

p4include/bmv2/pna.p4

Are we deviating from p4include/pna.p4 already? If not, I'd suggest starting with reusing p4include/pna.p4.

I also have some questions on our high level guidance on forking pna.p4:

I know we already have p4include/dpdk/pna.p4. But do we expect all PNA-supporting backends to make their own forks?

When forking is unavoidable, could we at least have more reusable code instead of copying the same file multiple times? For example, for the existing DPDK backend case, could we have the same standard pna.p4 backend end specific additions like pna_dpdk.p4 or pna_bmv2.p4?

These questions are not specific to this PR. So if preferred, we can probably have further discussions in a dedicated issue, unless there is a short answer already.

I have taken this pna.p4 file from https://github.com/p4lang/pna/blob/main/pna.p4.

p4include/dpdk/pna.p4 was last updated a year ago, and p4include/pna.p4 was last updated 2 years ago. After that, many changes are made to the PNA spec, like removing the pre_control block. I am using the latest 'pna.p4' file. p4include/tc/pna.p4 has the latest PNA spec with some extra code. The dpdk code will give errors with the latest PNA spec.

I think as long as everyone uses the latest PNA spec, we can have only one PNA spec file for every backend.
I am not sure why we have 3 PNA spec files: p4include/dpdk/pna.p4, p4include/tc/pna.p4 and p4include/pna.p4.

I think option 2:

For example, for the existing DPDK backend case, could we have the same standard pna.p4 backend end specific additions like pna_dpdk.p4 or pna_bmv2.p4?

will be good.

We can update p4include/pna.p4 if needed. @jafingerhut

@rupesh-chiluka-marvell Thanks for the pointer! I didn't know this before.

I know that dpdk/pna.p4 has their additions on top of the standard pna.p4, ipsec_accelerator for example:

p4c/p4include/dpdk/pna.p4

Line 516 in 7f98dad

extern ipsec_accelerator {

We can update p4include/pna.p4 if needed. @jafingerhut

That would be great. If everyone agrees, I can update it and use it.

Whether it is important or useful to have a p4include/pna.p4 include file separate from any particular target, is not clear to me. I suspect that this file might currently be used by many test programs?

Three options:

We could get rid of it to avoid further confusion.

We keep it in sync with https://github.com/p4lang/pna/blob/main/pna.p4 where I assume it is coming from.

We keep it in sync but also add some form of pragma annotation showing that this file is not mean to be used standalone. Not sure whether this is feasible. I vaguely remember some long discussion around the extensibility of the file.

Thanks @jafingerhut! Now I understand the reason for pna.p4 to be target-specific. I still feel there is room to improve how we organize these files. Simply maintaining a separate pna.p4 file for each backend target might lead to out-of-sync issues unless extra care is taken.

I have some thoughts on how to improve this in the short term:

The main idea is to generate the target-specific pna.p4 file instead of maintaining it by hand.

We can maintain a single pna.p4 template file, maybe using Jinja. The customizable parts then become parameters that could be provided by a separate file.

For further additions, like ipsec_accelerator in DPDK backend, we can also use Jinja to include a separate ipsec_accelerator.p4 into the main pna.p4 template.

Eventually, using the DPDK backend as an example, we'll have the following files:

p4include/

pna.p4: The Jinja template

dpdk/

pna_params.json: Target-specific params for the template

pna_experimental.p4: Target-specific experimental features to be included into the main pna.p4

pna.p4: Generated from the other files

In the longer term, I feel making "experimental" features like ipsec_accelerator in a separate file might preferred. Otherwise users might assume these are already standard PNA features. But having both pna.p4 and pna_experimental.p4 as includes would require larger-scale refactoring. So I feel it's better to keep using a single pna.p4 in the short term.

I can volunteer to do the work if there are no major concerns. And if needed, I can open a separate issue to further discuss this.

Thanks for volunteering. I will warn you that some of the changes made in dpdk/pna.p4 include adding some parameters to some extern methods in an extern object that exists in the original pna.p4 file that they started from at the time.

Also realize that different targets may easily advance at different rates in the future, not all of them being on the most recent version of the "source" pna.p4 file, and it seems to me like the only way to force them to be the same would be to get all developers of all targets to move in lock step with each other, which might not be feasible.

Ah, that indeed is a complicated situation. I see there is also tc/pna.p4. I'll have a try to see if any meaningful improvements can be made.

If #4752 gets approved and submitted, p4include/bmv2/pna.p4's contents could simply be

#include <_internal/pna/v0_7/all.p4>

Or the dev version if that's wanted.

qobilidop · 2024-06-19T06:54:51Z

@rupesh-chiluka-marvell Can you provide an example P4 program, and demonstrate that this backend is working? If setting up automated testing is too much for this PR, I think describing in the comments the commands to run might be good enough.

rupesh-chiluka-marvell · 2024-06-19T07:32:32Z

@rupesh-chiluka-marvell Can you provide an example P4 program, and demonstrate that this backend is working? If setting up automated testing is too much for this PR, I think describing in the comments the commands to run might be good enough.

All the changes to install the BMv2 PNA backend have been made in Makefiles.
We can follow the source installation steps mentioned in the P4C repository:

mkdir build
cd build
cmake ..
make -j4
sudo make install
sudo ldconfig

A new binary, p4c-bm2-pna, will be created.

We can compile P4 PNA programs using the p4c-bm2-pna binary:

p4c-bm2-pna <p4-pna-prog>.p4 -o <p4-pna-output>.json

I have taken the pna-demo-L2-one-table.p4 program from https://github.com/p4lang/pna/blob/main/examples/pna-demo-L2-one-table.p4 and commented out components in table.

I am attaching the output .json file: pna-demo-L2-one-table.json
I am not able to upload the .p4 file. So, I am attaching the input .p4 file as a .txt file: pna-demo-L2-one-table.txt

fruffy · 2024-06-19T13:31:23Z

backends/bmv2/pna_nic/pnaNic.cpp

+}
+
+void PnaCodeGenerator::createGlobals() {
+    /* TODO */


I recommend explicit failures. E.g., when there is a construct that is not implemented you should throw an error or P4C_UNIMPLEMENTED. This way you do not accidentally compile something that does not work. I recommend that for all the TODOs here.

fruffy · 2024-06-19T13:34:01Z

Yeah we should add some compile-only tests. Please take a look at the BMv2 PSA implementation and the way it sets up compilation.

https://github.com/p4lang/p4c/blob/main/backends/bmv2/CMakeLists.txt#L222

We do not need the check for a PNA_NIC just yet. I recommend just compiling the code for now and generating a reference file.

rupesh-chiluka-marvell · 2024-06-28T06:34:23Z

With the recent commit, I have made a few changes in the code hierarchy. Previously, the code hierarchy looked like this:

The psa_switch and pna_nic backends share a lot of code. To reduce the duplicate code, I created a few common classes that start with Portable... in the backends/bmv2/portable_common directory. After introducing the Portable classes, the code hierarchy looks like:

rupesh-chiluka-marvell · 2024-06-28T07:06:27Z

@qobilidop and I want to suggest slight code changes. We want to ask the community for its feedback. Currently, we have two classes, PsaProgramStructure and PsaCodeGenerator. PsaCodeGenerator doesn't have any new member variables (It uses only PsaProgramStructure member variables) and contains functions like:

void PsaCodeGenerator::create(ConversionContext *ctxt) {
    createTypes(ctxt);
    createHeaders(ctxt);
    createScalars(ctxt);
    createExterns();
    createParsers(ctxt);
    createActions(ctxt);
    createControls(ctxt);
    createDeparsers(ctxt);
    createGlobals();
}

Can we combine these two classes into one class, like PsaProgramStructureAndCodeGenerator? It helps simplify the development of the pna_nic backend after introducing Portable classes.

fruffy · 2024-06-28T13:26:44Z

What is the purpose of the program structure classes if the generators are derived from them? Is it just separation of concerns. For moving things forward, it seems reasonable to merge these classes.

rupesh-chiluka-marvell · 2024-06-30T14:35:16Z

backends/ebpf/CMakeLists.txt

@@ -61,7  61,20 @@ set (P4C_EBPF_SRCS
  ebpfModel.cpp
  midend.cpp
  lower.cpp
+  ../bmv2/common/JsonObjects.cpp


After transferring the CodeGenerator code into the ProgramStructure class, some include statements are shifted to portableProgramStructure.h file. I have added corresponding sources here. It is the exact reason for the changes in the backends/tc/CMakeLists.txt file.

After transferring the CodeGenerator code into the ProgramStructure class, some include statements are shifted to portableProgramStructure.h file.

I don't see any code changes under the ebpf dir. Could you link to an example of relevant include statements?

I didn't make any code changes under the ebpf directory. ebpf includes the psa_switch code.

p4c/backends/ebpf/ebpfBackend.cpp

Line 19 in b27702f

#include "backends/bmv2/psa_switch/psaSwitch.h"

p4c/backends/ebpf/psa/backend.cpp

Line 19 in b27702f

#include "backends/bmv2/psa_switch/psaSwitch.h"

Now, Some of the psa_switch code depends on portable classes. For linking purposes while building the ebpf target, I have added portable sources in the ebpf/CMakeLists.txt file.

Previously, the PsaCodeGenerator class was in a separate file, and the above header files are needed for the PsaCodeGenerator class.
Ebpf didn't use codeGenerator classes. So, these header files are not in the ebpf/CMakeLists.txt file.

I moved the CodeGenerator code into the ProgramStructure class and added these header sources to the epbf/CMakeLists.txt file.

I'm not that familiar with CMake but I wonder if there is a better way to handle this kind of transitive dependencies in CMake?

What I see is that ebpf code is only directly dependent on psaSwitch. What has changed are the dependencies of psaSwitch. I imagine there could be a way to only update psaSwitch dependencies, and ebpf could know these transitive dependencies automatically without having to list them here explicitly.

I know how to do this in Bazel but not in CMake. Maybe @fruffy has some ideas?

I did some searches on "CMake transitive dependency" and there does seem to be some solution. @rupesh-chiluka-marvell could you take a further look at this?

I'm not that familiar with CMake but I wonder if there is a better way to handle this kind of transitive dependencies in CMake?

They are usually handled by making a library or include PUBLIC.

This seems like a problem that should have been fixed earlier. eBPF should not include a BMv2 folder class. Let me take a look.

The dependencies on BMv2 are completely messed up here. I will create a PR to clean this up before it gets even worse.

#4770 The PR is now merged, please rebase your changes on top of it.

fruffy · 2024-07-28T15:37:26Z

cmake/FindBMV2.cmake

@@ -58,3  58,25 @@ mark_as_advanced(PSA_SWITCH PSA_SWITCH_CLI)
 find_package_handle_standard_args ("BMV2"
  "Program 'psa_switch_CLI' (https://github.com/p4lang/behavioral-model.git) not found;\nSearched ${BMV2_PSA_SWITCH_SEARCH_PATHS}.\nWill not run PSA BMv2 tests."
  PSA_SWITCH PSA_SWITCH_CLI)
+
+  set(BMV2_PNA_NIC_SEARCH_PATHS


Indentation is off here.

fruffy

@jafingerhut @qobilidop any further comments?

fruffy · 2024-07-29T15:57:24Z

backends/bmv2/pna_nic/README.md

@@ -0,0  1,29 @@
+# BMv2 `pna_nic` Backend


@AdarshRawat1 Does this documentation work with doxygen?

No, This Markdown heading format is not yet officially released, it's a work in progress. . As of today, this produces the output:

Alternatives

Code GitHub preview Doxygen Output

# BMv2 <code>pna_nic</code> Backend

"pna_nic"

'pna_nic'

@rupesh-chiluka-marvell Could you adjust?

@AdarshRawat1 Otherwise, you do not see any other issues, right?

I changed the header. Is it ok now?

Yes, we can fix any other upcoming issues later.

qobilidop

LGTM

Signed-off-by: Rupesh Chiluka <[email protected]>

Common code for both PSA and PNA resides in it. Added common classes: PortableProgramStructure, ParsePortableArchitecture, InspectPortableProgram, PortableCodeGenerator, PortableOptions. Signed-off-by: Rupesh Chiluka <[email protected]>

- Pass ProgramStructure as argument in CodeGenerator methods. Signed-off-by: Rupesh Chiluka <[email protected]>

Signed-off-by: Rupesh Chiluka <[email protected]>

fruffy added the pna-bmv2 Topics related to the BMv2 PNA back end. label Jun 17, 2024

qobilidop reviewed Jun 18, 2024

View reviewed changes

fruffy mentioned this pull request Jun 19, 2024

Make sure compiler_meta__->drop starts out false #4722

Merged

fruffy reviewed Jun 19, 2024

View reviewed changes

rupesh-chiluka-marvell force-pushed the p4c_pna_dev_v1 branch from bd98f40 to 2f42893 Compare June 27, 2024 08:54

rupesh-chiluka-marvell force-pushed the p4c_pna_dev_v1 branch from 2f42893 to 7423427 Compare June 30, 2024 13:43

rupesh-chiluka-marvell commented Jun 30, 2024

View reviewed changes

rupesh-chiluka-marvell force-pushed the p4c_pna_dev_v1 branch 2 times, most recently from 86cde01 to c58d7f1 Compare July 28, 2024 11:01

fruffy reviewed Jul 28, 2024

View reviewed changes

rupesh-chiluka-marvell force-pushed the p4c_pna_dev_v1 branch from 9c5d53a to c4e5877 Compare July 29, 2024 07:38

rupesh-chiluka-marvell marked this pull request as ready for review July 29, 2024 07:51

fruffy approved these changes Jul 29, 2024

View reviewed changes

fruffy reviewed Jul 29, 2024

View reviewed changes

qobilidop approved these changes Jul 30, 2024

View reviewed changes

rupesh-chiluka-marvell force-pushed the p4c_pna_dev_v1 branch 2 times, most recently from fb4497d to 1b7ab46 Compare July 31, 2024 07:17

rupesh-chiluka-marvell added 5 commits July 31, 2024 20:24

Add skeleton implementation for BMv2 PNA Backend

7fefeac

Signed-off-by: Rupesh Chiluka <[email protected]>

Added portable_common directory.

f2864eb

Common code for both PSA and PNA resides in it. Added common classes: PortableProgramStructure, ParsePortableArchitecture, InspectPortableProgram, PortableCodeGenerator, PortableOptions. Signed-off-by: Rupesh Chiluka <[email protected]>

Removed inheritance from ProgramStructure in CodeGenerator Classes.

77b8331

- Pass ProgramStructure as argument in CodeGenerator methods. Signed-off-by: Rupesh Chiluka <[email protected]>

Added README.md in 'pna_nic' and 'portable_common' directories

d7b42ff

Signed-off-by: Rupesh Chiluka <[email protected]>

Changed README header

771aec4

Signed-off-by: Rupesh Chiluka <[email protected]>

rupesh-chiluka-marvell force-pushed the p4c_pna_dev_v1 branch from 1b7ab46 to 771aec4 Compare July 31, 2024 14:55

fruffy approved these changes Jul 31, 2024

View reviewed changes

fruffy added the bmv2 Topics related to BMv2 or v1model label Jul 31, 2024

fruffy enabled auto-merge July 31, 2024 14:57

fruffy added this pull request to the merge queue Jul 31, 2024

Merged via the queue into p4lang:main with commit d2c1827 Jul 31, 2024
17 checks passed

rupesh-chiluka-marvell deleted the p4c_pna_dev_v1 branch August 3, 2024 05:25

Add skeleton implementation for BMv2 PNA Backend #4729

Add skeleton implementation for BMv2 PNA Backend #4729

Conversation

rupesh-chiluka-marvell commented Jun 17, 2024

Choose a reason for hiding this comment

rupesh-chiluka-marvell Jun 18, 2024 • edited Loading

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

qobilidop commented Jun 19, 2024

rupesh-chiluka-marvell commented Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruffy commented Jun 19, 2024 • edited Loading

rupesh-chiluka-marvell commented Jun 28, 2024 • edited Loading

rupesh-chiluka-marvell commented Jun 28, 2024

fruffy commented Jun 28, 2024

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

Choose a reason for hiding this comment

fruffy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alternatives

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

qobilidop left a comment

Choose a reason for hiding this comment

rupesh-chiluka-marvell Jun 18, 2024 •

edited

Loading

rupesh-chiluka-marvell commented Jun 19, 2024 •

edited

Loading

fruffy commented Jun 19, 2024 •

edited

Loading

rupesh-chiluka-marvell commented Jun 28, 2024 •

edited

Loading