Manager crash when no ports initialized

[dennisafa]

Manager crashes if we try to send packets out on an uninitialized port.

Summary:

The function onvm_pkt_enqueue_port() will call onvm_pkt_flush_port_queue() which TX bursts packets out of the port with rte_eth_tx_burst(). If you run the manager with no ports and try to do this, it'll segfault on rte_eth_tx_burst(). This fix checks if the port is initialized before sending out packets by adding an array that tracks which ports are initialized.

Usage: Run manager with ./go.sh 0,1,2,3 0 0xf0 -s stdout -a 0x7f000000000 and basic_monitor with ./go.sh 1 -p 1 then run speed_tester with ./go.sh 2 -d 1 It should not crash anymore.

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

Merging notes:

• Dependencies: None

TODO before merging :

• [x] PR is ready for review

Test Plan:

Follow the usage guide, verify that manager does not crash anymore.

Review:

@koolzz @kevindweb

[onvm]

In response to PR creation

CI Message

Your results will arrive shortly

[koolzz]

What If you have 1 port enabled and you try to send out of 2? Looks like this doesn't fix that.

[koolzz]

Also slightly concerned about adding a check to the critical path, as this would occur every pkt send (out of the NIC). What does the pktgen perf difference look like on cloudlab nodes?

[dennisafa]

What If you have 1 port enabled and you try to send out of 2? Looks like this doesn't fix that.

hmm, yea good point. what if we initialize all port ID's to NULL (or -1) before we assign them for a quick check? will check out cloudlab perf. edit: might be a bit more complicated for ID's, ill think of a fix.

[dennisafa]

@onvm give me a run

pktgen is acting up for whatever reason, ill get the # in a bit

[onvm]

@onvm give me a run

pktgen is acting up for whatever reason, ill get the # in a bit

CI Message

Your results will arrive shortly

[dennisafa]

some pktgen tests from cloudlab without the extra check:

with the check:

no notable performance regressions.

[kevindweb]

@dennisafa this works, tested with your solution, and saw that develop does fail with this edge case. Pktgen is fine. My only thought is that it's pretty redundant to keep checking for this port. If an NF has been initialized, the port ID it is trying to access will not change through its life cycle. This means that if the NF tries to get 1 packets, or 1000000 packets, they will all fail. The manager has now wasted the time to check for that port an extra 999,999 times. I hope that makes sense, but there has to be a way to check at initialization of NF if the port is valid (tell it to change if invalid or something), rather than let the NF attempt to enqueue packets forever, consuming resources from manager

[dennisafa]

@dennisafa this works, tested with your solution, and saw that develop does fail with this edge case. Pktgen is fine. My only thought is that it's pretty redundant to keep checking for this port. If an NF has been initialized, the port ID it is trying to access will not change through its life cycle. This means that if the NF tries to get 1 packets, or 1000000 packets, they will all fail. The manager has now wasted the time to check for that port an extra 999,999 times. I hope that makes sense, but there has to be a way to check at initialization of NF if the port is valid (tell it to change if invalid or something), rather than let the NF attempt to enqueue packets forever, consuming resources from manager

the port to be sent out of is defined by the meta->destination value as seen here in basic_monitor i can imagine that NF's may want flexibility with the port that packets are bursted out of.

[koolzz]

@dennisafa Code looks good, although as @kevindweb pointed out it is a bunch of extra checks. Can we test with flow lookup disabled macro(I believe its still enabled by default) that should show our best perf ratio comparison. If that number is the same then I think we're clear to merge

[dennisafa]

@dennisafa Code looks good, although as @kevindweb pointed out it is a bunch of extra checks. Can we test with flow lookup disabled macro(I believe its still enabled by default) that should show our best perf ratio comparison. If that number is the same then I think we're clear to merge

Here's the perf with the develop branch, flow table disabled:

And with this branch:

[koolzz]

@onvm

[onvm]

@onvm

CI Message

Your results will arrive shortly