Bug fix: Flow Directory. (test_flow_dir) #248

bdevierno1 · 2020-07-14T09:27:05Z

Test flow directory NF bug fix.

Summary:

NF was crashing when attempting to add a new rule to the shared sdn_nf flow table.

Usage:
As described in readme.

This PR includes
Resolves issues
Breaking API changes
Internal API changes
Usability improvements
Bug fixes
New functionality
New NF/onvm_mgr args
Changes to starting NFs
Dependency updates
Web stats updates

Merging notes:

Dependencies: None

TODO before merging :

PR is ready for review

This resolves issue #247

Test Plan:

Run NF with Pktgen. Test that NF does not crash as before. As I made a change to the fill key functionality. All NFs that add/remove/lookup keys to a flow table need to be tested.

Review:

Anyone.

onvm · 2020-07-14T09:27:07Z

In response to PR creation

CI Message

Your results will arrive shortly

onvm · 2020-07-14T09:30:52Z

In response to PR creation

CI Message

Error: Failed to parse Pktgen stats

bdevierno1 · 2020-07-17T04:37:08Z

@onvm Hello HAL, do you read me?

onvm · 2020-07-17T04:37:09Z

@onvm Hello HAL, do you read me?

CI Message

Your results will arrive shortly

onvm · 2020-07-17T04:41:15Z

@onvm Hello HAL, do you read me?

CI Message

Error: Failed to fetch results from nimbnode23

examples/test_flow_dir/test_flow_dir.c

onvm/onvm_nflib/onvm_flow_table.h

bdevierno1 · 2020-07-20T09:18:04Z

In my last commit I have added a new feature/example populating service chains to the flow table #60 . You can then use this to test the onvm flow dir API by starting up up to four test_flow_dir NFs. Another way is to run this NF to populate the flow table then subsequently use the simple forward NF; but first modifying the packet handler to use the action next instead.

I have made further changes to the flow table api as it was printing the keys I had pre-defined backwards.

I have reverted the flow_entry variable to initialize to NULL. However, I had to allocate memory within the packet handler which will probably be slower. I will update this soon as there is probably a better way to do this.

examples/test_flow_dir/test_flow_dir.c

bdevierno1 · 2020-07-28T09:23:21Z

This is ready for review. If users set destination to 255 it will use the next action. Notable changes in last few commits involve using onvm's rss hash during table lookup.

bdevierno1 · 2020-07-28T15:45:57Z

@onvm Hello HAL, do you read me?

onvm · 2020-07-28T16:42:32Z

@onvm Hello HAL, do you read me

CI Message

Your results will arrive shortly

onvm · 2020-07-28T16:44:17Z

@onvm Hello HAL, do you read me

CI Message

Your results will arrive shortly

onvm

@onvm Hello HAL, do you read me

CI Message

Run successful see results:
Test took: 14 minutes, 59 seconds
✔️ Pktgen performance check passed
❌ PR drops Speed Test performance below minimum requirement

[Results from nimbnode23]

Median TX pps for Pktgen: 7735755
Performance rating - 100.46% (compared to 7700000 average)
Median TX pps for Speed Tester: 39947208
Performance rating - 95.11% (compared to 42000000 average)

kevindweb · 2020-07-28T20:41:18Z

@onvm is it really slow?

onvm · 2020-07-28T20:41:20Z

@onvm is it really slow?

CI Message

Your results will arrive shortly

onvm

@onvm is it really slow?

CI Message

Run successful see results:
Test took: 4 minutes, 1 second
✔️ Pktgen performance check passed
❌ PR drops Speed Test performance below minimum requirement

[Results from nimbnode23]

Median TX pps for Pktgen: 7733636
Performance rating - 100.44% (compared to 7700000 average)
Median TX pps for Speed Tester: 39891946
Performance rating - 94.98% (compared to 42000000 average)

koolzz

see comment

koolzz · 2020-07-28T20:53:38Z

onvm/onvm_nflib/onvm_pkt_common.c

