Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Bug fix: Flow Directory. (test_flow_dir) #248
base: develop
Are you sure you want to change the base?
Bug fix: Flow Directory. (test_flow_dir) #248
Changes from 27 commits
dfbc44f
66d1725
f6d04a9
6bf658d
524db7f
289eaf3
785a682
0888f1a
ac08651
179afc0
a312d3f
d88f412
1c20d9e
6e419cb
7cc7a61
94446c6
31447bd
ed8681c
887a277
7c7ae8f
05dff7b
785ee44
92092ee
7c79874
90b77a1
98b4530
c92b77a
d113696
0ff3053
02cf1a4
faa11e6
61bff85
965329a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No magic numbers please, what is this 255? Why do we need it?
Can't the packet just set action ONVM_NF_ACTION_NEXT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.