Update stats for outgoing packets

[bdevierno1]

Update stats updating during ONVM_NF_ACTION_OUT action.

Summary:

ONVM stats was updating stats.act_out twice.

This will occur when sending out packets out a port. To replicate this run bridge ./go.sh 2 and send packets with speed tester to the bridge nf ./go.sh 1 -d 2 Before: After:

Usage:

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

Test Plan:

Check output of ONVM stats for correct values.

Review:

@sreya519

[onvm]

In response to PR creation

CI Message

Your results will arrive shortly

[kevindweb]

Seems odd, but this actually reduced performance by about 3%. Did this happen on your machine @bdevierno1 ? CI lets this "pass" because it's not huge, but since we have so many PRs that stay above 100%, we probably want to look into this

[twood02]

We don’t want to trust NFs to directly work with ports, plus there are some potential scalability issues (you would need an output queue on the NIC for each NF, which becomes infeasible for large numbers of NFs). Better to just go through the manager and have one output queue per TX thread.

It’s hard to believe the 3% change is real. Have CI test again and see if it goes away.

[kkrama]

I agree that having the NF Manager mediate transmission is important, especially with a number of competing NFs wanting to transmit and compete for the link.

On Thu, Jun 11, 2020 at 2:32 PM Tim Wood notifications@github.com wrote:

We don’t want to trust NFs to directly work with ports, plus there are some potential scalability issues (you would need an output queue on the NIC for each NF, which becomes infeasible for large numbers of NFs). Better to just go through the manager and have one output queue per TX thread.

It’s hard to believe the 3% change is real. Have CI test again and see if it goes away.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sdnfv/openNetVM/pull/227#issuecomment-642938197, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6TUJNXA4253CPK52BFFMDRWFEOTANCNFSM4N3CUWIA .

-- K. K. Ramakrishnan

[kevindweb]

@onvm 3%?

[onvm]

@onvm 3%?

CI Message

Your results will arrive shortly

[kevindweb]

@tim could be compiler optimizations? I dealt with an error last year that was unexplainable and we chalked it up to the compiler...

[dennisafa]

Seems odd, but this actually reduced performance by about 3%. Did this happen on your machine @bdevierno1 ? CI lets this "pass" because it's not huge, but since we have so many PRs that stay above 100%, we probably want to look into this

I think that in general we have fluctuations on different nodes. It could be a variety of reasons including what processes are active on those cores, etc. I don't think it means much. Edit: Though in this case we are modifying the critical path and that variable is now being referenced from a different proc. I also don't really believe it but I'll double check on my own nodes. Also thank you for the clarifications @kkrama @twood02 that makes sense

[kevindweb]

@dennisafa you're right fluctuations do happen, but it's abnormal twice in a row, when I tested another PR this afternoon that had 100+%. I think we should test this on another machine to verify, because CI hasn't reported lower than 97 (except for ben's memory PR) in a long time

[bdevierno1]

@onvm Just out of curiosity.

[onvm]

@onvm Just out of curiosity.

CI Message

Your results will arrive shortly

[bdevierno1]

@onvm WOPR Did you ever play tic-tac-toe?

[onvm]

@onvm WOPR Did you ever play tic-tac-toe?

CI Message

Your results will arrive shortly

[bdevierno1]

Im still not sure why there was a perf drop for speed tester. In the last change I made I switch in the logic of the comparisons as when sending packets out there is a higher probability of if (tx_mgr->mgr_type_t != MGR) { being true.

@kevindweb I did notice a perf drop on my node as well in my earlier change.

[kevindweb]

@bdevierno1 that's an interesting change, weird that it made CI happy again, that leads me to believe it's definitely a compiler-based problem.

[twood02]

I think this is ready for a final review and merge. The performance difference seen is an artifact of the testing, I don't think it is real.