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.

Sign up for GitHub

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

Jump to bottom

[dpapp] Initial dpapp implementation being a vpp plugin #609

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

[dpapp] Initial dpapp implementation being a vpp plugin #609

wants to merge 9 commits into from

Conversation

jimmyzhai
Copy link
Collaborator

@jimmyzhai jimmyzhai commented Jul 23, 2024 •
edited
Loading

Following dpapp HLD - #606, this is the 3th part implementation. It implements the basic logic of inline flow creation, deletion and ageout.

  • Build with command make dpapp
  • Run with command make run-dpapp

@KrisNey-MSFT
Copy link
Collaborator

hi @jimmyzhai - is this ready to merge? Thank you!

@KrisNey-MSFT
Copy link
Collaborator

hi @jimmyzhai it looks like we have conflicts that need to be resolved please and thank you.

@jimmyzhai jimmyzhai requested a review from r12f August 14, 2024 16:00
r12f
r12f reviewed Aug 22, 2024
View reviewed changes
dash-pipeline/vpp-plugin/vpp.sh Outdated
@@ -0,0 +1,17 @@
#!/bin/bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

also rename this file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@jimmyzhai jimmyzhai force-pushed the dp_app_add_vpp_plugin branch 3 times, most recently from e2c1a02 to 1ac27da Compare September 2, 2024 12:48
r12f
r12f reviewed Sep 2, 2024
View reviewed changes
dash-pipeline/dpapp/CMakeLists.txt
@@ -0,0 +1,11 @@
cmake_minimum_required(VERSION 3.5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you mind to add a README here to help people getting started on it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@KrisNey-MSFT
Copy link
Collaborator

checking in @jimmyzhai - is this a lower priority in general? TY!

@KrisNey-MSFT
Copy link
Collaborator

just checking in :)

@jimmyzhai
Copy link
Collaborator Author

just checking in :)

I'm actively updating this PR now after #608 is merged.

@KrisNey-MSFT
Copy link
Collaborator

KrisNey-MSFT commented Oct 30, 2024 via email

@jimmyzhai jimmyzhai force-pushed the dp_app_add_vpp_plugin branch 4 times, most recently from fb6bab2 to 765c873 Compare November 5, 2024 17:25
@KrisNey-MSFT
Copy link
Collaborator

hi @jimmyzhai - do you need help from @chrispsommers on this one?

@jimmyzhai jimmyzhai force-pushed the dp_app_add_vpp_plugin branch 9 times, most recently from 276617b to 0b51e53 Compare November 6, 2024 07:45
@jimmyzhai jimmyzhai force-pushed the dp_app_add_vpp_plugin branch 7 times, most recently from 8f735d4 to 8031721 Compare November 12, 2024 09:25
@jimmyzhai
Copy link
Collaborator Author

hi @jimmyzhai - do you need help from @chrispsommers on this one?

Hi @chrispsommers, I've always encountered workflow DASH-BMV2-CI failure with many tries. The error is:

**The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.

Run PTF Tests The operation was canceled.**

The error may happen sometimes before step "Run PTF Tests". The run-saithrift-ptftests succeeds in my local host. Looks the job is canceled by github action runner (due to running out of resources??). The runner host has 4 vCPUs, 16G memory (seen in step "Check dpapp status"). With adding dpapp, it takes more system overload and reaches limit? Any idea? Would you try to adjust github action runner host to more capacity machine (8 vCPUs) and re-run this workflow?

Found the cause. The runner is stopped due to sysctl vm.max_map_count = 1548, which limits max memory mapping number of one process.

