lgtm. thanks!

[buildbot, ok to test]

I should add this to cpuunclaimed, too (which sleeps during the kpoll loop to reduce overhead, which fills the buffers).

@brendangregg I can add cpuunclaimed. Do you want 64 pages or more? Or -b?

Taking a look at others...

I'd take it off execsnoop (doubt it needs it, unless you think otherwise). The potentially high buffer output tools where it could be added are: biosnoop, cpuunclaimed, dcsnoop, the *slower.py tools (especially when run with an arg of "0"), statsnoop, tcplife. If you want, go ahead and add it to them, thanks.

I don't have an opinion yet on what they should all be. I guess we can start with 64 pages.

I'd still avoid the -b unless we really need to, like with trace.

Hi @markdrayton! All function arguments are optional in Lua, and set to null if not passed in. So in order to emulate the Python API, all we have to do is:

function PerfEventArray:_open_perf_buffer(cpu, callback, ctype, pages_per_buffer)
  -- [...]
  -- default to `pages_per_buffer`, or 8 if not given
  local reader = libbcc.bpf_open_perf_buffer(_cb, nil, -1, cpu, pages_per_buffer or 8)

  -- [...]

I think I also figured out the Lua side. Update coming.

I couldn't work out the Lua stuff :-( @vmg, the problem is not with _perf_buffer_open, but perf_buffer_open, because it has some tricky vararg magic:

function PerfEventArray:open_perf_buffer(callback, data_type, ...)

I don't see how I can put an optional arg either side of the ... without confusing Lua. Based on http://stackoverflow.com/questions/6022519/define-default-values-for-function-arguments, I also tried approximating named parameters:

function PerfEventArray:open_perf_buffer(args, ...)
  setmetatable(args, {__index={page_cnt=8}})
  local callback = args[1]
  local data_type = args[2]
  assert(data_type, "a data type is needed for callback conversion")
  local ctype = ffi.typeof(data_type.."*", ...)
  for i = 0, Posix.cpu_count() - 1 do
    self:_open_perf_buffer(i, callback, ctype, args.page_cnt)
  end
end

but that fails like so:

[ERROR] bcc.lua:2756: wrong number of type parameters
stack traceback:
        [C]: in function 'typeof'
        bcc.lua:2756: in function 'open_perf_buffer'
        [..]

@vmg, any suggestions here?

@brendangregg in the meantime I have updated the other scripts you mentioned.

Apparently I did something stupid here and closed the PR.

I was wondering!

@markdrayton I understand now. Try this on for size:

function PerfEventArray:open_perf_buffer(callback, data_type, data_params, buffer_size)
  assert(data_type, "a data type is needed for callback conversion")
  local ctype = ffi.typeof(data_type.."*", unpack(data_params or {}))
  for i = 0, Posix.cpu_count() - 1 do
    self:_open_perf_buffer(i, callback, ctype, buffer_size)
  end
end

Here's what's what going on: the varargs to open_perf_buffers are actually optional values for the parameterized types used to build the native FFI type (see http://luajit.org/ext_ffi_semantics.html, section "Parameterized Types"). They allow you to safely template the given C type, and they are passed verbatim to ffi.typeof to do the templating.

If we want to add more optional arguments to this API, our only option is to actually pass in the data params in a table, and add the buffer_size argument after that table. Since we're now passing the types as a single table, we need to "splat" them (or unpack in Lua parlance) so ffi.typeof can receive those verbatim.

Here's how the API was used before this change:

tbl:open_perf_buffer(my_callback, "struct kernel_type { uint8_t buffer[$]; }", SIZE_IN_KERNEL)
tbl:open_perf_buffer(my_callback, "struct custom_type { $ buffer[$]; }", "uint8_t", 1024)

Notice how the templating is type-safe. In this example we're using it to reproduce a type from the kernel and "grab" the SIZE_IN_KERNEL constant in case it changes between kernel versions. It can also be used to typedef an arbitrary type.

And here's how you'd use the API now:

tbl:open_perf_buffer(my_callback, "struct kernel_type { uint8_t buffer[$]; }", {SIZE_IN_KERNEL}, 4096)
tbl:open_perf_buffer(my_callback, "struct custom_type { $ buffer[$]; }", {"uint8_t", 1024}, 4096)
tbl:open_perf_buffer(my_callback, "struct non_templated_type { int foo; }", nil, 4096)

Here we're passing in a buffer_size of 4096, besides the template arguments. That means that the template arguments must always be stored in an array/table, even for the case of a single template element. You also must pass in nil (or {}) in the case where there are no template elements and you want to pass a custom buffer size.

So yes, this is a breaking change. But I think it makes the API more consistent and more understandable. The old varargs were confusing, particularly since they were not documented.

One last thing to note: we're passing buffer_size verbatim to _open_perf_buffer. We always want to do that -- we want the nil to trickle down as much as we can before we or DEFAULT_VALUE it, to ensure that the default value is always set in a single place (in this case, the internal _open_perf_buffer.

Hope that clears things up and makes the build green. Let me know if I can help with anything else!

@vmg hooray! That seems to work nicely. I just updated the Lua API and any Lua scripts that call perf_buffer_open then force pushed.

@4ast, @brendangregg: this should be good to go now.

re: the red tests: It looks like something's up with the fc25 buildbot — the errors seem to relate to basic git operations and not to the code itself:

https://buildbot.iovisor.org/jenkins/job/bcc-pr/label=fc25/94/console

🎉👌

[buildbot, retest this please]

[buildbot, test this please]

Excellent!

Revisiting this: We'd rather fix this in bcc if possible (#1033) rather than each tool, but there may be exceptions. In other words, if you are about to copy-n-paste this into another tool, please read #1033 instead.