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

[wip] snabb top --yang: dump RFC7223 interface stats as JSON #886

Merged
merged 21 commits into from
Jul 13, 2016
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
b34c3ee
engine: Set shm path to "app/$name"
lukego Feb 22, 2016
94ff234
Amendments to #766:
eugeneia Apr 12, 2016
aac0c8c
Merge PR #766 (engine: Set shm path to "app/$name") into yang
eugeneia Apr 12, 2016
fad0f43
core.counter: Qualify counter names using `shm.resolve'.
eugeneia Apr 13, 2016
7ed4ed0
snabb top: add `--app' option to print app counters.
eugeneia Apr 12, 2016
eb9005b
snabb top: unlink own shm tree to avoid clutter.
eugeneia Apr 13, 2016
5fbe0d6
vhost_user: Add RFC 7223 app counters.
eugeneia Apr 14, 2016
8bb3215
Intel_app: Add RFC 7223 app counters.
eugeneia Apr 14, 2016
7a55478
snabb top: Add --link parameter to list link counters.
eugeneia Apr 14, 2016
dde5da2
core.app: Put app counters under "counters/<app>", update snabb top.
eugeneia Apr 14, 2016
924ff4e
lib.json: Import JSON4Lua 1.0.0, include encode functionality.
eugeneia Apr 18, 2016
8e34093
lib.macaddress: Support numeric initialization; add method to get num…
eugeneia Apr 18, 2016
5f9efd2
core.link: Create “discontinuity-time” counters.
eugeneia Apr 18, 2016
7b39148
snabb top: add `--yang' option to print YANG model as JSON.
eugeneia Apr 19, 2016
8984741
snabb top --yang: Represent uint64_t as decimal string.
eugeneia Apr 20, 2016
ee00d16
[core.lib] Generalize `timer' to optionally accept 'repeating'
eugeneia Apr 28, 2016
45490b8
Revert "Intel_app: Add RFC 7223 app counters."
eugeneia Apr 28, 2016
f0ed10b
intel_app: expose per-pciaddress statistics in `counters/<pciaddress>'.
eugeneia Apr 28, 2016
aca8064
Merge branch 'master' into yang-local
eugeneia May 13, 2016
c186591
lib.protocol.ethernet: Add n_mcast, branch-free Multicast predicate.
eugeneia Apr 25, 2016
b09e843
Fix for f0ed10b: require macaddress module.
eugeneia May 27, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions src/apps/intel/intel_app.lua
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ local zone = require("jit.zone")
local basic_apps = require("apps.basic.basic_apps")
local ffi = require("ffi")
local lib = require("core.lib")
local counter = require("core.counter")
local pci = require("lib.hardware.pci")
local register = require("lib.hardware.register")
local macaddress = require("lib.macaddress")
local intel10g = require("apps.intel.intel10g")
local freelist = require("core.freelist")
local receive, transmit, full, empty = link.receive, link.transmit, link.full, link.empty
Expand Down Expand Up @@ -36,6 +38,7 @@ end
-- Create an Intel82599 App for the device with 'pciaddress'.
function Intel82599:new (arg)
local conf = config.parse_app_arg(arg)
local self = setmetatable({counters = {}}, Intel82599)

if conf.vmdq then
if devices[conf.pciaddr] == nil then
Expand All @@ -45,12 +48,26 @@ function Intel82599:new (arg)
local poolnum = firsthole(dev.vflist)-1
local vf = dev.pf:new_vf(poolnum)
dev.vflist[poolnum+1] = vf
return setmetatable({dev=vf:open(conf)}, Intel82599)
self.dev = vf:open(conf)
else
local dev = intel10g.new_sf(conf):open()
if not dev then return null end
return setmetatable({dev=dev, zone="intel"}, Intel82599)
self.dev = assert(intel10g.new_sf(conf):open(), "Can not open device.")
self.zone = "intel"
end

self.counters['type'] = counter.open('type')
self.counters['discontinuity-time'] = counter.open('discontinuity-time')
self.counters['in-octets'] = counter.open('in-octets')
self.counters['in-discards'] = counter.open('in-discards')
self.counters['out-octets'] = counter.open('out-octets')
counter.set(self.counters['type'], 0x1000) -- Hardware interface
counter.set(self.counters['discontinuity-time'], C.get_unix_time())
if conf.vmdq then
self.counters['phys-address'] = counter.open('phys-address')
counter.set(self.counters['phys-address'],
macaddress:new(conf.macaddr):int())
end

return self
end

function Intel82599:stop()
Expand All @@ -71,6 +88,9 @@ function Intel82599:stop()
if close_pf then
close_pf:close()
end

-- delete counters
for name, _ in pairs(self.counters) do counter.delete(name) end
end


Expand All @@ -79,6 +99,11 @@ function Intel82599:reconfig(arg)
assert((not not self.dev.pf) == (not not conf.vmdq), "Can't reconfig from VMDQ to single-port or viceversa")

self.dev:reconfig(conf)

if conf.vmdq then
counter.set(self.counters['phys-address'],
macaddress:new(conf.macaddr):int())
end
end

-- Allocate receive buffers from the given freelist.
Expand All @@ -96,6 +121,13 @@ function Intel82599:pull ()
transmit(l, self.dev:receive())
end
self:add_receive_buffers()
if self.dev.rxstats then
counter.set(self.counters['in-octets'],
bit.lshift(self.dev.pf.qs.QBRC_H[self.dev.rxstats]()+0LL, 32)
+ self.dev.pf.qs.QBRC_L[self.dev.rxstats]())
counter.set(self.counters['in-discards'],
self.dev.pf.qs.QPRDC[self.dev.rxstats]())
end
end

function Intel82599:add_receive_buffers ()
Expand All @@ -116,6 +148,11 @@ function Intel82599:push ()
end
end
self.dev:sync_transmit()
if self.dev.txstats then
Copy link
Member

Choose a reason for hiding this comment

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

How frequently do these counters need to be synchronized with hardware?

This code is synchronizing them every breath but that adds up to a significant number of PCIe accesses even e.g. for NICs that are completely idle. This may cause performance degradation in scenarios that we don't have CI performance coverage on at the moment e.g. app network with very many NICs where most are idle but some are active.

One alternative would be to use a timer to update every e.g. one millisecond.

Copy link
Member Author

@eugeneia eugeneia Apr 21, 2016

Choose a reason for hiding this comment

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

Makes sense, honestly I had/have trouble getting a good understanding of Intel10G stat registers code. Could definitely use a thorough review with SHM stats in mind (e.g. get rid of Intel10G.dev.get_rx/tx_stats() altogether). In this PR I attempted to be make mostly non-disruptive (light bolt-on) changes.

counter.set(self.counters['out-octets'],
bit.lshift(self.dev.pf.qs.QBTC_H[self.dev.txstats]()+0LL, 32)
+ self.dev.pf.qs.QBTC_L[self.dev.txstats]())
end
end

-- Report on relevant status and statistics.
Expand Down
36 changes: 36 additions & 0 deletions src/apps/vhost/vhost_user.lua
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ local lib = require("core.lib")
local link = require("core.link")
local main = require("core.main")
local memory = require("core.memory")
local counter = require("core.counter")
local pci = require("lib.hardware.pci")
local ethernet = require("lib.protocol.ethernet")
local net_device= require("lib.virtio.net_device")
local timer = require("core.timer")
local ffi = require("ffi")
Expand Down Expand Up @@ -52,6 +54,15 @@ function VhostUser:new (args)
else
self.qemu_connect = self.client_connect
end
-- initialize counters
self.counters = {}
for _, name in ipairs({'type', 'discontinuity-time',
'in-octets', 'in-unicast', 'in-multicast', 'in-discards',
'out-octets', 'out-unicast', 'out-multicast'}) do
self.counters[name] = counter.open(name)
end
counter.set(self.counters['type'], 0x1001) -- Virtual interface
counter.set(self.counters['discontinuity-time'], C.get_unix_time())
return self
end

Expand All @@ -68,6 +79,9 @@ function VhostUser:stop()
self:free_mem_table()

if self.link_down_proc then self.link_down_proc() end

-- delete counters
for name, _ in pairs(self.counters) do counter.delete(name) end
end

function VhostUser:pull ()
Expand All @@ -86,6 +100,28 @@ function VhostUser:push ()
end
end

function VhostUser:tx_callback (p)
counter.add(self.counters['out-octets'], packet.length(p))
if ethernet:is_mcast(packet.data(p)) then
counter.add(self.counters['out-multicast'])
else
counter.add(self.counters['out-unicast'])
end
end

function VhostUser:rx_callback (p)
counter.add(self.counters['in-octets'], packet.length(p))
if ethernet:is_mcast(packet.data(p)) then
counter.add(self.counters['in-multicast'])
else
counter.add(self.counters['in-unicast'])
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to introduce this potentially unbiased branch onto the "fast path" for Virtio-net?

The risk I see is that on a workload with 50/50 mix of unicast/multicast traffic you will take the penalty of both a LuaJIT side-trace and also a CPU branch-misprediction half of the time. This could be significant and we don't currently have performance test coverage for such a workload.

One alternative would be to write this branch-free (using arithmetic, bitwise operators, and min/max). Sketch:

-- Set unicast and multicast to 0 and 1 as appropriate.
local multicast = bit.band(packet.data[0], 1)
local unicast = 1 - multicast
counter.add(self.counters['in-multicast'], multicast)
counter.add(self.counters['in-unicast'], unicast)

This way the same instructions would execute every time and only the values would change.


function VhostUser:rxdrop_callback (p)
counter.add(self.counters['in-discards'])
end

-- Try to connect to QEMU.
function VhostUser:client_connect ()
return C.vhost_user_connect(self.socket_path)
Expand Down
44 changes: 43 additions & 1 deletion src/core/app.lua
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ local lib = require("core.lib")
local link = require("core.link")
local config = require("core.config")
local timer = require("core.timer")
local shm = require("core.shm")
local histogram = require('core.histogram')
local counter = require("core.counter")
local zone = require("jit.zone")
Expand Down Expand Up @@ -68,6 +69,8 @@ end
-- Run app:methodname() in protected mode (pcall). If it throws an
-- error app will be marked as dead and restarted eventually.
local function with_restart (app, method)
local oldshm = shm.path
shm.path = app.shmpath
if use_restart then
-- Run fn in protected mode using pcall.
local status, err = pcall(method, app)
Expand All @@ -78,6 +81,7 @@ local function with_restart (app, method)
else
method(app)
end
shm.path = oldshm
end

-- Restart dead apps.
Expand Down Expand Up @@ -155,7 +159,13 @@ function apply_config_actions (actions, conf)
-- Table of functions that execute config actions
local ops = {}
function ops.stop (name)
if app_table[name].stop then app_table[name]:stop() end
if app_table[name].stop then
local shmorig = shm.path
shm.path = app_table[name].shmpath
app_table[name]:stop()
shm.path = shmorig
shm.unlink(app_table[name].shmpath)
end
end
function ops.keep (name)
new_app_table[name] = app_table[name]
Expand All @@ -165,7 +175,10 @@ function apply_config_actions (actions, conf)
function ops.start (name)
local class = conf.apps[name].class
local arg = conf.apps[name].arg
local shmpath, shmorig = "counters/"..name, shm.path
shm.path = shmpath
local app = class:new(arg)
shm.path = shmorig
if type(app) ~= 'table' then
error(("bad return value from app '%s' start() method: %s"):format(
name, tostring(app)))
Expand All @@ -174,6 +187,7 @@ function apply_config_actions (actions, conf)
app.appname = name
app.output = {}
app.input = {}
app.shmpath = shmpath
new_app_table[name] = app
table.insert(new_app_array, app)
app_name_to_index[name] = #new_app_array
Expand Down Expand Up @@ -493,6 +507,34 @@ function selftest ()
assert(app_table.app3 == orig_app3) -- should be the same
main({duration = 4, report = {showapps = true}})
assert(app_table.app3 ~= orig_app3) -- should be restarted
-- Test shm.path management
print("shm.path management")
local S = require("syscall")
local App4 = {zone="test"}
function App4:new ()
local c = counter.open('test')
counter.set(c, 42)
counter.commit()
return setmetatable({test_counter = c},
{__index = App4})
end
function App4:pull ()
assert(counter.read(self.test_counter) == 42, "Invalid counter value")
counter.add(self.test_counter)
end
function App4:stop ()
assert(counter.read(self.test_counter) == 43, "Invalid counter value")
counter.delete('test')
end
local c_counter = config.new()
config.app(c_counter, "App4", App4)
configure(c_counter)
main({done = function () return app_table.App4.test_counter end})
assert(S.stat(shm.root.."/"..shm.resolve("counters/App4/test")),
"Missing : counters/App4/test")
configure(config.new())
assert(not S.stat(shm.root.."/"..shm.resolve("counters/App4")),
"Failed to unlink counters/App4")
print("OK")
end

Expand Down
12 changes: 7 additions & 5 deletions src/core/counter.lua
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ local private = {}
local numbers = {} -- name -> number

function open (name, readonly)
if numbers[name] then error("counter already opened: " .. name) end
local qname = shm.resolve(name)
if numbers[qname] then error("counter already opened: " .. qname) end
local n = #public+1
if readonly then
public[n] = shm.open(name, counter_t, readonly)
Expand All @@ -53,21 +54,22 @@ function open (name, readonly)
public[n] = shm.create(name, counter_t)
private[n] = ffi.new(counter_t)
end
numbers[name] = n
numbers[qname] = n
return private[n]
end

function delete (name)
local number = numbers[name]
if not number then error("counter not found for deletion: " .. name) end
local qname = shm.resolve(name)
local number = numbers[qname]
if not number then error("counter not found for deletion: " .. qname) end
-- Free shm object
shm.unmap(public[number])
-- If we "own" the counter for writing then we unlink it too.
if public[number] ~= private[number] then
shm.unlink(name)
end
-- Free local state
numbers[name] = false
numbers[qname] = false
public[number] = false
private[number] = false
end
Expand Down
3 changes: 3 additions & 0 deletions src/core/link.lua
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,16 @@ function new (name)
for _, c in ipairs(counternames) do
r.stats[c] = counter.open("counters/"..name.."/"..c)
end
counter.set(counter.open("counters/"..name.."/discontinuity-time"),
C.get_unix_time())
return r
end

function free (r, name)
for _, c in ipairs(counternames) do
counter.delete("counters/"..name.."/"..c)
end
counter.delete("counters/"..name.."/discontinuity-time")
shm.unmap(r)
shm.unlink("links/"..name)
end
Expand Down
Loading