@jimmyzhai jimmyzhai closed this Nov 12, 2024
@jimmyzhai jimmyzhai reopened this Nov 12, 2024
@jimmyzhai jimmyzhai force-pushed the dp_app_add_vpp_plugin branch 3 times, most recently from 21306ad to 27c84a6 Compare November 14, 2024 15:28
@jimmyzhai jimmyzhai requested a review from r12f November 14, 2024 15:57
r12f
r12f approved these changes Nov 21, 2024
View reviewed changes
dash-pipeline/dpapp/dash/dash.c Outdated
static uword
dash_timer_process (vlib_main_t * vm, vlib_node_runtime_t * rt, vlib_frame_t * f)
{

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: empty line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

dash-pipeline/dpapp/dash/dash.c Outdated
int i;
f64 sleep_duration = 1.0;

unix_sleep (5.0); /* FIXME: delay 5s */
Copy link
Collaborator

Choose a reason for hiding this comment

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

will be better to explain why this is a FIXME.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the unnecessary delay

dash-pipeline/dpapp/dash/dash.c
#define REPLY_MSG_ID_BASE sm->msg_id_base
#include <vlibapi/api_helper_macros.h>

/* *INDENT-OFF* */
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

vpp code style convention

dash-pipeline/dpapp/dash/node.c Outdated
vlib_get_next_frame (vm, node, next_index, to_next, n_left_to_next);

while (n_left_from >= 4 && n_left_to_next >= 2)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

will be better to get the indent fixed in this file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

dash-pipeline/dpapp/dash/node.c Outdated

typedef enum
{
DASH_NEXT_INTERFACE_OUTPUT,
Copy link
Collaborator

Choose a reason for hiding this comment

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

format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

dash-pipeline/dpapp/dash/node.c Outdated
en0 = (ethernet_header_t*)dh0 - 1;
#define _(a) tmp0[a] = en0->src_address[a];
foreach_mac_address_offset;
#undef _
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is.... really unintuitive... let's change this to a function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

dash-pipeline/dpapp/dash/node.c Outdated

while (n_left_from >= 4 && n_left_to_next >= 2)
{
u32 next0 = DASH_NEXT_INTERFACE_OUTPUT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

will be better to give them better names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just follow vpp naming pattern

dash-pipeline/dpapp/dash/node.c Outdated
foreach_mac_address_offset;
#undef _

/* Update dash header */
Copy link
Collaborator

Choose a reason for hiding this comment

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

will be better to move this into dedicated functions, same as build new packet, verify, build new packet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

dash-pipeline/dpapp/dash/node.c Outdated
u32 next0 = DASH_NEXT_INTERFACE_OUTPUT;
u32 next1 = DASH_NEXT_INTERFACE_OUTPUT;
u32 sw_if_index0, sw_if_index1;
u8 tmp0[6], tmp1[6];
Copy link
Collaborator

Choose a reason for hiding this comment

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

if multiple packets are processed in 1 iterator, at least we should create a struct context for each packet instead of doing *0,1. if we really like to do it in the vector way, we should also leverage array, so SIMD instructions can be generated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also do we really need to process multiple packets at once? although the framework allow us to do, but I don't think this performance will impact us that much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For simplicity, removed multiple packets processing in 1 iterator.

dash-pipeline/dpapp/dash/node.c Outdated

while (n_left_from > 0 && n_left_to_next > 0)
{
u32 bi0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably can get rid of it to make the code more clear and maintainable. this huge chunk of duplicated code is not really ideal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

r12f
r12f reviewed Nov 21, 2024
View reviewed changes
test/test-cases/functional/ptf/p4_dash_utils.py Outdated
sport = packet['UDP'].sport
dport = packet['UDP'].dport
else: # TODO: later for other ip proto
assert(False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

throw with message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

test/test-cases/functional/ptf/p4_dash_utils.py Outdated
dport,
packet['IP'].proto)
if existed:
assert(flow)
Copy link
Collaborator

Choose a reason for hiding this comment

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

assert/throw with message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

test/test-cases/functional/ptf/p4_dash_utils.py Outdated
if existed:
assert(flow)
else:
assert(not flow)
Copy link
Collaborator

Choose a reason for hiding this comment

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

assert/throw with message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

dash-pipeline/dpapp/dash/dash.c Outdated

while (1)
{
/* FIXME: Use time-wheel per-worker thread later */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment here to highlight the threading model, because dash_flow_table doesn't have a lock.

Copy link
Collaborator

Choose a reason for hiding this comment

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

remove the for loop and fix me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@r12f r12f self-requested a review November 21, 2024 04:35
@jimmyzhai
Fix per comments:
9fb1ce8 
1. not enable dpdk_plugin.so, then not need hugepage allocation
2. Update NODE_FN dash_node to be simple
@jimmyzhai
Raise assert error with message
542f59b
@jimmyzhai
Support command arguments in send_p2a_pkt.py
b52014d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrispsommers chrispsommers Awaiting requested review from chrispsommers

@r12f r12f Awaiting requested review from r12f

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jimmyzhai @KrisNey-MSFT @r12f