[DTrace-devel] [PATCH] bufsize considered too small even when increased sufficiently
Kris Van Hees
kris.van.hees at oracle.com
Thu Oct 29 14:42:01 PDT 2020
On Fri, Oct 23, 2020 at 07:47:53PM -0400, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> The buffer size can be judged too small, even though it is increased
> more than sufficiently. E.g., consider
>
> # dtrace -x bufsize=32 -qn 'BEGIN { trace(1); trace(2); exit(0) }'
> bufsize increased to 4096
> 12
> # dtrace -x bufsize=32 -qn 'BEGIN { trace(1); trace(2);
> trace(3); exit(0) }'
> dtrace: could not enable tracing: Enabling exceeds size of buffer
>
> The first case shows we rounded the buffer size up to a full page.
> The second case shows a small-buffer error even though 4k is plenty.
Although it seems counter-intuitive, I would argue that the behaviour is in
fact correct as-is. There are two different things going on here that you are
combining in looking at the behaviour (and I agree, it is confusing).
The specification of the bufsize sets an explicit limit on the size of the
buffer that the user is expecting. And therefore, when the script(s) need more
than that limit to actually record their data, an erorr is flagged. That is
the expected behaviour when a specific bufsize is set.
The other behaviour is that perf event buffers must comprise complete memory
pages, and thus the size of the backing memory block must be a whole multiple
of the page size. As such, when we are ready to create the buffers we need to
increase the buffer size to the nearest multiple of the page size. Perhaps it
would have been less confusing if I didn't add a message that the buffer size
got increased. After all, the user doesn't actually notice this. So perhaps,
the better patch is to get rid of that message?
> Change dtrace_go() so that it does not check bufsize before calling
> dt_pebs_init(), but simply pass a minimum size in to be checked.
> Add a test for this problem.
>
> Orabug: 32065592
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> libdtrace/dt_peb.c | 34 ++++++++++++-------
> libdtrace/dt_peb.h | 2 +-
> libdtrace/dt_work.c | 15 ++++----
> test/unittest/buffering/tst.increasebufsize.d | 32 +++++++++++++++++
> test/unittest/buffering/tst.increasebufsize.r | 6 ++++
> 5 files changed, 67 insertions(+), 22 deletions(-)
> create mode 100644 test/unittest/buffering/tst.increasebufsize.d
> create mode 100644 test/unittest/buffering/tst.increasebufsize.r
>
> diff --git a/libdtrace/dt_peb.c b/libdtrace/dt_peb.c
> index 0ef1c419..6355605c 100644
> --- a/libdtrace/dt_peb.c
> +++ b/libdtrace/dt_peb.c
> @@ -146,31 +146,35 @@ dt_pebs_exit(dtrace_hdl_t *dtp)
> * monitor all perf event buffers at once. This file descriptor is returned
> * upon success.. Failure is indicated with a -1 return value.
> */
> -int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize)
> +int dt_pebs_init(dtrace_hdl_t *dtp, size_t reqsize, size_t minsize)
> {
> int i;
> int mapfd;
> - size_t num_pages;
> + size_t bufsize;
> dt_ident_t *idp;
> dt_peb_t *pebs;
>
> /*
> * The perf event buffer implementation in the kernel requires that the
> * buffer comprises n full memory pages, where n must be a power of 2.
> - * Since the buffer size is specified in bytes in DTrace, we need to
> - * convert this size (and possibly round it up) to an acceptable value.
> + * Since the buffer size is specified in bytes in DTrace, we possibly
> + * need to round it up to an acceptable value.
> */
> - num_pages = roundup_pow2((bufsize + getpagesize() - 1) / getpagesize());
> - if (num_pages * getpagesize() > bufsize)
> - fprintf(stderr, "bufsize increased to %lu\n",
> - num_pages * getpagesize());
> + bufsize = roundup_pow2((reqsize + getpagesize() - 1) / getpagesize());
> + bufsize *= getpagesize();
> + if (bufsize > reqsize)
> + fprintf(stderr, "bufsize increased to %lu\n", bufsize);
> + if (bufsize < minsize)
> + return dt_set_errno(dtp, EDT_BUFTOOSMALL);
>
> /*
> * Determine the fd for the 'buffers' BPF map.
> + * FIXME: The errno is not right, but this was the existing
> + * behavior and no other EDT_* fits better.
> */
> idp = dt_dlib_get_map(dtp, "buffers");
> if (idp == NULL || idp->di_id == DT_IDENT_UNDEF)
> - return -ENOENT;
> + return dt_set_errno(dtp, EDT_NOMEM);
>
> mapfd = idp->di_id;
>
> @@ -179,7 +183,7 @@ int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize)
> */
> dtp->dt_pebset = dt_zalloc(dtp, sizeof(dt_pebset_t));
> if (dtp->dt_pebset == NULL)
> - return -ENOMEM;
> + return dt_set_errno(dtp, EDT_NOMEM);
>
> /*
> * Allocate the per-CPU perf event buffers.
> @@ -188,13 +192,13 @@ int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize)
> sizeof(struct dt_peb));
> if (pebs == NULL) {
> dt_free(dtp, dtp->dt_pebset);
> - return -ENOMEM;
> + return dt_set_errno(dtp, EDT_NOMEM);
> }
>
> dtp->dt_pebset->pebs = pebs;
>
> dtp->dt_pebset->page_size = getpagesize();
> - dtp->dt_pebset->data_size = num_pages * dtp->dt_pebset->page_size;
> + dtp->dt_pebset->data_size = bufsize;
>
> /*
> * Initialize a perf event buffer for each online CPU.
> @@ -240,5 +244,9 @@ int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize)
> fail:
> dt_pebs_exit(dtp);
>
> - return -1;
> + /*
> + * FIXME: The errno is not right, but this was the existing
> + * behavior and no other EDT_* fits better.
> + */
> + return dt_set_errno(dtp, EDT_NOMEM);
> }
> diff --git a/libdtrace/dt_peb.h b/libdtrace/dt_peb.h
> index 961db93c..03ee7df7 100644
> --- a/libdtrace/dt_peb.h
> +++ b/libdtrace/dt_peb.h
> @@ -42,7 +42,7 @@ typedef struct dt_pebset {
> } dt_pebset_t;
>
> extern void dt_pebs_exit(dtrace_hdl_t *);
> -extern int dt_pebs_init(dtrace_hdl_t *, size_t);
> +extern int dt_pebs_init(dtrace_hdl_t *, size_t, size_t);
>
> #ifdef __cplusplus
> }
> diff --git a/libdtrace/dt_work.c b/libdtrace/dt_work.c
> index 02246219..737b173a 100644
> --- a/libdtrace/dt_work.c
> +++ b/libdtrace/dt_work.c
> @@ -143,7 +143,8 @@ dtrace_status(dtrace_hdl_t *dtp)
> int
> dtrace_go(dtrace_hdl_t *dtp, uint_t cflags)
> {
> - size_t size;
> + size_t reqsize;
> + size_t minsize;
> int err;
>
> if (dtp->dt_active)
> @@ -186,13 +187,11 @@ dtrace_go(dtrace_hdl_t *dtp, uint_t cflags)
> * the buffer. In other words, the buffer needs to be large enough to
> * hold at least one perf-encapsulated trace data record.
> */
> - dtrace_getopt(dtp, "bufsize", &size);
> - if (size == 0 ||
> - size < sizeof(struct perf_event_header) + sizeof(uint32_t) +
> - dtp->dt_maxreclen)
> - return dt_set_errno(dtp, EDT_BUFTOOSMALL);
> - if (dt_pebs_init(dtp, size) == -1)
> - return dt_set_errno(dtp, EDT_NOMEM);
> + dtrace_getopt(dtp, "bufsize", &reqsize);
> + minsize = sizeof(struct perf_event_header) + sizeof(uint32_t) +
> + dtp->dt_maxreclen;
> + if (dt_pebs_init(dtp, reqsize, minsize) == -1)
> + return -1;
>
> BEGIN_probe();
>
> diff --git a/test/unittest/buffering/tst.increasebufsize.d b/test/unittest/buffering/tst.increasebufsize.d
> new file mode 100644
> index 00000000..e4a2fbd5
> --- /dev/null
> +++ b/test/unittest/buffering/tst.increasebufsize.d
> @@ -0,0 +1,32 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION:
> + * A small buffer size will be increased automatically.
> + *
> + * SECTION: Buffers.
> + * Buffers and Buffering/Buffer Sizes;
> + * Options and Tunables/bufsize;
> + */
> +
> +#pragma D option bufsize=16
> +
> +BEGIN
> +{
> + trace(12345); trace(67890);
> + trace(12345); trace(67890);
> + trace(12345); trace(67890);
> + trace(12345); trace(67890);
> + trace(12345); trace(67890);
> + trace(12345); trace(67890);
> + trace(12345); trace(67890);
> + trace(12345); trace(67890);
> + trace(12345); trace(67890);
> + trace(12345); trace(67890);
> + exit(0);
> +}
> diff --git a/test/unittest/buffering/tst.increasebufsize.r b/test/unittest/buffering/tst.increasebufsize.r
> new file mode 100644
> index 00000000..0616c681
> --- /dev/null
> +++ b/test/unittest/buffering/tst.increasebufsize.r
> @@ -0,0 +1,6 @@
> + FUNCTION:NAME
> + :BEGIN 12345 67890 12345 67890 12345 67890 12345 67890 12345 67890 12345 67890 12345 67890 12345 67890 12345 67890 12345 67890
> +
> +-- @@stderr --
> +dtrace: script 'test/unittest/buffering/tst.increasebufsize.d' matched 1 probe
> +bufsize increased to 4096
> --
> 2.18.4
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list