Added nf tag and core to stats by kevindweb · Pull Request #110 · sdnfv/openNetVM

kevindweb · 2019-04-23T01:41:50Z

Added nf tag from the recent pr, and the core number to manager stats

Summary:

Changed and tested the 3 verbosity levels. Spacing should be fine. Since nf->tag is no more than 14 characters (excluding \0), the beginning has that many spaces reserved. Core number is similar to sid and iid, so it doesn't change much.

Usage:
Run onvm regularly, and add NFs to see them on the stdout output

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

TODO before merging :

PR is ready for review
Maybe wait for shared cpu?

Test Plan:

Review:

@koolzz

(optional) Subscribers: << @-mention people who probably care about these changes >>

onvm · 2019-04-23T01:41:58Z

In response to PR creation

CI Message

Your results will arrive shortly

koolzz · 2019-04-23T01:45:47Z

+            "drop\n------------------------------------------------------------------------------------------------------------------\n";
        NF_MSG[1] =
-            "\nNF IID / SID     rx_pps  /  tx_pps            rx  /  tx                out   /    tonf     /   drop\n"
+            "\nNF TAG         IID / SID / CORE    rx_pps  /  tx_pps     rx_drop  /  tx_drop         out   /    tonf     /   drop\n"


Define these lines in a header file

koolzz · 2019-04-23T01:47:01Z

Also please include screenshots

onvm · 2019-04-23T01:49:06Z

In response to PR creation

CI Message

Run successful see results:
[Results from nimbnode30]
Median TX pps for Speed Tester: 35126881

Linter Failed

examples/arp_response/arp_response.c:284: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 1
examples/flow_table/openflow.h:50: Using deprecated casting style. Use static_cast(...) instead [readability/casting] [4]
examples/flow_table/openflow.h:569: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:634: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:771: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:804: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:865: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:926: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 7
examples/nf_router/nf_router.c:173: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 1
onvm/onvm_mgr/onvm_pkt.c:68: Are you taking an address of a cast? This is dangerous: could be a temp var. Take the address before doing the cast, rather than after [runtime/casting] [4]
Total errors found: 1
onvm/onvm_mgr/onvm_stats.c:329: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:330: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:332: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:334: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:414: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
onvm/onvm_mgr/onvm_stats.c:414: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:415: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
onvm/onvm_mgr/onvm_stats.c:415: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:418: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:423: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
onvm/onvm_mgr/onvm_stats.c:423: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:424: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
onvm/onvm_mgr/onvm_stats.c:424: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:427: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:429: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
onvm/onvm_mgr/onvm_stats.c:429: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 16
onvm/onvm_nflib/onvm_pkt_common.c:98: Are you taking an address of a cast? This is dangerous: could be a temp var. Take the address before doing the cast, rather than after [runtime/casting] [4]
Total errors found: 1

kevindweb · 2019-04-23T02:29:44Z

Here are the stats output from using basic_monitor and speed_tester with the new stats.

As you can see the NF tag has to reserve the same size buffer, regardless of the length of the tag

koolzz · 2019-04-23T03:12:33Z

Looks nice, I'll try to review after SD

koolzz · 2019-04-26T02:29:46Z

As we discussed in the meeting:

Configure -v stats, shift the 2nd line so it lines up with the first line (I think leaving the fiels below iid/sid/cores is fine rn.
Test edge cases with max values for these (max length tag, max sid/max iid, etc)
Code cleanup (I've left some suggestions)

koolzz · 2019-04-26T02:30:29Z


                if (verbosity_level == ONVM_RAW_STATS_DUMP) {
-                        fprintf(stats_out, "%s,%u,%u,%" PRIu64 ",%" PRIu64 ",%" PRIu64 ",%" PRIu64 ",%" PRIu64
+                        fprintf(stats_out, "%s,%s,%u,%u,%u,%" PRIu64 ",%" PRIu64 ",%" PRIu64 ",%" PRIu64 ",%" PRIu64


I suggest we define this String the same way we define ONVM_STATS_MSG and others, it takes up like 4 lines here and isn't very readable.

well same with others obv

koolzz · 2019-04-26T02:31:46Z

Also we'll probably want to wait for the shared cpu changes so we can have 1 nice stats update instead of adding another pr later.

kevindweb · 2019-04-27T20:44:52Z

Here is the updated screenshot of the normal and verbose onvm stats, respectively.

koolzz · 2019-05-11T21:01:21Z

Waiting on merging this because shared cpu has additional stats

koolzz · 2019-05-18T21:30:36Z

@nks5295 brought up a good idea, we can add the number of children into the -v stats

kevindweb · 2019-05-26T02:45:38Z

@onvm testy test

onvm · 2019-05-26T02:45:45Z

@onvm testy test

CI Message

Your results will arrive shortly

onvm

@onvm testy test

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Speed tester performance check passed
❌ Linter Failed (please fix style errors)

[Results from nimbnode30]

Median TX pps for Speed Tester: 35238454
Performance rating - 100.68% (compared to 35000000 average)

Linter Output

onvm/onvm_mgr/onvm_stats.c:465: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
onvm/onvm_mgr/onvm_stats.c:465: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:466: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
onvm/onvm_mgr/onvm_stats.c:466: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:469: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:472: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:474: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
onvm/onvm_mgr/onvm_stats.c:474: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:475: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
onvm/onvm_mgr/onvm_stats.c:475: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:478: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:480: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
onvm/onvm_mgr/onvm_stats.c:480: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:523: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:526: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:535: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 16
onvm/onvm_mgr/onvm_stats.h:63: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:66: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:67: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 3

koolzz · 2019-05-26T21:16:34Z

 #define ONVM_STR_STATS_WEB "web"

+extern const char *NF_MSG[3];
+#define ONVM_STATS_MSG "\nNF TAG         IID / SID / CORE    rx_pps  /  tx_pps        rx_drop  /  tx_drop"\


Lets try

#define ONVM_STATS_MSG \ "\nNF TAG IID / SID / CORE rx_pps / tx_pps rx_drop / tx_drop out / tonf / drop\n"\ ...

This way its cleaner to read. Also I feel like its fine if it goes over the char limit for these definitions

Ok sounds good, and line limits are going to be the only lint errors in this pr which is fine I guess

kevindweb · 2019-05-26T22:49:15Z

Here's the updated screenshots from the different verbosity levels and shared cpu mode.

For regular stdout, no special arguments:

./go.sh 0,1,2,3 3 0xF0 -a 0x7f000000000 -s stdout

For shared cpu mode enabled (./go.sh 0,1,2,3 3 0xF0 -a 0x7f000000000 -s stdout -c):

For verbose and shared cpu (./go.sh 0,1,2,3 3 0xF0 -a 0x7f000000000 -s stdout -c -v):

koolzz

Instead of working on the bottom in verbose mode can we have

children / parent / State
. 
.
.
    5    /    3   /  W

Not sure how to fit the names but you get the idea (could we shorten those somehow, if not just add space heh)

Regarding other code comments try moving all the incredibly long "%-14s %2u / %-2u / %2u" Strings into the .h file. That might make the code a bit cleaner

koolzz · 2019-05-27T01:15:43Z

-                    "\n---------------------------------------------------------------------------------------------------"
-                    "\n";
-        }
+                    "\nNF TAG         IID / SID / CORE    rx_pps  /  tx_pps        rx_drop  /  tx_drop"


Move to the .h file

koolzz · 2019-05-27T01:16:44Z

+                                act_next, act_buffer, act_returned, num_wakeups, wakeup_rate, active);
                } else if (verbosity_level == 2) {
-                        fprintf(stats_out, "NF  %2u / %-2u  - %9" PRIu64 " / %-9" PRIu64 "  %11" PRIu64 " / %-11" PRIu64
+                        fprintf(stats_out, "%-14s %2u  /  %-2u / %2u    %9" PRIu64 " / %-9" PRIu64 "   %11" PRIu64 " / %-11" PRIu64


Try defining ""%-14s %2u / %-2u / %2u %9" PRIu64 " / %-9" PRIu64 " %11" PRIu64 " / %-11" PRIu64......."
in the .h file. Might make this a bit cleaner.

If you think that moving that into the .h file is harder to maintain you can keep it. You call

kevindweb · 2019-05-27T01:48:40Z

I'll fix the newest change request tomorrow, but here's the updated shared cpu verbose mode:

kevindweb · 2019-05-27T17:15:16Z

@onvm tell me how it is

onvm · 2019-05-27T17:15:23Z

@onvm tell me how it is

CI Message

Your results will arrive shortly

koolzz · 2019-05-27T17:21:00Z

Should we shorten the lines so its a bit better alligned?

 IID / SID / CORE
 PNT / S|W / CHLD

I nknow its less intuitive but this was the panel looks nice

onvm

@onvm tell me how it is

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Speed tester performance check passed
❌ Linter Failed (please fix style errors)

[Results from nimbnode30]

Median TX pps for Speed Tester: 38619871
Performance rating - 110.34% (compared to 35000000 average)

Linter Output

onvm/onvm_mgr/onvm_stats.c:405: If an else has a brace on one side, it should have it on both [readability/braces] [5]
onvm/onvm_mgr/onvm_stats.c:496: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:497: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:501: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:502: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:506: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.c:510: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 7
onvm/onvm_mgr/onvm_stats.h:64: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:68: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:69: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:76: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 4

koolzz · 2019-05-27T17:25:28Z

I think we can do the above comment^ + basically only display the 3rd line in verbose mode (it'll only have shared cpu stats) when shared CPU is enabled.

kevindweb · 2019-05-27T18:36:51Z

Here is the updated picture of verbose and shared cpu

kevindweb · 2019-05-27T18:38:25Z

@onvm how do you like me now?

onvm · 2019-05-27T18:38:33Z

@onvm how do you like me now?

CI Message

Your results will arrive shortly

onvm

@onvm how do you like me now?

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Speed tester performance check passed
❌ Linter Failed (please fix style errors)

[Results from nimbnode30]

Median TX pps for Speed Tester: 38593746
Performance rating - 110.27% (compared to 35000000 average)

Linter Output

onvm/onvm_mgr/onvm_stats.h:64: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:70: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:77: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 3

kevindweb · 2019-05-27T19:33:29Z

@onvm are the updates working?

onvm · 2019-05-27T19:33:36Z

@onvm are the updates working?

CI Message

Your results will arrive shortly

onvm

@onvm are the updates working?

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Speed tester performance check passed
❌ Linter Failed (please fix style errors)

[Results from nimbnode30]

Median TX pps for Speed Tester: 38611526
Performance rating - 110.32% (compared to 35000000 average)

Linter Output

onvm/onvm_mgr/onvm_stats.h:64: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:70: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:77: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 3

koolzz

Looked over it, looks good. See suggestions.

…ate_stats Conflicts: onvm/onvm_mgr/onvm_stats.c

koolzz · 2019-05-31T17:39:46Z

@kevindweb fixed the conflicts, should be good to merge now

kevindweb · 2019-05-31T21:44:24Z

@onvm how many errors are here?

onvm · 2019-05-31T21:44:32Z

@onvm how many errors are here?

CI Message

Your results will arrive shortly

onvm

@onvm how many errors are here?

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Speed tester performance check passed
❌ Linter Failed (please fix style errors)

[Results from nimbnode30]

Median TX pps for Speed Tester: 39244590
Performance rating - 112.13% (compared to 35000000 average)

Linter Output

onvm/onvm_mgr/onvm_stats.h:64: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:68: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:69: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:73: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:75: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 5

koolzz · 2019-05-31T22:31:02Z

@onvm .

onvm · 2019-05-31T22:31:11Z

@onvm .

CI Message

Your results will arrive shortly

onvm

@onvm .

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Speed tester performance check passed
❌ Linter Failed (please fix style errors)

[Results from nimbnode30]

Median TX pps for Speed Tester: 39321769
Performance rating - 112.35% (compared to 35000000 average)

Linter Output

onvm/onvm_mgr/onvm_stats.h:62: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:63: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:65: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:66: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:67: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:69: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:70: Lines should be <= 120 characters long [whitespace/line_length] [5]
onvm/onvm_mgr/onvm_stats.h:72: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 8

This PR features additions and fixes to the onvm_mgr console stats: - Adds NF tag, parent id, core id, children count - Shortens the NF state to a single upper case letter - S (sleeping) or W (working) Commit log: * Added nf tag and core to stats * Made constants for the stats in the header file * Fixed verbose stat line-up * Fixed service id totals stats * Fixed style of output * Fixed shared cpu verbose * Moved print formatters to onvm_stats.h * Fixed up stat lines * Fixed macro styling

Added nf tag and core to stats

b0ca0bb

koolzz reviewed Apr 23, 2019

View reviewed changes

Made constants for the stats in the header file

73a38c2

koolzz added the enhancement 🚀 label Apr 26, 2019

koolzz reviewed Apr 26, 2019

View reviewed changes

kevindweb added 3 commits April 27, 2019 14:28

Merge branch 'develop' of github.com:sdnfv/openNetVM into update_stats

e0c7ad4

Fixed verbose stat line-up

553b4eb

Fixed service id totals stats

8965cc5

onvm suggested changes May 26, 2019

View reviewed changes

koolzz added this to the ONVM 19.05 Release milestone May 26, 2019

koolzz reviewed May 26, 2019

View reviewed changes

Merge develop into pr and fix conflicts

7bc15f8

Fixed style of output

087c05b

koolzz requested changes May 27, 2019

View reviewed changes

Fixed shared cpu verbose

a1740ad

Moved print formatters to onvm_stats.h

c76624b

onvm suggested changes May 27, 2019

View reviewed changes

Fixed up stat lines

5646a94

onvm suggested changes May 27, 2019

View reviewed changes

koolzz reviewed May 29, 2019

View reviewed changes

Comment thread onvm/onvm_mgr/onvm_stats.h Outdated

kevindweb and others added 2 commits May 30, 2019 19:42

Fixed macro styling

e172e5f

Merge branch 'develop' of https://github.com/sdnfv/openNetVM into upd…

e9cc708

…ate_stats Conflicts: onvm/onvm_mgr/onvm_stats.c

onvm suggested changes May 31, 2019

View reviewed changes

koolzz added 2 commits May 31, 2019 17:14

Fix console macros

cf05319

Fix stats output macro consistency

0c7c636

onvm suggested changes May 31, 2019

View reviewed changes

koolzz merged commit b8c4dbe into sdnfv:develop May 31, 2019

kevindweb deleted the update_stats branch March 21, 2020 21:04

Conversation

kevindweb commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Test Plan:

Review:

Uh oh!

onvm commented Apr 23, 2019

CI Message

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koolzz commented Apr 23, 2019

Uh oh!

onvm commented Apr 23, 2019

CI Message

Linter Failed

Uh oh!

kevindweb commented Apr 23, 2019

Uh oh!

koolzz commented Apr 23, 2019

Uh oh!

koolzz commented Apr 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koolzz commented Apr 26, 2019

Uh oh!

kevindweb commented Apr 27, 2019

Uh oh!

koolzz commented May 11, 2019

Uh oh!

koolzz commented May 18, 2019

Uh oh!

kevindweb commented May 26, 2019

Uh oh!

onvm commented May 26, 2019

CI Message

Uh oh!

onvm left a comment

Choose a reason for hiding this comment

CI Message

Linter Output

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevindweb commented May 26, 2019

Uh oh!

koolzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevindweb commented May 27, 2019

Uh oh!

kevindweb commented May 27, 2019

Uh oh!

onvm commented May 27, 2019

CI Message

Uh oh!

koolzz commented May 27, 2019

Uh oh!

onvm left a comment

Choose a reason for hiding this comment

CI Message

Linter Output

Uh oh!

koolzz commented May 27, 2019

Uh oh!

kevindweb commented May 27, 2019

Uh oh!

kevindweb commented May 27, 2019

kevindweb commented Apr 23, 2019 •

edited

Loading