Skip to content
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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add per-queue stats to bessctl #1007

wants to merge 9 commits into from

Conversation

sangjinhan
Copy link
Member

... w/ some code polishing around it. From bessctl:

  • show port command now shows counters for each queue, rather than aggregated ones only.
  • a new command monitor queue has been added.

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
Copy link

codecov bot commented Jul 28, 2020

Codecov Report

Merging #1007 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5250185...22c9c09. Read the comment docs.

Copy link
Contributor

@melvinw melvinw left a 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];
Copy link
Contributor

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 {}; }
Copy link
Contributor

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))
Copy link
Contributor

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?

Copy link
Member

@shinae-woo shinae-woo left a 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);
Copy link
Member

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);
Copy link
Member

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants