-
Notifications
You must be signed in to change notification settings - Fork 156
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
Add per-queue stats to bessctl #1007
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally! 🙏
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you remove this commented code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
... w/ some code polishing around it. From bessctl:
show port
command now shows counters for each queue, rather than aggregated ones only.monitor queue
has been added.