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

Fair Queue NF #254

Merged
merged 5 commits into from
Oct 25, 2021
Merged

Fair Queue NF #254

merged 5 commits into from
Oct 25, 2021

Conversation

rohit-mp
Copy link
Contributor

This PR adds a new example - Fair Queue with Round Robin Scheduler

The NF uses the advanced rings mode, classifies IPv4 packets based on the header information, and simulates a round-robin dequeue.

The CRC32 hash from the DPDK library has been used to classify the packets.

The NF uses a custom implementation of the standard FIFO queuing system. An implementation using the rte_ring structure from the DPDK library failed to scale for large number of queues.

Summary:

Usage:

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

Merging notes:

  • Dependencies: None

TODO before merging :

  • PR is ready for review

Test Plan:

An equal number of flows and queues are created to test the NF. The packets are generated by Pktgen and have same headers values except destination port with a 1000B packet size. An example:

A 1G NIC was used to send the packets to the NF. As can be seen from the image, each queue gets approximately 15MBps throughput which is 1/8th of the total link speed. A maximum of 35% hash collision probability is observed when the NF is tested with a large number of flows and queues (up to 1024).

NF Checklist:

  • Documentation describes purpose of NF, all arguments, and basic usage
  • NF stores all non-global state in a struct referenced by nf->data
  • NF performs initialization in a function_table->setup function and frees dynamic memory before exiting (NA)

Review:

(optional) << @-mention people who should review these changes >>

Subscribers: @mohittahiliani

@onvm
Copy link

onvm commented Aug 24, 2020

In response to PR creation

CI Message

Aborting, need an authorized user to run CI

@rohit-mp
Copy link
Contributor Author

The code in the branch works fine as it is now as seen below
Screenshot from 2020-08-24 22-32-28

But upon interchanging the thread functionalities (i.e., applying this patch, the code seems to break as seen below
Screenshot from 2020-08-24 22-33-36

I was not able to figure out why this happens, and it would be great if I could get some help.

@rohit-mp
Copy link
Contributor Author

Hi team, is there any update on this?

@bdevierno1
Copy link
Contributor

Hi team, is there any update on this?

Hi @rohit-mp sorry for the delayed response. The team is looking at it now.

@twood02 twood02 requested a review from NoahChinitz June 17, 2021 13:53
@catherinemeadows
Copy link
Contributor

catherinemeadows commented Jun 29, 2021

Hi @rohit-mp sorry for the delay and thanks so much for making this PR. When running the fair queue NF, I noticed two bugs:

  1. When invalid arguments are used, the NF shuts down with a segmentation fault. To replicate this, try to run the fair queue NF as such: ./go.sh 1 -d. To fix this, in your destroy_fairqueue() function, you should check if (fairqueue == NULL) to handle the case that setup_fairqueue was never called in parse_app_args because -1 was returned in the case for no -d flag set
  2. Regardless of the number of queues I specify, the manager always registers that I've started up two fair queue NFs with the same service ID but different instance IDs. Is there intention behind this and if so, what is the reasoning?

The below screenshot shows both happening.
Screen Shot 2021-06-29 at 10 31 26 AM

@twood02
Copy link
Member

twood02 commented Aug 23, 2021

@catherinemeadows - I think from our discussion earlier we decided that point 1 in your comment above is something we can easily fix ourselves and point 2 is expected behavior because this is based on the scaling API.

Other than that, was this ready to go?

prevent failure in destructor

Co-authored-by: Catherine Meadows <meadowsc@gwu.edu>
@twood02 twood02 added the ready-for-gatekeeper 🚪 PRs that have been approved and are ready for a final review/merge label Oct 25, 2021
@dennisafa dennisafa merged commit 3149348 into sdnfv:develop Oct 25, 2021
@rohit-mp
Copy link
Contributor Author

Thanks for the reviews and the fixes!
Sorry I was preoccupied and wasn't able to allocate time to this.

@twood02
Copy link
Member

twood02 commented Jan 23, 2022

Thanks for the contribution @rohit-mp !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new NF 🛰️ ready-for-gatekeeper 🚪 PRs that have been approved and are ready for a final review/merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants