Add per-queue stats to bessctl #1007
Conversation
The command is typically run in a periodic manner, eventually spamming the debug log messages. Since these stats counters are hardly useful (e.g., `bessctl show port` can be used whenever necessary), this log message does not deserve VLOG(1).
... w/ some additional code polishing
the current feature design and implementation are somewhat broken, as: * for the TX direction it only works for the PMDPort * there is no mechanism to turn the feature off to avoid overhead * the metrics are hardly useful: the requested RX batch size is always constant, for TX direction the metrics are mostly meaningless, etc.
... which was intended to be done when BESS was ported from C a long long time ago.
- now `show port` command lists per-queue counters, not only aggregated ones. - new command `monitor queue` has been added.
Codecov Report
@@ Coverage Diff @@
## master #1007 +/- ##
=======================================
Coverage 55.96% 55.96%
=======================================
Files 9 9
Lines 1165 1165
=======================================
Hits 652 652
Misses 513 513 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
LGTM. Thanks! I'll merge if we can get another pair of eyes on this. @krsna1729 @shinae-woo could one of you take a quick look?
core/port.h
Outdated
| @@ -293,6 +293,9 @@ class Port { | |||
|
|
|||
| const PortBuilder *port_builder() const { return port_builder_; } | |||
|
|
|||
| // per-queue stat counters | |||
| QueueStats queue_stats_[PACKET_DIRS][MAX_QUEUES_PER_DIR]; | |||
| @@ -247,7 +253,7 @@ class Port { | |||
| virtual size_t DefaultIncQueueSize() const { return kDefaultIncQueueSize; } | |||
| virtual size_t DefaultOutQueueSize() const { return kDefaultOutQueueSize; } | |||
|
|
|||
| virtual uint64_t GetFlags() const { return 0; } | |||
| virtual DriverFeatures GetFeatures() const { return {}; } | |||
There was a problem hiding this comment.
huh. are there plans to make this real/settable?
|
|
||
| if (qid != -1) or (port.name in has_unknown): | ||
| cli.fout.write('{:<22}{} {}\n'.format(name, inc, out)) | ||
| #cli.fout.write('{:<22}{:>14.1f}{:>10.3f}{:>10.0f} {:>14.1f}{:>10.3f}{:>10.0f}\n'.format(name, *data)) |
There was a problem hiding this comment.
nit: can you remove this commented code?
There was a problem hiding this comment.
@sangjinhan Could you please check the comments and rebase this PR again?
| if (cnt == 0) { | ||
| return {.block = true, .packets = 0, .bits = 0}; | ||
| } | ||
|
|
||
| batch->set_cnt(cnt); |
There was a problem hiding this comment.
nit: Just to prevent future mistake, how about setting this before returning whenif (cnt==0) {... }
| if (cnt == 0) { | ||
| return {.block = true, .packets = 0, .bits = 0}; | ||
| } | ||
|
|
||
| batch->set_cnt(cnt); |
There was a problem hiding this comment.
nit: Just to prevent future mistake, how about setting this before returning whenif (cnt==0) {... }
| // the driver really supports per-queue stats or not. Instead, if all queue | ||
| // stats counters are 0, we assume that it only supports per-port stats | ||
| // Note that this heuristic is safe from potential double counting. | ||
| if (!any) { |
... w/ some code polishing around it. From bessctl:
show portcommand now shows counters for each queue, rather than aggregated ones only.monitor queuehas been added.