Krnlmon changes to support linux_networking-v3 (Combined Recipe) #84

nupuruttarwar · 2024-02-21T00:14:43Z

No description provided.

nupuruttarwar · 2024-02-26T23:10:43Z

This is a draft PR and not ready for review. This will be ready for review one combined recipe P4 is tested on hardware

Signed-off-by: nupuruttarwar <[email protected]>

ffoulkes

Looks pretty good. I've noted some things I think should be addressed before this PR is merged.

switchapi/es2k/CMakeLists.txt

ffoulkes · 2024-04-01T21:39:15Z

switchapi/es2k/CMakeLists.txt

@@ -3,7 +3,9 @@
 # Copyright 2022-2023 Intel Corporation


Copyright date?

You modified this file, so the end date should be changed to 2024.

ffoulkes · 2024-04-01T21:41:02Z

switchapi/es2k/CMakeLists.txt

@@ -28,8 +26,10 @@ add_library(switchapi_target_o OBJECT
  switch_tunnel.c
  switch_vrf.c
  switchapi_utils.c
+  $<TARGET_OBJECTS:switchapi_lnwv3_target_o>


Delete this. You've specified it in the parent folder's cmake listfile.

ffoulkes · 2024-04-01T21:51:47Z

switchapi/es2k/lnw_v3/CMakeLists.txt

@@ -0,0 +1,26 @@
+# CMake build file for krnlmon/switchapi/es2k
+#
+# Copyright 2022-2023 Intel Corporation


Suggested change

# Copyright 2022-2023 Intel Corporation

# Copyright 2022-2024 Intel Corporation

You modified this file after copying it, so the end copyright date should be 2024.

CMakeLists.txt

ffoulkes · 2024-04-01T23:31:44Z

switchapi/es2k/lnw_v3/switch_pd_p4_name_routing.h

+#ifndef __SWITCH_PD_P4_NAME_ROUTING_MAPPING__
+#define __SWITCH_PD_P4_NAME_ROUTING_MAPPING__
+
+#ifdef __cplusplus


The extern "C" isn't needed. This file defines preprocessor symbols, which are are the same for both C and C++.

The end-of-line comment on the #endif is unnecessary. We're only conditionalizing one line; there's no confusion over which condition we're closing; and extra text adds visual noise and makes the line within the conditional harder to read.

I've been removing the comment from the #endifs (and deleting the occasional superfluous extern "C") as I work on files.

ffoulkes · 2024-04-01T23:33:48Z

switchapi/es2k/lnw_v3/switch_pd_p4_name_routing.h

+  "linux_networking_control.ecmp_v6_hash_action"
+
+#ifdef __cplusplus
+}


Should be removed, along with the superfluous extern "C".

I generally include an end-of-line comment on the closing brace (// extern "C"), to make it clear what kind of block we're closing. I do something similar for namespaces in C++.

This is another change I've been making as I edit files.

ffoulkes · 2024-04-01T23:34:48Z

switchapi/es2k/switch_nhop.c

@@ -18,7 +18,7 @@

 #include "switchapi/switch_nhop.h"

-#include "switch_pd_routing.h"
+#include "switchapi/es2k/lnw_v3/switch_pd_routing.h"


Suggested change

#include "switchapi/es2k/lnw_v3/switch_pd_routing.h"

#include "switch_pd_routing.h"

ffoulkes · 2024-04-01T23:36:56Z

switchapi/es2k/switch_lag.c

@@ -18,7 +18,7 @@

 #include "ipu_types/ipu_types.h"
 #include "switch_pd_utils.h"
-#include "switchapi/es2k/switch_pd_lag.h"
+#include "switchapi/es2k/lnw_v3/switch_pd_lag.h"


Suggested change

#include "switchapi/es2k/lnw_v3/switch_pd_lag.h"

#include "lnw_v3/switch_pd_lag.h"

If there is more than one occurrence, we should consider creating a wrapper file. Or, better still, define a common interface to the different implementations of switch_pd_lag.c.

ffoulkes · 2024-04-01T23:37:50Z

switchapi/es2k/switch_pd_p4_name_mapping.h

@@ -135,7 +135,7 @@
 #define LNW_L2_FWD_TX_TABLE_KEY_BRIDGE_ID "user_meta.pmeta.bridge_id"
 #define LNW_L2_FWD_TX_TABLE_KEY_DST_MAC "dst_mac"

-#define LNW_L2_FWD_TX_TABLE_ACTION_L2_FWD "linux_networking_control.l2_fwd"
+#define LNW_L2_FWD_TX_TABLE_ACTION_L2_FWD "linux_networking_control.l2_fwd_tx"


Danger, Will Robinson! Non-obvious breaking change between V2 and V3!

This was changed for debug purposes, will change it back to l2_fwd. Thanks for pointing it out

ffoulkes · 2024-04-02T02:43:17Z

@nupuruttarwar
I've implemented the LNW_VERSION cmake variable: PR #110.

If you bracket any V3-specific changes in common code with #ifdef LNW_V3, it will make it easier to find them later.
I've set the default to V3, so LNW_V3 will be defined (if --target=es2k) unless the user overrides it. (Assuming that the code in question incorporates the change, either directly or by rebasing your PR on main once it's merged.)

…in branch Signed-off-by: nupuruttarwar <[email protected]>

CMakeLists.txt

switchapi/es2k/CMakeLists.txt

switchapi/es2k/lnw_v2/switch_pd_p4_name_routing.h

switchapi/es2k/lnw_v3/switch_pd_p4_name_routing.h

switchapi/es2k/lnw_v3/switch_pd_lag.c

ffoulkes

LGTM

ffoulkes · 2024-04-02T18:55:16Z

switchapi/es2k/CMakeLists.txt

@@ -3,7 +3,9 @@
 # Copyright 2022-2023 Intel Corporation


You modified this file, so the end date should be changed to 2024.

ffoulkes · 2024-04-02T18:56:22Z

switchapi/es2k/lnw_v3/CMakeLists.txt

@@ -0,0 +1,26 @@
+# CMake build file for krnlmon/switchapi/es2k
+#
+# Copyright 2022-2023 Intel Corporation


You modified this file after copying it, so the end copyright date should be 2024.

switchapi/es2k/lnw_v3/switch_pd_lag.c

ffoulkes · 2024-04-02T18:59:38Z

switchapi/es2k/switch_pd_p4_name_mapping.h

@@ -135,7 +135,7 @@
 #define LNW_L2_FWD_TX_TABLE_KEY_BRIDGE_ID "user_meta.pmeta.bridge_id"
 #define LNW_L2_FWD_TX_TABLE_KEY_DST_MAC "dst_mac"

-#define LNW_L2_FWD_TX_TABLE_ACTION_L2_FWD "linux_networking_control.l2_fwd"
+#define LNW_L2_FWD_TX_TABLE_ACTION_L2_FWD "linux_networking_control.l2_fwd_tx"


ffoulkes · 2024-04-02T19:07:25Z

Something I failed to mention:

It would have been appropriate to provide a CL comment that provided more detail on the nature of the changes, and any limitations; primarily for the benefit of the reviewers. This would mention the intention to provide side-by-side support for V2 and V3 (in a subsequent update), and to update the Bazel build (also in a subsequent update). It's a way of capturing the information and sharing it with the rest of the team.

This PR inroduces support for linux networking version V3 which addresses scalability issues in V2 version. Mainly tunnel and and forwarding tables are moved to be exact match type to provide a scale of upto 1M entries. And lag and ecmp tables are moved to WCM since scale of 1K is enough for these cases. We have conditionalized V2 code under LNW_V2 and V3 code under LNW_V3. The intention is to provide side-by-side support for V2 and V3. Also, Bazel build will be updated for V3 codebase in a subsequent update. Once V3 version is stable, plan is to deprecate V2 version Signed-off-by: nupuruttarwar <[email protected]>

switchapi/es2k/switch_pd_utils.h

5abeel

LGTM

nupuruttarwar force-pushed the combined_rcp_kmon branch from a222c07 to 6c01b4c Compare March 13, 2024 17:27

Krnlmon changes to support linux_networking-v3 (Combined Recipe)

5d56747

Signed-off-by: nupuruttarwar <[email protected]>

