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.

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

Sampling #307

Merged
merged 6 commits into from
May 8, 2023
Merged

Sampling #307

merged 6 commits into from
May 8, 2023

Conversation

vikasJain85
Copy link
Contributor

@vikasJain85 vikasJain85 commented Apr 29, 2023

What type of PR is this?
/kind feature

What this PR does / why we need it:
Sampling Application uses 3-tuple to maintain context information can be a useful tool for managing network traffic and ensuring that packets are delivered to their intended destinations.

Which issue(s) this PR fixes:

Issue: #223
Signed-off-by: Vikas Jain vikas.jain@intel.com
Signed-off-by: Vijaya Tiruveedula vijaya.tiruveedula@intel.com

Vijaya Rani and others added 3 commits April 29, 2023 08:04
…src ip, dst ip, protocol) of a receiving packet

2. if a port receives multiple packets with the same 3-tuple assume they are
part of same context and increment count.
3. if the packet count of same context greater than 15, drop the packer
otherwise forward the packet
4. Overall, using a 3-tuple to maintain context information can be a useful tool
for managing network traffic and ensuring that packets are delivered to their
intended destinations.
Copy link
Contributor

Choose a reason for hiding this comment

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

The binary sampling file should not be included in the commit.

Need to add sampling/sampling binary file to the cndp/lang/go/.gitignore file and remove the binary from the commit.

@@ -227,8 +229,10 @@ func GetIPv4(pkt *Packet) *IPv4Hdr {

if pkt != nil {
ether := GetEtherHdr(pkt)

//fmt.Println("ethernet Header", ether)
Copy link
Contributor

@KeithWiles KeithWiles Apr 29, 2023

Choose a reason for hiding this comment

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

Need to remove the commented out code debug code from the commit.

@@ -0,0 +1,17 @@
Brief Description about Sampling Go Apllication
Copy link
Contributor

Choose a reason for hiding this comment

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

Convert this file from a README to a doc.go file. This allows us to use the document tools to display this type of information, which is something needed for other Go code and will create an issue. Make sure the copyright notice is correct with the current year only.

},
"fwd:1": {
"group": "group1",
"lports": ["eno12399:0"],
Copy link
Contributor

@KeithWiles KeithWiles Apr 29, 2023

Choose a reason for hiding this comment

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

Having two threads manage the same port should not be done. The transmit side of CNDP has protection from two or more threads accessing the same netdev/qid, but the receive side does not have that protection. Normally we have only one thread doing receives on a given netdev/qid, but that thread can send to any other netdev/qid port to allow forwarding of packets.

Unless the example requires two I would remove the second thread for now, but it really needs to be able to handle more than one netdev/qid devices.

"bufcnt":64,
"bufsz": 2,
"mtype": "2MB",
"regions": [
Copy link
Contributor

Choose a reason for hiding this comment

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

If you remove the second thread and because only one netdev/qid was defined in the lports section, please also change the regions from "regions": [32, 32], to "regions":[64],

@@ -0,0 +1,159 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be named sampling.jsonc not sampling.jsnoc

Copy link
Contributor

@KeithWiles KeithWiles left a comment

Choose a reason for hiding this comment

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

I am still reviewing this PR, but I had a few quick comments.

I noticed the DCO or Signed-off-by lines are missing in one of more commits in this PR. We require all commits to contain this line in the commit message. If you click on the Details link in the Conversation tab where the tests are run it will explain how to correct this problem.

Fix up the json file to remove second thread and umem region.

Signed-off-by: Keith Wiles <keith.wiles@intel.com>
@KeithWiles KeithWiles merged commit e96f91f into CloudNativeDataPlane:main May 8, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants