[DTrace-devel] [PATCH 31/38] Fix dt_pebs_init() call
eugene.loh at oracle.com
eugene.loh at oracle.com
Thu Jun 27 05:38:57 UTC 2024
From: Eugene Loh <eugene.loh at oracle.com>
The function had a few issues, mainly with its comments,
ranging from typos to incorrect descriptions of behavior.
The function has only one caller. The enforcement of a
size minimum was problematic in a number of respects:
- it was missing 4 bytes
- it was enforcing the minimum before the size was
increased by the caller
- it was checking dt_pebs_init()==-1,
missing errors with other negative return values
So change the function to accept a minimum and change its
return values.
Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
---
libdtrace/dt_peb.c | 27 ++++++++++-----------
libdtrace/dt_peb.h | 2 +-
libdtrace/dt_work.c | 15 +++++++-----
test/unittest/options/err.b-too-low.d | 22 +++++++++--------
test/unittest/options/err.bufsize-too-low.d | 22 +++++++++--------
test/unittest/options/tst.b.d | 22 +++++++++--------
test/unittest/options/tst.bufsize.d | 22 +++++++++--------
7 files changed, 71 insertions(+), 61 deletions(-)
diff --git a/libdtrace/dt_peb.c b/libdtrace/dt_peb.c
index 5268f089..4983ba91 100644
--- a/libdtrace/dt_peb.c
+++ b/libdtrace/dt_peb.c
@@ -136,18 +136,14 @@ dt_pebs_exit(dtrace_hdl_t *dtp)
}
/*
- * Initialize the perf event buffers (one per online CPU). Each buffer will
- * the given number of pages (i.e. the total size of each buffer will be
- * num_pages * getpagesize()). The allocated memory for each buffer is mmap'd
- * so the kernel can write to it, and its representative file descriptor is
- * recorded in the 'buffers' BPF map so that BPF code knows where to write
- * trace data for a specific CPU.
+ * Initialize the perf event buffers, one per online CPU. Each buffer will
+ * have num_pages * getpagesize()). The dt_peb_open() call mmaps the allocated
+ * memory so the kernel can write to it. The file descriptor is recorded in
+ * the 'buffers' BPF map and added to the event polling file descriptor.
*
- * An event polling file descriptor is created as well, and it is configured to
- * monitor all perf event buffers at once. This file descriptor is returned
- * upon success.. Failure is indicated with a -1 return value.
+ * The return value indicates success (0) or failure (-1).
*/
-int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize)
+int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize, size_t bufsizemin)
{
int i;
int mapfd;
@@ -166,12 +162,15 @@ int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize)
fprintf(stderr, "bufsize increased to %lu\n",
num_pages * getpagesize());
+ if (num_pages * getpagesize() < bufsizemin)
+ return dt_set_errno(dtp, EDT_BUFTOOSMALL);
+
/*
* Determine the fd for the 'buffers' BPF map.
*/
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); // FIXME we used to do something more akin to ENOENT
mapfd = idp->di_id;
@@ -180,7 +179,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.
@@ -189,7 +188,7 @@ 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;
@@ -241,5 +240,5 @@ int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize)
fail:
dt_pebs_exit(dtp);
- return -1;
+ return dt_set_errno(dtp, EDT_NOMEM); // FIXME need something better
}
diff --git a/libdtrace/dt_peb.h b/libdtrace/dt_peb.h
index e0f408f2..9ad23252 100644
--- a/libdtrace/dt_peb.h
+++ b/libdtrace/dt_peb.h
@@ -43,7 +43,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 69a86358..7a0eb1da 100644
--- a/libdtrace/dt_work.c
+++ b/libdtrace/dt_work.c
@@ -267,18 +267,21 @@ dtrace_go(dtrace_hdl_t *dtp, uint_t cflags)
return dt_set_errno(dtp, errno);
/*
- * We need enough space for the pref_event_header, a 32-bit size, a
+ * We need enough space for the perf_event_header, a 32-bit size, a
* 4-byte gap, and the largest trace data record we may be writing to
* the buffer. In other words, the buffer needs to be large enough to
* hold at least one perf-encapsulated trace data record.
+ *
+ * While dt_pebs_init() rounds the requested size up, size==0 is a
+ * special case.
*/
dtrace_getopt(dtp, "bufsize", &size);
- if (size == 0 ||
- size < sizeof(struct perf_event_header) + sizeof(uint32_t) +
- dtp->dt_maxreclen)
+ if (size == 0)
return dt_set_errno(dtp, EDT_BUFTOOSMALL);
- if (dt_pebs_init(dtp, size) == -1)
- return dt_set_errno(dtp, EDT_NOMEM);
+ if (dt_pebs_init(dtp, size,
+ sizeof(struct perf_event_header) + sizeof(uint32_t)
+ + 4 + dtp->dt_maxreclen) == -1)
+ return -1;
/*
* We must initialize the aggregation consumer handling before we
diff --git a/test/unittest/options/err.b-too-low.d b/test/unittest/options/err.b-too-low.d
index bb77e37c..f62155dd 100644
--- a/test/unittest/options/err.b-too-low.d
+++ b/test/unittest/options/err.b-too-low.d
@@ -1,6 +1,6 @@
/*
* Oracle Linux DTrace.
- * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2022, 2024, 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.
*/
@@ -12,19 +12,21 @@
*/
/*
- * We use a buffer size of 59 because that should be just too small to hold the
- * trace records generated in this script:
- * - perf_event_header (40 bytes)
- * - size (4 bytes)
- * - gap (4 bytes)
- * - EPID (4 bytes)
- * - tag (4 bytes)
- * - exit value (4 bytes)
+ * We need over 4k bytes for the 4 string trace records generated in this script
+ * plus some meta data.
+ * A bufsize of 1 to 4096 bytes is rounded up to 4096 and is insufficient.
+ * A bufsize of 4097 to 8192 bytes is rounded up to 8192 and is sufficient.
*/
-/* @@runtest-opts: -b59 */
+/* @@runtest-opts: -b4096 */
+
+#pragma D option strsize=1024
BEGIN
{
+ trace("abc");
+ trace("def");
+ trace("ghi");
+ trace("jkl");
exit(0);
}
diff --git a/test/unittest/options/err.bufsize-too-low.d b/test/unittest/options/err.bufsize-too-low.d
index bbbdb5c5..25efdf72 100644
--- a/test/unittest/options/err.bufsize-too-low.d
+++ b/test/unittest/options/err.bufsize-too-low.d
@@ -1,6 +1,6 @@
/*
* Oracle Linux DTrace.
- * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2022, 2024, 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.
*/
@@ -12,19 +12,21 @@
*/
/*
- * We use a buffer size of 59 because that should be just too small to hold the
- * trace records generated in this script:
- * - perf_event_header (40 bytes)
- * - size (4 bytes)
- * - gap (4 bytes)
- * - EPID (4 bytes)
- * - tag (4 bytes)
- * - exit value (4 bytes)
+ * We need over 4k bytes for the 4 string trace records generated in this script
+ * plus some meta data.
+ * A bufsize of 1 to 4096 bytes is rounded up to 4096 and is insufficient.
+ * A bufsize of 4097 to 8192 bytes is rounded up to 8192 and is sufficient.
*/
-/* @@runtest-opts: -xbufsize=59 */
+/* @@runtest-opts: -xbufsize=4096 */
+
+#pragma D option strsize=1024
BEGIN
{
+ trace("abc");
+ trace("def");
+ trace("ghi");
+ trace("jkl");
exit(0);
}
diff --git a/test/unittest/options/tst.b.d b/test/unittest/options/tst.b.d
index 57fa030d..3bf08edc 100644
--- a/test/unittest/options/tst.b.d
+++ b/test/unittest/options/tst.b.d
@@ -1,6 +1,6 @@
/*
* Oracle Linux DTrace.
- * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2022, 2024, 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.
*/
@@ -12,19 +12,21 @@
*/
/*
- * We use a buffer size of 60 because that should be the exact size necessary
- * to hold the trace records generated in this script:
- * - perf_event_header (40 bytes)
- * - size (4 bytes)
- * - gap (4 bytes)
- * - EPID (4 bytes)
- * - tag (4 bytes)
- * - exit value (4 bytes)
+ * We need over 4k bytes for the 4 string trace records generated in this script
+ * plus some meta data.
+ * A bufsize of 1 to 4096 bytes is rounded up to 4096 and is insufficient.
+ * A bufsize of 4097 to 8192 bytes is rounded up to 8192 and is sufficient.
*/
-/* @@runtest-opts: -b60 */
+/* @@runtest-opts: -b4097 */
+
+#pragma D option strsize=1024
BEGIN
{
+ trace("abc");
+ trace("def");
+ trace("ghi");
+ trace("jkl");
exit(0);
}
diff --git a/test/unittest/options/tst.bufsize.d b/test/unittest/options/tst.bufsize.d
index 96b0f1b8..23af81aa 100644
--- a/test/unittest/options/tst.bufsize.d
+++ b/test/unittest/options/tst.bufsize.d
@@ -1,6 +1,6 @@
/*
* Oracle Linux DTrace.
- * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2022, 2024, 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.
*/
@@ -12,19 +12,21 @@
*/
/*
- * We use a buffer size of 60 because that should be the exact size necessary
- * to hold the trace records generated in this script:
- * - perf_event_header (40 bytes)
- * - size (4 bytes)
- * - gap (4 bytes)
- * - EPID (4 bytes)
- * - tag (4 bytes)
- * - exit value (4 bytes)
+ * We need over 4k bytes for the 4 string trace records generated in this script
+ * plus some meta data.
+ * A bufsize of 1 to 4096 bytes is rounded up to 4096 and is insufficient.
+ * A bufsize of 4097 to 8192 bytes is rounded up to 8192 and is sufficient.
*/
-/* @@runtest-opts: -xbufsize=60 */
+/* @@runtest-opts: -xbufsize=4097 */
+
+#pragma D option strsize=1024
BEGIN
{
+ trace("abc");
+ trace("def");
+ trace("ghi");
+ trace("jkl");
exit(0);
}
--
2.18.4
More information about the DTrace-devel
mailing list