@@ -102,7 +102,7 @@ onvm_pkt_process_tx_batch(struct queue_mgr *tx_mgr, struct rte_mbuf *pkts[], uin
                        // and !<return value> is 1.
                        nf->stats.act_drop++;
                        nf->stats.tx += !onvm_pkt_drop(pkts[i]);
-                } else if (meta->action == ONVM_NF_ACTION_NEXT) {
+                } else if (meta->action == ONVM_NF_ACTION_NEXT || meta->destination == 255) {


No magic numbers please, what is this 255? Why do we need it?
Can't the packet just set action ONVM_NF_ACTION_NEXT?

Having a special number is purposeful since we want to use this to indicate the next action even if the NF wasn't designed for that purpose (eg this lets us make the SimpleFwd NF use the "next action" behavior if you set 255 at the command line as the destination ID).

But you are right that it shouldn't be totally magic -- @bdevierno1 you should #define a constant somewhere for this instead of using 255 directly here.

Sure, feels like a lazy way to change the action(we could just bake in the same if meta->destination ==255 -> meta->action = ONVM_NF_ACTION_NEXT into the NF itself) but if people find it easier lets define a global const.

@bdevierno1 Also if we're doing this, after #defining the constant (probably in common.h) do a onvm mgr startup check to confirm that your const number is more than MAX_NFS. Otherwise if someone has 512 NFS configured they will be suprised that forwarding to 256 doesn't work

This line was resulting in the perf drop with speed tester. I couldn't find another way to optimize this besides moving the ONVM_NF_ACTION_TONF if statement above the one for the NEXT action on line 105. But I think this would feel like it would be an over-optimization for speed tester? I'll update to use the global const for now. I could also update all the sample NFs to use a flag instead to indicate to use the next action as well if we decide that's better.

bdevierno1 · 2020-07-29T03:37:07Z

@onvm Test please.

onvm · 2020-07-29T03:37:08Z

@onvm Test please.

CI Message

Your results will arrive shortly

onvm

@onvm Test please.

CI Message

Run successful see results:
Test took: 4 minutes, 3 seconds
✔️ Pktgen performance check passed
❌ PR drops Speed Test performance below minimum requirement

[Results from nimbnode23]

Median TX pps for Pktgen: 7732411
Performance rating - 100.42% (compared to 7700000 average)
Median TX pps for Speed Tester: 39828650
Performance rating - 94.83% (compared to 42000000 average)

bdevierno1 · 2020-08-03T08:21:12Z

Ran two tests using c220g1 nodes in Wisconsin site. Noticed about a 2% performance drop.

Without setting 255 as next action.

kevindweb · 2020-08-03T15:40:30Z

onvm/onvm_nflib/onvm_pkt_common.c

-                } else if (meta->action == ONVM_NF_ACTION_NEXT) {
-                        /* TODO: Here we drop the packet : there will be a flow table
-                        in the future to know what to do with the packet next */
+                } else if (meta->action == ONVM_NF_ACTION_NEXT || meta->destination == ACTION_NEXT_DEST_ID) {


@bdevierno1 setting 255 as the next action would definitely decrease performance, but I think that (from CI) we see that just having to process this or || statement in the condition slows down other NFs. Because we know that speed_tester would not have set the meta->destination to 255. I think that's the cause of the ~4% performance drop from CI

Yeah this is correct. Unfortunately, I tried different ways to optimize this but did not see any substantial improvements. By removing the OR I will still have to set another conditional which will have to be placed before this statement so as to maintain the same logic. Perhaps there is another way to do this?

examples/test_flow_dir/test_flow_dir.c

bvierno and others added 2 commits July 14, 2020 04:15

Bug fix: Not setting action after adding to table.

dfbc44f

Bug fix: Not setting action after adding to table.

66d1725

bdevierno1 closed this Jul 14, 2020

Removed memset when filling key in flow dir

f6d04a9

bdevierno1 reopened this Jul 17, 2020

Merge bug fix

6bf658d

bdevierno1 changed the title ~~Bug fix: Not setting action after adding to table.~~ Bug fix: Flow Directory. Jul 17, 2020

Benjamin De Vierno added 2 commits July 17, 2020 04:54

Styling

524db7f

Update onvm ipv4 tuple size.

289eaf3

bdevierno1 commented Jul 17, 2020

View reviewed changes

examples/test_flow_dir/test_flow_dir.c Outdated Show resolved Hide resolved

bdevierno1 commented Jul 17, 2020

View reviewed changes

examples/test_flow_dir/test_flow_dir.c Outdated Show resolved Hide resolved

bdevierno1 commented Jul 17, 2020

View reviewed changes

examples/test_flow_dir/test_flow_dir.c Show resolved Hide resolved

bdevierno1 commented Jul 17, 2020

View reviewed changes

onvm/onvm_nflib/onvm_flow_table.h Outdated Show resolved Hide resolved

bdevierno1 commented Jul 17, 2020

View reviewed changes

onvm/onvm_nflib/onvm_flow_table.h Show resolved Hide resolved

Convert port to big endian when filling key.

785a682

bdevierno1 commented Jul 19, 2020

View reviewed changes

onvm/onvm_nflib/onvm_flow_table.h Outdated Show resolved Hide resolved

Benjamin De Vierno added 2 commits July 20, 2020 03:50

Initialize flow entry to NULL & new feature

0888f1a

Conform to Linter

ac08651

Benjamin De Vierno added 2 commits July 20, 2020 05:15

Update print default service chain.

179afc0

Update flow entry variable to be global.

a312d3f

bdevierno1 mentioned this pull request Jul 20, 2020

NF flow_table runtime #242

Open

Benjamin De Vierno and others added 2 commits July 20, 2020 21:44

Update destination structs

d88f412

Update readme with new functionality.

1c20d9e

dennisafa reviewed Jul 22, 2020

View reviewed changes

examples/test_flow_dir/test_flow_dir.c Show resolved Hide resolved

Benjamin De Vierno added 2 commits July 28, 2020 01:49

Styling and undo changes to speed tester.

7c79874

Set Next action when destination is 255

90b77a1

Benjamin De Vierno and others added 2 commits July 28, 2020 10:03

Use pkt rss

98b4530

Documentation update. Service ID 255.

c92b77a

onvm suggested changes Jul 28, 2020

View reviewed changes

koolzz requested changes Jul 28, 2020

View reviewed changes

Benjamin De Vierno added 2 commits July 28, 2020 22:14

Define 255

d113696

Style

0ff3053

onvm suggested changes Jul 29, 2020

View reviewed changes

twood02 changed the title ~~Bug fix: Flow Directory.~~ Bug fix: Flow Directory. (test_flow_dir) Jul 29, 2020

Benjamin De Vierno added 2 commits July 30, 2020 03:57

Update to use latest pktgen

02cf1a4

Merge upstream/develop

faa11e6

kevindweb reviewed Aug 3, 2020

View reviewed changes

dennisafa previously approved these changes Dec 20, 2020

View reviewed changes

kevindweb reviewed Dec 20, 2020

View reviewed changes

examples/test_flow_dir/test_flow_dir.c Outdated Show resolved Hide resolved

kevindweb reviewed Dec 20, 2020

View reviewed changes

examples/test_flow_dir/test_flow_dir.c Outdated Show resolved Hide resolved

Benjamin De Vierno added 2 commits January 13, 2021 20:50

Merge remote-tracking branch 'upstream/develop' into test_flow_dir

61bff85

Remove multiple defn for var flow_entry

965329a

bdevierno1 dismissed dennisafa’s stale review via 965329a January 14, 2021 09:17

dennisafa added the bug fix 🐛 label Aug 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fix: Flow Directory. (test_flow_dir) #248

Bug fix: Flow Directory. (test_flow_dir) #248

bdevierno1 commented Jul 14, 2020 •

edited

Loading

onvm commented Jul 14, 2020

onvm commented Jul 14, 2020

bdevierno1 commented Jul 17, 2020

onvm commented Jul 17, 2020

onvm commented Jul 17, 2020

bdevierno1 commented Jul 20, 2020

bdevierno1 commented Jul 28, 2020 •

edited

Loading

bdevierno1 commented Jul 28, 2020

onvm commented Jul 28, 2020

onvm commented Jul 28, 2020

onvm left a comment

kevindweb commented Jul 28, 2020

onvm commented Jul 28, 2020

onvm left a comment

koolzz left a comment

koolzz Jul 28, 2020

twood02 Jul 28, 2020

koolzz Jul 28, 2020 •

edited

Loading

koolzz Jul 28, 2020

bdevierno1 Jul 29, 2020

bdevierno1 commented Jul 29, 2020

onvm commented Jul 29, 2020

onvm left a comment

bdevierno1 commented Aug 3, 2020

kevindweb Aug 3, 2020

bdevierno1 Aug 3, 2020

Bug fix: Flow Directory. (test_flow_dir) #248

Are you sure you want to change the base?

Bug fix: Flow Directory. (test_flow_dir) #248

Conversation

bdevierno1 commented Jul 14, 2020 • edited Loading

Summary:

Merging notes:

Test Plan:

Review:

onvm commented Jul 14, 2020

CI Message

onvm commented Jul 14, 2020

CI Message

bdevierno1 commented Jul 17, 2020

onvm commented Jul 17, 2020

CI Message

onvm commented Jul 17, 2020

CI Message

bdevierno1 commented Jul 20, 2020

bdevierno1 commented Jul 28, 2020 • edited Loading

bdevierno1 commented Jul 28, 2020

onvm commented Jul 28, 2020

CI Message

onvm commented Jul 28, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

kevindweb commented Jul 28, 2020

onvm commented Jul 28, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

koolzz left a comment

Choose a reason for hiding this comment

koolzz Jul 28, 2020

Choose a reason for hiding this comment

twood02 Jul 28, 2020

Choose a reason for hiding this comment

koolzz Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

koolzz Jul 28, 2020

Choose a reason for hiding this comment

bdevierno1 Jul 29, 2020

Choose a reason for hiding this comment

bdevierno1 commented Jul 29, 2020

onvm commented Jul 29, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

bdevierno1 commented Aug 3, 2020

kevindweb Aug 3, 2020

Choose a reason for hiding this comment

bdevierno1 Aug 3, 2020

Choose a reason for hiding this comment

bdevierno1 commented Jul 14, 2020 •

edited

Loading

bdevierno1 commented Jul 28, 2020 •

edited

Loading

koolzz Jul 28, 2020 •

edited

Loading