nupuruttarwar force-pushed the combined_rcp_kmon branch from 4f3537c to 5d56747 Compare April 1, 2024 01:46

Merge branch 'main' into combined_rcp_kmon

bd00f9d

nupuruttarwar marked this pull request as ready for review April 1, 2024 03:42

nupuruttarwar requested review from ffoulkes, n-sandeep, vsureshkumarp and 5abeel as code owners April 1, 2024 03:42

ffoulkes reviewed Apr 1, 2024

View reviewed changes

nupuruttarwar force-pushed the combined_rcp_kmon branch from 80554be to 69241ad Compare April 2, 2024 06:29

Fix clang format issues, address comments and merge conflicts with ma…

60094cf

…in branch Signed-off-by: nupuruttarwar <[email protected]>

nupuruttarwar force-pushed the combined_rcp_kmon branch from 69241ad to 60094cf Compare April 2, 2024 07:02

Merge branch 'main' into combined_rcp_kmon

6949a65

nupuruttarwar force-pushed the combined_rcp_kmon branch from fa733a8 to ea56f08 Compare April 2, 2024 17:30

5abeel reviewed Apr 2, 2024

View reviewed changes

nupuruttarwar force-pushed the combined_rcp_kmon branch 2 times, most recently from e010344 to dded0ab Compare April 2, 2024 18:28

ffoulkes approved these changes Apr 2, 2024

View reviewed changes

nupuruttarwar force-pushed the combined_rcp_kmon branch from dded0ab to 861f456 Compare April 2, 2024 19:31

nupuruttarwar force-pushed the combined_rcp_kmon branch from 861f456 to 9641887 Compare April 2, 2024 19:32

5abeel reviewed Apr 2, 2024

View reviewed changes

switchapi/es2k/switch_pd_utils.h Show resolved Hide resolved

5abeel approved these changes Apr 2, 2024

View reviewed changes

nupuruttarwar merged commit 62112e1 into main Apr 2, 2024
4 checks passed

nupuruttarwar deleted the combined_rcp_kmon branch April 2, 2024 20:23

5abeel mentioned this pull request May 3, 2024

Revert L2 fwd table action name #129

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Krnlmon changes to support linux_networking-v3 (Combined Recipe) #84

Krnlmon changes to support linux_networking-v3 (Combined Recipe) #84

nupuruttarwar commented Feb 21, 2024

nupuruttarwar commented Feb 26, 2024

ffoulkes left a comment

ffoulkes Apr 1, 2024

ffoulkes Apr 2, 2024

ffoulkes Apr 1, 2024

ffoulkes Apr 1, 2024

ffoulkes Apr 2, 2024

ffoulkes Apr 1, 2024

ffoulkes Apr 1, 2024

ffoulkes Apr 1, 2024

ffoulkes Apr 1, 2024

ffoulkes Apr 1, 2024

nupuruttarwar Apr 2, 2024

ffoulkes Apr 2, 2024

ffoulkes commented Apr 2, 2024 •

edited

Loading

ffoulkes left a comment

ffoulkes Apr 2, 2024

ffoulkes Apr 2, 2024

ffoulkes Apr 2, 2024

ffoulkes commented Apr 2, 2024

5abeel left a comment

	# Copyright 2022-2023 Intel Corporation
	# Copyright 2022-2024 Intel Corporation

	#include "switchapi/es2k/lnw_v3/switch_pd_routing.h"
	#include "switch_pd_routing.h"

	#include "switchapi/es2k/lnw_v3/switch_pd_lag.h"
	#include "lnw_v3/switch_pd_lag.h"

Krnlmon changes to support linux_networking-v3 (Combined Recipe) #84

Krnlmon changes to support linux_networking-v3 (Combined Recipe) #84

Conversation

nupuruttarwar commented Feb 21, 2024

nupuruttarwar commented Feb 26, 2024

ffoulkes left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffoulkes commented Apr 2, 2024 • edited Loading

ffoulkes left a 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

ffoulkes commented Apr 2, 2024

5abeel left a comment

Choose a reason for hiding this comment

ffoulkes commented Apr 2, 2024 •

edited

Loading