Page MenuHomeFreeBSD

Remove the arbitrary limit on DTrace buffers
Needs ReviewPublic

Authored by gnn on Feb 17 2017, 10:50 PM.

Details

Reviewers
markj
arun.thomas_gmail.com
rwatson
Group Reviewers
DTrace
Summary

Turn the limit into a sysctl (kern.dtrace.buffer_maxsize)

Test Plan
  1. dtrace -b 32m -n 'syscall:::' | head

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7548
Build 7701: arc lint + arc unit

Event Timeline

gnn updated this revision to Diff 25339.Feb 17 2017, 10:50 PM
gnn retitled this revision from to Remove the arbitrary limit on DTrace buffers.
gnn updated this object.
gnn edited the test plan for this revision. (Show Details)
rstone added a subscriber: rstone.Feb 17 2017, 10:55 PM
rstone added inline comments.
sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
12122

I like that this is tunable but isn't this allocated per-cpu? As I understand it the original heuristic was attempting to prevent us from allocating too much on large-cpu count machines.

Maybe that's irrelevant because for us "large cpu count" implies "huge amounts of memory"

gnn added inline comments.Feb 17 2017, 11:00 PM
sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
12122

I don't know of any systems with 40 cores that don't also have 128G or more of RAM. Also,note that the tunable is 16m, so even with 40 cores that's "only" 640M, and no one needs more than 640...M

markj added inline comments.Feb 17 2017, 11:10 PM
sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
12125

I don't really understand what this comment means. We don't sleep here if there's a memory shortage, so an allocation failure should just cause us to release any already-allocated buffers and bail. Am I missing something?

sys/cddl/dev/dtrace/dtrace_sysctl.c
99

SYSCTL_QUAD is wrong for this: dtrace_strsize_default is a size_t which is 32 bits in ILP32. I guess SYSCTL_ULONG is better.

100

typo: "szie"

markj added inline comments.Feb 17 2017, 11:18 PM
sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
12125

Hmm. libdtrace also needs to allocate buffers of this size for when it copies the kernel buffers, so one can, for instance, trigger the OOM killer by using a large enough buffer size. In that case, we should probably set buffer_maxsize based on physmem/ncpu. See r261122.

gnn marked 2 inline comments as done.Feb 17 2017, 11:19 PM
gnn added inline comments.
sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
12125

If you like I can also remove the comment. That's not original to me.

gnn updated this revision to Diff 25340.Feb 17 2017, 11:22 PM

Address two of markj's suggestions.

gnn marked 2 inline comments as done.Mar 21 2017, 4:55 AM
gnn added inline comments.
sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
12125

In 261122 all the limits were removed in libdtrace. Are you suggesting that due to that change I should at least do a protection setting of:

if (size > physmem * PAGE_SIZE)

at this point?

erj added a subscriber: erj.May 14 2018, 5:13 PM